-
Notifications
You must be signed in to change notification settings - Fork 190
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
Support 32bits targets #1811
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
// `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", | ||
))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the intention.
I did not want to approve this, since it is still WIP, but I clicked the wrong button :( |
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 😢 ). |
[[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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍🏼
A new generated diff is ready to view.
A new doc preview is ready to view. |
There's a question here of: is this |
I assumed the intention was to have it public as a utility to be used by customer for testing their own code. |
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 |
// `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", | ||
))] |
There was a problem hiding this comment.
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.
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. |
I see @jdisanti. |
I think it is fair to track |
A new generated diff is ready to view.
A new doc preview is ready to view. |
* Replace AtomicU64 with AtomicUsize to prevent compilation issues on 32 bits platforms. * Make sure that Rust tests compile on MacOS. * Add CHANGELOG next entry.
Motivation and Context
Fixes #1810
Description
We replace all
AtomicU64
instances withAtomicUsize
.This is a breaking change for
aws-smithy-client
sinceNeverService::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
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.