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: fix minimal-versions resolution #593

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Oct 31, 2023

To avoid nightly regressions breaking the build, the CI configuration has been updated to only use nightly for resolving Cargo.lock by using cargo update -Z minimal-versions.

Previously, it was running cargo check which would attempt to compile all of the dependencies and the code, which is why the diagnostic bug was triggered. By avoiding any kind of code compilation using nightly we can avoid such regressions in the future.

Additionally, the clippy job has been changed to run on the latest stable release (1.73.0) rather than nightly, which will prevent future clippy lints from breaking the build. Instead, they can be addressed when clippy is updated.

@rozbb
Copy link
Contributor

rozbb commented Oct 31, 2023

Unused import stuff is fixed in #590

@tarcieri
Copy link
Contributor Author

It is? How?

Regardless, this approach should be more futureproof in regard to any other nightly regressions.

@@ -85,7 +87,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: dtolnay/rust-toolchain@nightly
- uses: dtolnay/rust-toolchain@1.73.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should definitely pin clippy rather than running nightly clippy

@rozbb
Copy link
Contributor

rozbb commented Oct 31, 2023

Some of the contents of field.rs was pub use, but the module itself is only pub(crate). I suppose the new warning could be a bug, but it still points out correctly that there was a lot of superfluous pub use.

I agree about the CI being more robust. My only concern is that this adds another chore (updating pinned nightly). Is there a buildabot type automation for this?

@tarcieri
Copy link
Contributor Author

@rozbb if you merge #590 I can unpin nightly

@rozbb
Copy link
Contributor

rozbb commented Oct 31, 2023

Merged

To avoid nightly regressions breaking the build, the CI configuration
has been updated to *only* use nightly for resolving Cargo.lock by using
`cargo update -Z minimal-versions`.

Previously, it was running `cargo check` which would attempt to compile
all of the dependencies and the code, which is why the diagnostic bug
was triggered. By avoiding any kind of code compilation using nightly we
can avoid such regressions in the future.

Additionally, the clippy job has been changed to run on the latest
stable release (1.73.0) rather than nightly, which will prevent future
clippy lints from breaking the build. Instead, they can be addressed
when clippy is updated.
@tarcieri tarcieri force-pushed the fix-minimal-versions-resolution branch from e760df2 to 0c026ec Compare October 31, 2023 15:37
@tarcieri
Copy link
Contributor Author

Updated with the nightly pin removed, although notably if we don't pin nightly it's a source of nondeterminism in the build.

I still think the other changes simplify/stabilize the build if we don't do that, but personally I find it frustrating whenever the build breaks due to things unrelated to my changes.

@rozbb
Copy link
Contributor

rozbb commented Oct 31, 2023

I agree. If we had a more streamlined way of addressing it separately from our PRs then I'd go for it. Only reason I think we should have nightly in the CI is bc it forces us to make changes for what's coming down the pike into stable.

If you still think it's more reasonable to pin, though, then I'm happy to do it

@tarcieri
Copy link
Contributor Author

Let's cross that bridge when we get there (again), I guess.

At least these changes will limit the scope of the failures to the nightly test.

@rozbb rozbb merged commit 3c85f77 into main Oct 31, 2023
44 checks passed
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.

None yet

2 participants