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

feat: respect rust-version when generating lockfile #12861

Merged
merged 7 commits into from
Feb 19, 2024
4 changes: 0 additions & 4 deletions benches/benchsuite/benches/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ fn do_resolve<'cfg>(config: &'cfg Config, ws_root: &Path) -> ResolveInfo<'cfg> {
let specs = pkgs.to_package_id_specs(&ws).unwrap();
let has_dev_units = HasDevUnits::Yes;
let force_all_targets = ForceAllTargets::No;
let max_rust_version = None;
// Do an initial run to download anything necessary so that it does
// not confuse criterion's warmup.
let ws_resolve = cargo::ops::resolve_ws_with_opts(
Expand All @@ -42,7 +41,6 @@ fn do_resolve<'cfg>(config: &'cfg Config, ws_root: &Path) -> ResolveInfo<'cfg> {
&specs,
has_dev_units,
force_all_targets,
max_rust_version,
)
.unwrap();
ResolveInfo {
Expand Down Expand Up @@ -84,7 +82,6 @@ fn resolve_ws(c: &mut Criterion) {
force_all_targets,
..
} = lazy_info.get_or_insert_with(|| do_resolve(&config, &ws_root));
let max_rust_version = None;
b.iter(|| {
cargo::ops::resolve_ws_with_opts(
ws,
Expand All @@ -94,7 +91,6 @@ fn resolve_ws(c: &mut Criterion) {
specs,
*has_dev_units,
*force_all_targets,
max_rust_version,
)
.unwrap();
})
Expand Down
8 changes: 8 additions & 0 deletions crates/cargo-util-schemas/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1417,6 +1417,14 @@ impl std::str::FromStr for RustVersion {
fn from_str(value: &str) -> Result<Self, Self::Err> {
let partial = value.parse::<PartialVersion>();
let partial = partial.map_err(RustVersionErrorKind::PartialVersion)?;
partial.try_into()
}
}

impl TryFrom<PartialVersion> for RustVersion {
type Error = RustVersionError;

fn try_from(partial: PartialVersion) -> Result<Self, Self::Error> {
if partial.pre.is_some() {
return Err(RustVersionErrorKind::Prerelease.into());
}
Expand Down
2 changes: 2 additions & 0 deletions crates/resolver-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::time::Instant;
use cargo::core::dependency::DepKind;
use cargo::core::resolver::{self, ResolveOpts, VersionOrdering, VersionPreferences};
use cargo::core::Resolve;
use cargo::core::ResolveVersion;
use cargo::core::{Dependency, PackageId, Registry, Summary};
use cargo::core::{GitReference, SourceId};
use cargo::sources::source::QueryKind;
Expand Down Expand Up @@ -174,6 +175,7 @@ pub fn resolve_with_config_raw(
&[],
&mut registry,
&version_prefs,
ResolveVersion::with_rust_version(None),
Some(config),
);

Expand Down
2 changes: 0 additions & 2 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ pub fn resolve_std<'cfg>(
let cli_features = CliFeatures::from_command_line(
&features, /*all_features*/ false, /*uses_default_features*/ false,
)?;
let max_rust_version = ws.rust_version();
let resolve = ops::resolve_ws_with_opts(
&std_ws,
target_data,
Expand All @@ -154,7 +153,6 @@ pub fn resolve_std<'cfg>(
&specs,
HasDevUnits::No,
crate::core::resolver::features::ForceAllTargets::No,
max_rust_version,
)?;
Ok((
resolve.pkg_set,
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,16 @@ mod version_prefs;
/// * `version_prefs` - this represents a preference for some versions over others,
/// based on the lock file or other reasons such as `[patch]`es.
///
/// * `resolve_version` - this controls how the lockfile will be serialized.
///
/// * `config` - a location to print warnings and such, or `None` if no warnings
/// should be printed
pub fn resolve(
summaries: &[(Summary, ResolveOpts)],
replacements: &[(PackageIdSpec, Dependency)],
registry: &mut dyn Registry,
version_prefs: &VersionPreferences,
resolve_version: ResolveVersion,
config: Option<&Config>,
) -> CargoResult<Resolve> {
let _p = profile::start("resolving");
Expand Down Expand Up @@ -169,7 +172,7 @@ pub fn resolve(
cksums,
BTreeMap::new(),
Vec::new(),
ResolveVersion::default(),
resolve_version,
summaries,
);

Expand Down
69 changes: 61 additions & 8 deletions src/cargo/core/resolver/resolve.rs
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};
Expand Down Expand Up @@ -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
Expand All @@ -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.
///
Expand All @@ -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.
Expand All @@ -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 {
Copy link
Contributor

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

  • Whether this should respect --ignore-rust-version, where present
  • A workspace has no rust-version, so this is determined by finding the minimum package.rust-version among all workspace members

Copy link
Member Author

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 set rust-version".

Copy link
Contributor

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.

Copy link
Member Author

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 build
  • cargo 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 set rust-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?

Copy link
Member Author

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, so rust-version older than 1.75 won't affect lockfile versions. This would be less controversial.

Copy link
Contributor

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.

let Some(rust_version) = rust_version else {
return ResolveVersion::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove Default impl to help force people to use with_rust_version?

Copy link
Member Author

Choose a reason for hiding this comment

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

A even terrible idea: we rename it to fn default(…) so people never misuse 🙈.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 {
Expand Down
2 changes: 0 additions & 2 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,6 @@ pub fn create_bcx<'a, 'cfg>(
HasDevUnits::No
}
};
let max_rust_version = ws.rust_version();
let resolve = ops::resolve_ws_with_opts(
ws,
&mut target_data,
Expand All @@ -271,7 +270,6 @@ pub fn create_bcx<'a, 'cfg>(
&specs,
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
max_rust_version,
)?;
let WorkspaceResolve {
mut pkg_set,
Expand Down
6 changes: 0 additions & 6 deletions src/cargo/ops/cargo_generate_lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ pub struct UpdateOptions<'a> {

pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> {
let mut registry = PackageRegistry::new(ws.config())?;
let max_rust_version = ws.rust_version();
let mut resolve = ops::resolve_with_previous(
&mut registry,
ws,
Expand All @@ -36,7 +35,6 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> {
None,
&[],
true,
max_rust_version,
)?;
ops::write_pkg_lockfile(ws, &mut resolve)?;
Ok(())
Expand All @@ -57,8 +55,6 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
.config()
.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;

let max_rust_version = ws.rust_version();

let previous_resolve = match ops::load_pkg_lockfile(ws)? {
Some(resolve) => resolve,
None => {
Expand All @@ -78,7 +74,6 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
None,
&[],
true,
max_rust_version,
)?
}
}
Expand Down Expand Up @@ -157,7 +152,6 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
Some(&to_avoid),
&[],
true,
max_rust_version,
)?;

// Summarize what is changing for the user.
Expand Down
3 changes: 0 additions & 3 deletions src/cargo/ops/cargo_output_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,6 @@ fn build_resolve_graph(
crate::core::resolver::features::ForceAllTargets::No
};

let max_rust_version = ws.rust_version();

// Note that even with --filter-platform we end up downloading host dependencies as well,
// as that is the behavior of download_accessible.
let ws_resolve = ops::resolve_ws_with_opts(
Expand All @@ -152,7 +150,6 @@ fn build_resolve_graph(
&specs,
HasDevUnits::Yes,
force_all,
max_rust_version,
)?;

let package_map: BTreeMap<PackageId, Package> = ws_resolve
Expand Down
3 changes: 0 additions & 3 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,6 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
to_real_manifest(toml_manifest, false, source_id, package_root, config)?;
let new_pkg = Package::new(manifest, orig_pkg.manifest_path());

let max_rust_version = new_pkg.rust_version().cloned();

// Regenerate Cargo.lock using the old one as a guide.
let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?;
let mut tmp_reg = PackageRegistry::new(ws.config())?;
Expand All @@ -475,7 +473,6 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
None,
&[],
true,
max_rust_version.as_ref(),
)?;
let pkg_set = ops::get_resolved_packages(&new_resolve, tmp_reg)?;

Expand Down
3 changes: 0 additions & 3 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,6 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<(
let mut target_data =
RustcTargetData::new(ws, &opts.compile_opts.build_config.requested_kinds)?;
let mut resolve_differences = |has_dev_units| -> CargoResult<(WorkspaceResolve<'_>, DiffMap)> {
let max_rust_version = ws.rust_version();

let ws_resolve = ops::resolve_ws_with_opts(
ws,
&mut target_data,
Expand All @@ -255,7 +253,6 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<(
&specs,
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
max_rust_version,
)?;

let feature_opts = FeatureOpts::new_behavior(ResolveBehavior::V2, has_dev_units);
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/ops/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 --ignore-rust-version was used or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean the caller of write_pkg_lockfile? Yep we probably need to take care of that route.


if current_version < default_version {
resolve.set_version(default_version);
Expand Down
Loading