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

fix 32 bit support, add i686-unknown-linux-gnu CI target #113

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Jul 5, 2023

ci: add a 32bit target build w/ cross.

This commit introduces a CI target that uses cross to cross-compile for a i686-unknown-linux-gnu target to ensure we maintain 32bit arch compatibility. Unfortunately I don't believe GitHub actions has a native 32bit runner so cross-compilation is our best bet.

Before applying the subsequent fix commit this task fails with the error reported in #112:

   Compiling rustls-webpki v0.101.0 (/home/runner/work/webpki/webpki)
error[E0080]: evaluation of constant value failed
   --> src/der.rs:246:45
    |
246 | const LONG_FORM_LEN_FOUR_BYTES_MAX: usize = (1 << (8 * 4)) - 1;
    |                                             ^^^^^^^^^^^^^^ attempt to shift left by `32_i32`, which would overflow

der: fix DER max size constants for 32bit targets.

This commit introduces a fix to the LONG_FORM_LEN_XXX_BYTES_MAX constants in the der module to avoid overflow on 32bit targets.

Resolves #112

cargo: bump version to 0.101.1

Prepare for a release that includes the 32bit arch fix.

@cpu cpu self-assigned this Jul 5, 2023
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #113 (d4aa00a) into main (d9b4338) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #113   +/-   ##
=======================================
  Coverage   95.36%   95.36%           
=======================================
  Files          15       15           
  Lines        3346     3346           
=======================================
  Hits         3191     3191           
  Misses        155      155           
Impacted Files Coverage Δ
src/der.rs 98.42% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

.github/workflows/ci.yml Show resolved Hide resolved
with:
persist-credentials: false
- uses: dtolnay/rust-toolchain@nightly
- run: cargo install cross

Choose a reason for hiding this comment

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

Question: This seems to add about a minute to the CI runtime. Do you think its worth caching the install or using cargo-binstall for a pre-compiled version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hesitant to add any 3rd party actions since I think the project has taken a pretty conservative stance on those in the past.

I'm not familiar with cargo-binstall but it does sound interesting. Maybe we could consider the speed optimizations in a follow-up once the breakage is addressed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like https://github.com/marketplace/actions/install-development-tools would maybe be the best way to integrate (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The "is this secure" part of the FAQ (https://github.com/cargo-bins/cargo-binstall#faq) gives me some pause :-/

Choose a reason for hiding this comment

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

Their description doesn't seem horribly problematic (they go from crates.io -> GitHub), but its probably not worth the 1 minute of CI time then if we're both slightly hesitant. I was hoping there was a more clear answer for pre-compiled binaries.

@cpu
Copy link
Member Author

cpu commented Jul 5, 2023

Tacked on a version bump commit with the assumption we'll want to turn around a v0.102.0 v0.101.1 release when this lands.

src/der.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Jul 5, 2023

I will do a second release prep checkist PR afterwards with a README changelog update.

@djc
Copy link
Member

djc commented Jul 5, 2023

I think this can be 0.101.1 instead of 0.102.0? No semver compatibility was hurt in this PR.

@cpu
Copy link
Member Author

cpu commented Jul 5, 2023

I think this can be 0.101.1 instead of 0.102.0? No semver compatibility was hurt in this PR.

Oops, you're right. Fixing.

Prepare for a release that includes the 32bit arch fix.
@cpu
Copy link
Member Author

cpu commented Jul 5, 2023

Merging this since I'd like to get the issue solved ASAP. If @ctz has further refinements in mind I will chase them separately.

@cpu cpu added this pull request to the merge queue Jul 5, 2023
Merged via the queue into rustls:main with commit 91cab3f Jul 5, 2023
21 checks passed
@cpu cpu deleted the cpu-112-fix-32bit-arch branch July 5, 2023 20:19
@cpu
Copy link
Member Author

cpu commented Jul 5, 2023

I will do a second release prep checkist PR afterwards with a README changelog update.

#114

@cpu cpu mentioned this pull request Jul 6, 2023
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.

Broken on 32-bit platforms
4 participants