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

Vendor the bitflags crate #1560

Closed
wants to merge 4 commits into from
Closed

Conversation

asomers
Copy link
Member

@asomers asomers commented Oct 15, 2021

Bitflags has a policy of changing the MSRV in minor releases. This is a
breaking change, which semver does not allow in minor releases. Nix
adjusts by locking bitflags to a specific minor version. But Cargo does
not allow multiple semver-compatible versions of a crate to be included
in the same build, with the result that crates which use both Nix 0.23
and also use bitflags 1.2 cannot compile.

Fix the problem by vendoring the bitflags crate, removing the external
dependency completely.

Fixes #1555

Bitflags has a policy of changing the MSRV in minor releases.  This is a
breaking change, which semver does not allow in minor releases.  Nix
adjusts by locking bitflags to a specific minor version.  But Cargo does
not allow multiple semver-compatible versions of a crate to be included
in the same build, with the result that crates which use both Nix 0.23
and also use bitflags 1.2 cannot compile.

Fix the problem by vendoring the bitflags crate, removing the external
dependency completely.
The vendor/ directory will henceforth contain virginal code copied from
upstream, and any necessary local modifications will only be made to
src/bitflags.rs.

Fixes nix-rust#1555
@asomers asomers added this to the 0.23.1 milestone Oct 16, 2021
@matklad
Copy link
Contributor

matklad commented Oct 23, 2021

Bitflags has a policy of changing the MSRV in minor releases. This is a
breaking change, which semver does not allow in minor releases.

JFYI, there isn’t agreement about this, a lot of maintainers think differently. See https://old.reddit.com/r/rust/comments/qcy2w2/psa_rust_cargo_handles_minimum_supported_rust/hhlgzva/ for the most recent debate.

if you are already familiar with the other side debate, sorry for the noise.

If not, I can lay down arguments for why MSRV bump should not be considered breaking and, consequently, why vendoring bitflags is not required. My personal strength and confidence of opinion are both about 0.9 here, but there’s no denying the fact that many people disagree :)

@asomers
Copy link
Member Author

asomers commented Oct 23, 2021

@matklad I'm aware of the arguments against it. They all boil down to "everybody should be using Rust stable", because if a crate raises its MSRV willy-nilly, then it's impossible for any downstream crate to support the MSRV it promises. That's the "S" in "MSRV". The new change in Cargo 1.56.0 should hopefully remedy this situation; see Nix PR #1561 . But only if the "Influencing version resolution" described in https://github.com/rust-lang/rfcs/blob/master/text/2495-min-rust-version.md#influencing-version-resolution gets implemented, which so far it hasn't.

@matklad
Copy link
Contributor

matklad commented Oct 23, 2021

Ok! I guess I see where you are coming from, but I also don't agree with conclusions. I am going to spell out my reasoning just in case, but it's fine to conclude that I am just wrong :)

I don't think "everybody should be using Rust stable" is what is happening in practice. once_cell, lazy_static, regex, rayon are all past 1.0 crates which don't consider MSRV bump breaking, and yet they promise to support Rust versions older than currently supported by nix.

I also don't think that dependencies bumping MSRV immediately make it impossible to uphold your own MSRV promise. Let's say that we develop a package which promises compatibility with Rust 1.45 and we have bitflags = "1.2" in Cargo.toml. If now bitflags releases 1.3, which requires 1.46, it doesn't immediately break you crate's MSRV promise. Downstream projects with Cargo.lock can still compile with 1.45, provided that they have bitfalgs 1.2 in their lockfile. Downstream projects which use 1.46 can update their lockfiles to bitflags 1.3. To break your own MSRV, you need to upgrade deperency in Cargo.toml to bitflags = "1.3". In other words, MSRV for libraries should mean "there exist a set of dependencies which works with this Rust version", not "every set of dependencies works with Rust version".

One practical problem here is that Cargo indeed doesn't know about minimal versions during resolution yet, so bitflags update can break your CI for MSRV. The solution to that is to pin Cargo.lock during MSRV checking on CI. Couple examples here are rayon's and onece_cell's CIs.

@asomers
Copy link
Member Author

asomers commented Nov 16, 2021

@matklad that's an interesting idea. It's the first real alternative to "everybody should run stable" that I've heard. And it until Cargo gets an msrv-aware resolver, it looks like it could work. Would you be willing to submit a PR to set it up? The biggest headache, I think, will be maintaining that file. Are you aware of any tools that can help?

@matklad
Copy link
Contributor

matklad commented Nov 16, 2021

Yeah, let's try this: #1592 (note: that's intentionally not based on the current master, to verify that the trick could have prevented bitflags breaking the CI).

Are you aware of any tools that can help?

cargo +nightly generate-lockfile -Zminimal-versions gets the job done, provided that you and deps are minimal-version correct. If that' not the case, I just manually downgrade the offending packages with cargo update -p offending-package --precise an.older.version.

@asomers
Copy link
Member Author

asomers commented Nov 17, 2021

Sadly, -Zminimal-versions doesn't work for most complex crates, because it requires all of your deps to be minimal-version correct, too. There's been talk about fixing that by making it downgrade all immediate deps to the minimal versions, but use the latest version for transitive deps, but I don't think anybody has implemented that yet.

@asomers
Copy link
Member Author

asomers commented Dec 15, 2021

Closing in favor of #1593

@asomers asomers closed this Dec 15, 2021
matklad referenced this pull request in dalek-cryptography/curve25519-dalek Feb 2, 2022
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.

Cannot have nix 0.20 and 0.23 in same package due to conflicting bitflags requirement
2 participants