Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support 32bits targets #1811

Merged
merged 5 commits into from
Oct 6, 2022
Merged

Support 32bits targets #1811

merged 5 commits into from
Oct 6, 2022

Conversation

LukeMathWalker
Copy link
Contributor

@LukeMathWalker LukeMathWalker commented Oct 5, 2022

Motivation and Context

Fixes #1810

Description

We replace all AtomicU64 instances with AtomicUsize.
This is a breaking change for aws-smithy-client since NeverService::num_call is exposed via its public API.

There are some minor tweaks in the test suite of the Python server to make sure that the test suite compiles on a MacOS machine.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Comment on lines +76 to +83
// `is_listener` on `Socket` is only available on certain platforms.
// In particular, this fails to compile on MacOS.
#[cfg(any(
target_os = "android",
target_os = "freebsd",
target_os = "fuchsia",
target_os = "linux",
))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still run the socket_can_be_cloned test below in MacOS, it looks like the is_listener call in it is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the intention behind the test - was the idea to explicitly verify that the cloned socket is in a listening state after cloning? @crisidev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the intention.

@crisidev
Copy link
Contributor

crisidev commented Oct 5, 2022

I did not want to approve this, since it is still WIP, but I clicked the wrong button :(

@crisidev
Copy link
Contributor

crisidev commented Oct 5, 2022

How do I remove the approval? Anyway, not a big deal. We need @jdisanti or @Velfi approval here.

@82marbag 82marbag requested a review from crisidev October 5, 2022 11:17
@LukeMathWalker LukeMathWalker marked this pull request as ready for review October 5, 2022 11:19
@LukeMathWalker LukeMathWalker requested review from a team as code owners October 5, 2022 11:19
@LukeMathWalker
Copy link
Contributor Author

I put it as WIP because I am researching the CI side of things, but I'll probably just open a separate PR - that's likely to take longer and be somewhat hairy (QEMU 😢 ).

Comment on lines +14 to +28
[[smithy-rs]]
message = """
Replace all usages of `AtomicU64` with `AtomicUsize` to support 32bit targets.
"""
references = ["smithy-rs#1811"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server"}
author = "LukeMathWalker"

[[smithy-rs]]
message = """
Replace all usages of `AtomicU64` with `AtomicUsize` to support 32bit targets.
"""
references = ["smithy-rs#1811"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"}
author = "LukeMathWalker"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's breaking only for the client - let me know if there are better ways to convey it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this conveys the problem 👍🏼

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber
Copy link
Contributor

hlbarber commented Oct 5, 2022

There's a question here of: is this NeverService intended for public use? Or is it just used as a mock client for this crates tests. If the later then perhaps we should #[cfg(test)] the whole module?

@LukeMathWalker
Copy link
Contributor Author

I assumed the intention was to have it public as a utility to be used by customer for testing their own code.
If that's the case, it'd probably still makes sense to have it behind a dedicated feature flag (e.g. test_utils).

@crisidev
Copy link
Contributor

crisidev commented Oct 5, 2022

I think Jon can answer this question.. For the time being I would just fix the issue with 32bit targets and address the possibility of moving everything under #[cfg(test] flag. Same goes for improving the CI.

Comment on lines +76 to +83
// `is_listener` on `Socket` is only available on certain platforms.
// In particular, this fails to compile on MacOS.
#[cfg(any(
target_os = "android",
target_os = "freebsd",
target_os = "fuchsia",
target_os = "linux",
))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still run the socket_can_be_cloned test below in MacOS, it looks like the is_listener call in it is unnecessary.

@jdisanti
Copy link
Collaborator

jdisanti commented Oct 5, 2022

There's a question here of: is this NeverService intended for public use? Or is it just used as a mock client for this crates tests. If the later then perhaps we should #[cfg(test)] the whole module?

I don't know for certain. It's public right now, so I assume the intent was to provide it broadly to customers. I'm not sure how useful it would be though, since if you replace the client middleware with it, then you won't be able to test timeouts anyway since it will replace the timeout middleware. I suppose you could manually recreate the entire chain with it at the end.

@LukeMathWalker
Copy link
Contributor Author

I see @jdisanti.
In the interest of unblocking the customers on powerpc - should we move forward with this PR and track the fate of NeverService in a separate issue?

@crisidev
Copy link
Contributor

crisidev commented Oct 5, 2022

I see @jdisanti. In the interest of unblocking the customers on powerpc - should we move forward with this PR and track the fate of NeverService in a separate issue?

I think it is fair to track NeverService in a separate issue.

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@LukeMathWalker LukeMathWalker merged commit 1b2b42d into main Oct 6, 2022
@LukeMathWalker LukeMathWalker deleted the 32-bits branch October 6, 2022 09:17
hlbarber pushed a commit that referenced this pull request Oct 7, 2022
* Replace AtomicU64 with AtomicUsize to prevent compilation issues on 32 bits platforms.

* Make sure that Rust tests compile on MacOS.

* Add CHANGELOG next entry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support compiling aws-smithy-client on 32bit CPU architecture
5 participants