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

Ensure that MSRV tests do not regress #1593

Merged
merged 1 commit into from
Dec 12, 2021
Merged

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Nov 16, 2021

When testing with older versions of rustc, there's a CI failure mode
when our dependency, which used to be compatible with that Rust version,
publishes a new version which can't be compiled using that old Rust
anymore. That's pretty unfortunate, as that means that third parties can
break our CI without any changes to the code in this repo.

To protect against that, let's use Cargo.lock on CI, but only when
testing against older versions. For stable Rust, we continue to generate
Cargo.lock with freshest dependencies. Implementation wise, we save
known-good Cargo.lock as Cargo.lock.msrv, and just copy that in every
relevant CI job.

To get a working Cargo.lock.msrv, I run cargo +nightly generate-lockfile -Zminimal-versions -- that is guaranteed to support
the oldest Rust version we can.

@matklad
Copy link
Contributor Author

matklad commented Nov 16, 2021

See #1592 for the demonstration that this approach wouldn've work with the recent bitflags update.

One thing I am not sure about if we should have this cp Cargo.lock.msrv Cargo.lock thing, or if we should just commit .msrv as a proper Cargo.lock. The latter approach simplifies CI, as there's only one job which is tested against stable Rust. However, I've went with copying -- while more typing, I think it makes it more clear what we are doing here.

@matklad matklad force-pushed the msrv-ci-for-real branch 2 times, most recently from 939dcc5 to 31626fa Compare November 16, 2021 11:10
@matklad
Copy link
Contributor Author

matklad commented Nov 16, 2021

Of course #1592 just works, while this one has some failures I can't make heads or tails of... Debuging.

@matklad matklad force-pushed the msrv-ci-for-real branch 5 times, most recently from 3e72c93 to 4d00259 Compare November 16, 2021 12:19
@matklad
Copy link
Contributor Author

matklad commented Nov 16, 2021

Ok, the last failrue looks like they'll actually fire with the current master, and that I am just unlucky. I've silenced them in the latest commit. I think this PR is good to go, but I can also rebase it on master if someone fixes the deprecations there :D

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks ok, but could you please rebase on master to fix the CI problems?

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Didn't take too long for this technique to run into a problem. The build is failing on Redox because redox-syscall-0.2 does not build with recent nightly compilers. And you pulled in that crate because tempfile isn't minimal-versions correct. Could you please increase the version of redox-syscall in Cargo.lock.msrv ?

@asomers
Copy link
Member

asomers commented Nov 18, 2021

redox-syscall 0.2.9 should do it.

When testing with older versions of rustc, there's a CI failure mode
when our dependency, which used to be compatible with that Rust version,
publishes a new version which can't be compiled using that old Rust
anymore. That's pretty unfortunate, as that means that third parties can
break our CI without any changes to the code in this repo.

To protect against that, let's use Cargo.lock on CI, but only when
testing against older versions. For stable Rust, we continue to generate
Cargo.lock with freshest dependencies. Implementation wise, we save
known-good Cargo.lock as `Cargo.lock.msrv`, and just copy that in every
relevant CI job.

To get a working `Cargo.lock.msrv`, I run `cargo +nightly
generate-lockfile -Zminimal-versions` -- that is guaranteed to support
the oldest Rust version we can.
@matklad
Copy link
Contributor Author

matklad commented Nov 18, 2021

Yeah, lock.msrv makes stuff possible, it doesn't make it super-out-of-the-box, especially if the nightly is also involved.

This case is interesting. I'd say that for nightly we probably shouldn't use minver at all, as it seems easier and more useful to keep latest stuff compatible with the latest nightly. But redox seems to be a unique project which uses older nightly, which is a bummer. Given that our most MSRV-aggressive dependency, bitflags, is compatible with 1.46, I would guess that maybe treating this as a non-MSRV build would be lower maintenance overall? That is, I'd expect breakages due to the nightly compiler upgrade to be more of an issue than breakages due to package updates. That's wild guess though. More philosophically, maintaining MSRV for nightly seems like a bit too much to push downsstream.

Implemented the above plan of not using lock.msrv for redox in the latest commits, lets see if that passes CI.

@asomers
Copy link
Member

asomers commented Nov 19, 2021

Yeah, lock.msrv makes stuff possible, it doesn't make it super-out-of-the-box, especially if the nightly is also involved.

This case is interesting. I'd say that for nightly we probably shouldn't use minver at all, as it seems easier and more useful to keep latest stuff compatible with the latest nightly. But redox seems to be a unique project which uses older nightly, which is a bummer. Given that our most MSRV-aggressive dependency, bitflags, is compatible with 1.46, I would guess that maybe treating this as a non-MSRV build would be lower maintenance overall? That is, I'd expect breakages due to the nightly compiler upgrade to be more of an issue than breakages due to package updates. That's wild guess though. More philosophically, maintaining MSRV for nightly seems like a bit too much to push downsstream.

Implemented the above plan of not using lock.msrv for redox in the latest commits, lets see if that passes CI.

Not use minver on nightly? But -Zminimal-versions only works with nightly! Or is that not what you meant?

@matklad
Copy link
Contributor Author

matklad commented Nov 19, 2021

Not use minver on nightly? But -Zminimal-versions only works with nightly! Or is that not what you meant?

I mean "when testing with nightly compiler version, don't use Cargo.lock.msrv, and rely on default Cargo's behavior of picking maximal versions". That makes sense for nightly, as the situation is generally the opposite: code breaks with newer compiler versions more frequently than with older ones.

@matklad
Copy link
Contributor Author

matklad commented Nov 19, 2021

Ok, so the latest version passes the tests! We can keep it, or I can indeed revert to the previous approach and just bump redox-syscall, whatever you prefer more.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Ok, I can live with sub-par support for Redox. @rtzoeller are you happy with this PR?

Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

The approach seems reasonable to me. @asomers do we need to regenerate the Cargo.lock.msrv file before submitting?

@asomers
Copy link
Member

asomers commented Dec 11, 2021

I don't think so. If there's anything wrong with Cargo.lock.msrv, bors won't merge it.

bors r+

bors bot added a commit that referenced this pull request Dec 11, 2021
1593: Ensure that MSRV tests do not regress r=asomers a=matklad

When testing with older versions of rustc, there's a CI failure mode
when our dependency, which used to be compatible with that Rust version,
publishes a new version which can't be compiled using that old Rust
anymore. That's pretty unfortunate, as that means that third parties can
break our CI without any changes to the code in this repo.

To protect against that, let's use Cargo.lock on CI, but only when
testing against older versions. For stable Rust, we continue to generate
Cargo.lock with freshest dependencies. Implementation wise, we save
known-good Cargo.lock as `Cargo.lock.msrv`, and just copy that in every
relevant CI job.

To get a working `Cargo.lock.msrv`, I run `cargo +nightly
generate-lockfile -Zminimal-versions` -- that is guaranteed to support
the oldest Rust version we can.

Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 11, 2021

Build failed:

@asomers
Copy link
Member

asomers commented Dec 11, 2021

The test failure is due to Clippy getting pickier on the latest nightly . I'll fix it.

@rtzoeller
Copy link
Collaborator

bors retry

@bors bors bot merged commit 87142d2 into nix-rust:master Dec 12, 2021
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.

3 participants