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

[CI] Check semver compatibility with all target platforms, not just the host platform #357

Closed
joshlf opened this issue Sep 8, 2023 · 8 comments · Fixed by #1523
Closed
Labels
compatibility-nonbreaking Changes that are (likely to be) non-breaking experience-medium This issue is of medium difficulty, and requires some experience help wanted Extra attention is needed

Comments

@joshlf
Copy link
Member

joshlf commented Sep 8, 2023

We use cargo-semver-checks-action in CI to ensure we do not accidentally break semver compatibility:

# Check semver compatibility with the most recently-published version on
# crates.io. We do this in the matrix rather than in its own job so that it
# gets run on different targets. Some of our API is target-specific (e.g.,
# SIMD type impls), and so we need to run on each target.
#
# TODO(https://github.com/obi1kenobi/cargo-semver-checks-action/issues/54):
# Currently we don't actually do anything with `matrix.target`, so we're
# just duplicating work by running this job multiple times, each time
# targetting the host platform.
- name: Check semver compatibility
uses: obi1kenobi/cargo-semver-checks-action@v2
with:
# Don't semver check zerocopy-derive; as a proc macro, it doesn't have
# an API that cargo-semver-checks can understand.
package: zerocopy
feature-group: all-features
# TODO: Set this to the specific nightly we have pinned in CI. Not a big
# deal since this isn't affected by the trybuild stderr files, which is
# the reason we need to pin to a specific nightly.
rust-toolchain: nightly
if: matrix.crate == 'zerocopy' && matrix.features == '--all-features' && matrix.toolchain == 'nightly'

This check occurs within our build matrix, so it's re-run for each target:

target: [
"i686-unknown-linux-gnu",
"x86_64-unknown-linux-gnu",
"arm-unknown-linux-gnueabi",
"aarch64-unknown-linux-gnu",
"powerpc-unknown-linux-gnu",
"powerpc64-unknown-linux-gnu",
"riscv64gc-unknown-linux-gnu",
"mips-unknown-linux-gnu",
"mips64-unknown-linux-gnuabi64",
"s390x-unknown-linux-gnu",
"wasm32-wasi"
]

However, we aren't actually making use of target, meaning we are re-running the exact same check 11 times on the Github Actions host target (x86_64-unknown-linux-gnu).

We need to modify this CI job to respect the value of target. To do this, see here: obi1kenobi/cargo-semver-checks-action#54 (comment)

@joshlf joshlf added help wanted Extra attention is needed experience-medium This issue is of medium difficulty, and requires some experience compatibility-nonbreaking Changes that are (likely to be) non-breaking labels Sep 8, 2023
@Sh0g0-1758
Copy link
Contributor

Hello, @joshlf @jswrenn . Can you please assign me this issue. I am interested in working on this.

@joshlf
Copy link
Member Author

joshlf commented Oct 23, 2023

Thanks @Sh0g0-1758! Assigned.

@memark
Copy link
Contributor

memark commented Jun 30, 2024

I've started looking into this. I made some additional comments in the linked issue obi1kenobi/cargo-semver-checks-action#54

I basically see three ways forward here:

  • get cargo-semver-checks-action to add a pass-thru property in the form of target
  • not use the action; install, build and run cargo semver-checks --target ourselves
  • add a .cargo/config file with [build] target = ... to the downloaded repo before running the action (I only tried this briefly, but the file at least appears to be respected.)

@joshlf
Copy link
Member Author

joshlf commented Jul 9, 2024

Based on obi1kenobi/cargo-semver-checks-action#54 (comment), I think that a PR to cargo-semver-checks-action is the way to go. If that turns out to be unexpectedly difficult, it wouldn't be a huge deal for us to just use cargo semver-checks --target directly. Thanks for investigating this!

@memark
Copy link
Contributor

memark commented Jul 14, 2024

Update:
My PR in that repo was merged today. I'll have a look at the associated cache key bug as well, and then I'll come back to finally solve this issue.

joshlf added a commit that referenced this issue Jul 15, 2024
@joshlf
Copy link
Member Author

joshlf commented Jul 15, 2024

Amazing, thank you for that!! It turned out to be a one-line change on our side, so I went ahead and implemented it: #1523

Thanks for unblocking this!

@memark
Copy link
Contributor

memark commented Jul 15, 2024

Glad I could help!

(As mentioned, there is still a chance that we're running into a caching issue in that action with different targets, but I'll get to that shortly.)

@joshlf
Copy link
Member Author

joshlf commented Jul 15, 2024

(As mentioned, there is still a chance that we're running into a caching issue in that action with different targets, but I'll get to that shortly.)

Sounds good! For performance reasons, we only run a subset of tests when a PR is pending (we run the full set of tests in the merge queue), so it's possible that when we try to merge #1523, that bug will crop up. Fingers crossed...

joshlf added a commit that referenced this issue Aug 1, 2024
Credit to @memark for landing obi1kenobi/cargo-semver-checks-action#82,
which unblocked this!

Per #1565, we do not run on wasm32-wasi yet.

Closes #357
github-merge-queue bot pushed a commit that referenced this issue Aug 1, 2024
Credit to @memark for landing obi1kenobi/cargo-semver-checks-action#82,
which unblocked this!

Per #1565, we do not run on wasm32-wasi yet.

Closes #357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-nonbreaking Changes that are (likely to be) non-breaking experience-medium This issue is of medium difficulty, and requires some experience help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants