-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: respect rust-version
when generating lockfile
#12861
Changes from all commits
10b8e7b
7b09193
92b82d6
fb95ac4
9ae485d
cd7cca3
b82910f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
use cargo_util_schemas::core::PartialVersion; | ||
use cargo_util_schemas::manifest::RustVersion; | ||
|
||
use super::encode::Metadata; | ||
use crate::core::dependency::DepKind; | ||
use crate::core::{Dependency, PackageId, PackageIdSpec, PackageIdSpecQuery, Summary, Target}; | ||
|
@@ -48,7 +51,7 @@ pub struct Resolve { | |
|
||
/// A version to indicate how a `Cargo.lock` should be serialized. | ||
/// | ||
/// When creating a new lockfile, the version with `#[default]` is used. | ||
/// When creating a new lockfile, the version in [`ResolveVersion::default`] is used. | ||
/// If an old version of lockfile already exists, it will stay as-is. | ||
/// | ||
/// It's important that if a new version is added that this is not updated | ||
|
@@ -64,25 +67,30 @@ pub struct Resolve { | |
/// | ||
/// It's theorized that we can add more here over time to track larger changes | ||
/// to the `Cargo.lock` format, but we've yet to see how that strategy pans out. | ||
#[derive(Default, PartialEq, Eq, Clone, Copy, Debug, PartialOrd, Ord)] | ||
#[derive(PartialEq, Eq, Clone, Copy, Debug, PartialOrd, Ord)] | ||
pub enum ResolveVersion { | ||
/// Historical baseline for when this abstraction was added. | ||
V1, | ||
/// A more compact format, more amenable to avoiding source-control merge | ||
/// conflicts. The `dependencies` arrays are compressed and checksums are | ||
/// listed inline. Introduced in 2019 in version 1.38. New lockfiles use | ||
/// V2 by default from 1.41 to 1.52. | ||
/// listed inline. | ||
/// | ||
/// * Introduced in 2019 in version 1.38. | ||
/// * New lockfiles use V2 by default from 1.41 to 1.52. | ||
V2, | ||
/// A format that explicitly lists a `version` at the top of the file as | ||
/// well as changing how git dependencies are encoded. Dependencies with | ||
/// `branch = "master"` are no longer encoded the same way as those without | ||
/// branch specifiers. Introduced in 2020 in version 1.47. New lockfiles use | ||
/// V3 by default staring in 1.53. | ||
#[default] | ||
/// branch specifiers. | ||
/// | ||
/// * Introduced in 2020 in version 1.47. | ||
/// * New lockfiles use V3 by default starting in 1.53. | ||
V3, | ||
/// SourceId URL serialization is aware of URL encoding. For example, | ||
/// `?branch=foo bar` is now encoded as `?branch=foo+bar` and can be decoded | ||
/// back and forth correctly. Introduced in 2024 in version 1.77. | ||
/// back and forth correctly. | ||
/// | ||
/// * Introduced in 2024 in version 1.78. | ||
V4, | ||
/// Unstable. Will collect a certain amount of changes and then go. | ||
/// | ||
|
@@ -91,6 +99,17 @@ pub enum ResolveVersion { | |
} | ||
|
||
impl ResolveVersion { | ||
/// Gets the default lockfile version. | ||
/// | ||
/// This is intended to be private. | ||
/// You shall use [`ResolveVersion::with_rust_version`] always. | ||
/// | ||
/// Update this and the description of enum variants of [`ResolveVersion`] | ||
/// when we're changing the default lockfile version. | ||
fn default() -> ResolveVersion { | ||
ResolveVersion::V3 | ||
} | ||
|
||
/// The maximum version of lockfile made into the stable channel. | ||
/// | ||
/// Any version larger than this needs `-Znext-lockfile-bump` to enable. | ||
|
@@ -99,6 +118,40 @@ impl ResolveVersion { | |
pub fn max_stable() -> ResolveVersion { | ||
ResolveVersion::V4 | ||
} | ||
|
||
/// Gets the default lockfile version for the given Rust version. | ||
pub fn with_rust_version(rust_version: Option<&RustVersion>) -> Self { | ||
let Some(rust_version) = rust_version else { | ||
return ResolveVersion::default(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A even terrible idea: we rename it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed this in an awkward way but easier to refer to: 991663c. Let me know if there are other strategies to communicate this. |
||
}; | ||
|
||
let rust_1_41 = PartialVersion { | ||
major: 1, | ||
minor: Some(41), | ||
patch: None, | ||
pre: None, | ||
build: None, | ||
} | ||
.try_into() | ||
.expect("PartialVersion 1.41"); | ||
let rust_1_53 = PartialVersion { | ||
major: 1, | ||
minor: Some(53), | ||
patch: None, | ||
pre: None, | ||
build: None, | ||
} | ||
.try_into() | ||
.expect("PartialVersion 1.53"); | ||
|
||
if rust_version >= &rust_1_53 { | ||
ResolveVersion::V3 | ||
} else if rust_version >= &rust_1_41 { | ||
ResolveVersion::V2 | ||
} else { | ||
ResolveVersion::V1 | ||
} | ||
} | ||
} | ||
|
||
impl Resolve { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,9 +64,10 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes | |
// out lock file updates as they're otherwise already updated, and changes | ||
// which don't touch dependencies won't seemingly spuriously update the lock | ||
// file. | ||
let default_version = ResolveVersion::default(); | ||
let default_version = ResolveVersion::with_rust_version(ws.rust_version()); | ||
let current_version = resolve.version(); | ||
let next_lockfile_bump = ws.config().cli_unstable().next_lockfile_bump; | ||
tracing::debug!("lockfile - current: {current_version:?}, default: {default_version:?}"); | ||
Comment on lines
+67
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the callers of this have the rust version. Should we use that? The difference is whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean the caller of |
||
|
||
if current_version < default_version { | ||
resolve.set_version(default_version); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put this behind
-Zmsrv-policy
or insta-stable?I'm fine with insta-stable. The main policy questions would be around
--ignore-rust-version
, where presentrust-version
, so this is determined by finding the minimumpackage.rust-version
among all workspace membersThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't feel like it's a thing
-Zmsrv-policy
wants to address. If we had got MSRV-aware resolver when we started versioning lockfile, I believe we would have added this.Regarding
--ignore-rust-version
, I am pretty sold on respecting that, given the semantic is like "hey I want Cargo to work as if I never setrust-version
".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into the current codebase, I am now more unsure about supporting
--ignore-rust-version
for generating lockfile. If we aim to do that, we may need to expose--ignore-rust-version
to every command that generates lockfiles.cargo build
and other commands involving buildcargo fix
cargo generate-lockfile
cargo tree
cargo metadata
cargo update
It's a cognitive overload for users to learn they have the control over lockfile versions for all these commands. The user experience would be better if we just provide it out-of-the-box. Lockfile versions are somehow opaque to users to me.
I cannot think of any situation people may opt-in
--ignore-rust-version
for controlling the lockfile version. It's incompatible when you want to support old versions but requesting for a new lockfile version. For people want old lockfiles, unless there are bugs in newer versions, otherwise we don't want people to continue using old versions. And they are alwasy able to setrust-version
for older lockfiles.Moreover, the workaround is always there — manually changing the
version
field in the lockfile.Is there any use case I am not aware of for respecting
--ignore-rust-version
for lockfile generating?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could start respecting
rust-version
from v4, sorust-version
older than1.75
won't affect lockfile versions. This would be less controversial.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reminding me of an idea I had for MSRV-aware resolver of putting the MSRV in the lockfile and making it sticky. Having to pass flags along for every call can be pretty frustrating.