Add runtime architecture testing for x86_64 and ARM #4428
Conversation
vcjana
force-pushed
the
crc-fast
branch
2 times, most recently
from
bcc2ced to
24b01eb
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
landonxjames
left a comment
landonxjames
left a comment
There was a problem hiding this comment.
This generally looks good. Couple of questions in the comments.
The last thing that I would like to see is a test run with the new workflow with crc-fast pinned to the problematic version (1.4 I think?) that fails. It would be nice to have some proof that these new tests would have caught the issue we saw that caused us to pin the dependency in the first place.
.github/workflows/ci.yml
Outdated
| run: cargo install cross --git https://github.com/cross-rs/cross | ||
| - name: Test smithy-rs runtime on ${{ matrix.target }} | ||
| shell: bash | ||
| run: cross test --target ${{ matrix.target }} --manifest-path "rust-runtime/Cargo.toml" --workspace --exclude aws-smithy-http-server-python --exclude aws-smithy-http-server-typescript --exclude aws-smithy-experimental --exclude aws-smithy-http-client |
There was a problem hiding this comment.
We probably do want to test aws-smithy-http-client across architectures. I don't think it does anything really arch specific, but some of the dependencies (rustls, aws-lc) probably do and it is a fundamental enough crate that I would like as much testing of it as possible. Why was it excluded, was it hitting a failure on a particular architecture?
There was a problem hiding this comment.
These doctests fail on all architectures (including x86_64) because aws_smithy_runtime isn't available during doctest compilation.
Error: error[E0433]: failed to resolve: use of unresolved module or unlinked crate aws_smithy_runtime
Why this surfaced: The existing CI doesn't run doctests for this crate. Using cross test --workspace exposed this pre-existing issue.
Impact: This is not architecture-specific - it's a pre-existing doctest compilation issue that was already present but not caught by CI. The ignore annotations prevent these broken doctests from blocking the architecture tests.
No broader doctest disabling is needed - just these 3 specific examples that reference unavailable dependencies.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
Created test PR #4446 with crc-fast pinned to 1.4. Once CI runs, it should demonstrate the ARM tests failing with SIGILL, proving these architecture tests would have caught the issue before merge. |
As suggested in review, aws-smithy-http-client should be tested across architectures since its dependencies (rustls, aws-lc) have arch-specific implementations.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
This still looks fine, but looking at #4446 it does not appear to have failed due to the expected SIGILL, but on an unrelated runtime version check.
I believe this is because you set crc-fast = "1.4". In the Rust ecosystem the default dependency behavior allows semver compatible updates, so crc-fast = "1.4" really means:
1.4.0 <= crc-fast version <2.0.0
You can read more about how to specify dependencies in Cargo here.
For the test of this we would want to set crc-fast = "=1.4"
Note I'm trying to work around the Docker image layer issue we are hitting again in #4456 so hopefully you can be unblocked on testing tomorrow
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
2 participants
Summary
Adds multi-architecture testing to smithy-rs CI to catch architecture-specific issues like crc-fast 1.4 SIGILL on ARM before merge.
Changes
test-runtime-architecturesjob to CI workflowcrossfor cross-compilation and testingWhy its needed?
The crc-fast 1.4 SIGILL bug was only caught after release because smithy-rs only tested on x86_64. This change would have caught it before merge by testing on ARM.
Testing