From fdbe9f8e053eee16c292d865bce745f6da77deba Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 28 Jul 2020 12:17:55 -0700 Subject: [PATCH] Update lock file encodings on changes This commit alters Cargo's lockfile encoding update strategy from its previous incarnation. Previously Cargo had two versions, one for new lock files and one for old lock files. Each of these versions were different and would affect how Cargo manages lock file updates. The intention was that we'd roll out defaults to new lock files first and then later to preexisting lock files. This requires two separate changes, though, and it's not necessarily clear when to start updating old lock files. Additionally when old lock files were opted in it would break builds using `--locked` if they simply updated Cargo because Cargo would would want to bring the lock file versions forward. The purpose of this change is to solve these issues. The new strategy for updating a lock file's encoding is to simply preserve what's already existing on the filesystem until we actually decide to write out a new lock file. When Cargo updates a lock file on-disk then it will, at that time, update the lock file encoding to whatever the current default is. This means that there's only one version number to keep track of (the default for encoding). Cargo will always preserve the preexisting encoding unless another change is required to the lock file. --- src/cargo/core/resolver/encode.rs | 33 ++++++++--- src/cargo/core/resolver/mod.rs | 2 +- src/cargo/core/resolver/resolve.rs | 66 ++++++---------------- src/cargo/ops/cargo_generate_lockfile.rs | 8 +-- src/cargo/ops/cargo_package.rs | 4 +- src/cargo/ops/lockfile.rs | 32 ++++++++--- src/cargo/ops/resolve.rs | 4 +- tests/testsuite/lockfile_compat.rs | 70 +++++++++++++++++++----- 8 files changed, 129 insertions(+), 90 deletions(-) diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index ce3ade40b19..d2414801ed9 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -19,17 +19,32 @@ //! We do, however, want to change `Cargo.lock` over time. (and we have!). To do //! this the rules that we currently have are: //! -//! * Add support for the new format to Cargo -//! * Continue to, by default, generate the old format -//! * Preserve the new format if found -//! * Wait a "long time" (e.g. 6 months or so) -//! * Change Cargo to by default emit the new format +//! * Add support for the new format to Cargo. This involves code changes in +//! Cargo itself, likely by adding a new variant of `ResolveVersion` and +//! branching on that where necessary. This is accompanied with tests in the +//! `lockfile_compat` module. +//! +//! * Do not update `ResolveVersion::default()`. The new lockfile format will +//! not be used yet. +//! +//! * Preserve the new format if found. This means that if Cargo finds the new +//! version it'll keep using it, but otherwise it continues to use whatever +//! format it previously found. +//! +//! * Wait a "long time". This is at least until the changes here hit stable +//! Rust. Often though we wait a little longer to let the changes percolate +//! into one or two older stable releases. +//! +//! * Change the return value of `ResolveVersion::default()` to the new format. +//! This will cause new lock files to use the latest encoding as well as +//! causing any operation which updates the lock file to update to the new +//! format. //! //! This migration scheme in general means that Cargo we'll get *support* for a -//! new format into Cargo ASAP, but it won't really be exercised yet (except in -//! Cargo's own tests really). Eventually when stable/beta/nightly all have -//! support for the new format (and maybe a few previous stable versions) we -//! flip the switch. Projects on nightly will quickly start seeing changes, but +//! new format into Cargo ASAP, but it won't be exercised yet (except in Cargo's +//! own tests). Eventually when stable/beta/nightly all have support for the new +//! format (and maybe a few previous stable versions) we flip the switch. +//! Projects on nightly will quickly start seeing changes, but //! stable/beta/nightly will all understand this new format and will preserve //! it. //! diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index dbc4fe8be8d..094c64065b1 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -161,7 +161,7 @@ pub fn resolve( cksums, BTreeMap::new(), Vec::new(), - ResolveVersion::default_for_new_lockfiles(), + ResolveVersion::default(), summaries, ); diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index c9903d0503e..9345aaf9e55 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -5,7 +5,6 @@ use crate::util::errors::CargoResult; use crate::util::interning::InternedString; use crate::util::Graph; use std::borrow::Borrow; -use std::cmp; use std::collections::{HashMap, HashSet}; use std::fmt; @@ -219,35 +218,8 @@ unable to verify that `{0}` is the same as when the lockfile was generated // Be sure to just copy over any unknown metadata. self.metadata = previous.metadata.clone(); - // The goal of Cargo is largely to preserve the encoding of `Cargo.lock` - // that it finds on the filesystem. Sometimes `Cargo.lock` changes are - // in the works where they haven't been set as the default yet but will - // become the default soon. - // - // The scenarios we could be in are: - // - // * This is a brand new lock file with nothing previous. In that case - // this method isn't actually called at all, but instead - // `default_for_new_lockfiles` called below was encoded during the - // resolution step, so that's what we're gonna use. - // - // * We have an old lock file. In this case we want to switch the - // version to `default_for_old_lockfiles`. That puts us in one of - // three cases: - // - // * Our version is older than the default. This means that we're - // migrating someone forward, so we switch the encoding. - // * Our version is equal to the default, nothing to do! - // * Our version is *newer* than the default. This is where we - // critically keep the new version of encoding. - // - // This strategy should get new lockfiles into the pipeline more quickly - // while ensuring that any time an old cargo sees a future lock file it - // keeps the future lockfile encoding. - self.version = cmp::max( - previous.version, - ResolveVersion::default_for_old_lockfiles(), - ); + // Preserve the lockfile encoding where possible to avoid lockfile churn + self.version = previous.version; Ok(()) } @@ -383,6 +355,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated self.version } + pub fn set_version(&mut self, version: ResolveVersion) { + self.version = version; + } + pub fn summary(&self, pkg_id: PackageId) -> &Summary { &self.summaries[&pkg_id] } @@ -418,27 +394,21 @@ impl fmt::Debug for Resolve { } } -impl ResolveVersion { - /// The default way to encode new `Cargo.lock` files. +impl Default for ResolveVersion { + /// The default way to encode new or updated `Cargo.lock` files. /// /// It's important that if a new version of `ResolveVersion` is added that /// this is not updated until *at least* the support for the version is in - /// the stable release of Rust. It's ok for this to be newer than - /// `default_for_old_lockfiles` below. - pub fn default_for_new_lockfiles() -> ResolveVersion { - ResolveVersion::V2 - } - - /// The default way to encode old preexisting `Cargo.lock` files. This is - /// often trailing the new lockfiles one above to give older projects a - /// longer time to catch up. + /// the stable release of Rust. /// - /// It's important that this trails behind `default_for_new_lockfiles` for - /// quite some time. This gives projects a quite large window to update in - /// where we don't force updates, so if projects span many versions of Cargo - /// all those versions of Cargo will have support for a new version of the - /// lock file. - pub fn default_for_old_lockfiles() -> ResolveVersion { - ResolveVersion::V1 + /// This resolve version will be used for all new lock files, for example + /// those generated by `cargo update` (update everything) or building after + /// a `cargo new` (where no lock file previously existed). This is also used + /// for *updated* lock files such as when a dependency is added or when a + /// version requirement changes. In this situation Cargo's updating the lock + /// file anyway so it takes the opportunity to bump the lock file version + /// forward. + fn default() -> ResolveVersion { + ResolveVersion::V2 } } diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index feccede48be..a86b34e3ae0 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -21,7 +21,7 @@ pub struct UpdateOptions<'a> { pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> { let mut registry = PackageRegistry::new(ws.config())?; - let resolve = ops::resolve_with_previous( + let mut resolve = ops::resolve_with_previous( &mut registry, ws, &ResolveOpts::everything(), @@ -30,7 +30,7 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> { &[], true, )?; - ops::write_pkg_lockfile(ws, &resolve)?; + ops::write_pkg_lockfile(ws, &mut resolve)?; Ok(()) } @@ -113,7 +113,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes registry.add_sources(sources)?; } - let resolve = ops::resolve_with_previous( + let mut resolve = ops::resolve_with_previous( &mut registry, ws, &ResolveOpts::everything(), @@ -153,7 +153,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes .shell() .warn("not updating lockfile due to dry run")?; } else { - ops::write_pkg_lockfile(ws, &resolve)?; + ops::write_pkg_lockfile(ws, &mut resolve)?; } return Ok(()); diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index fc98f5de66a..5dc077c4b4d 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -284,14 +284,14 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult { // Regenerate Cargo.lock using the old one as a guide. let tmp_ws = Workspace::ephemeral(new_pkg, ws.config(), None, true)?; - let (pkg_set, new_resolve) = ops::resolve_ws(&tmp_ws)?; + let (pkg_set, mut new_resolve) = ops::resolve_ws(&tmp_ws)?; if let Some(orig_resolve) = orig_resolve { compare_resolve(config, tmp_ws.current()?, &orig_resolve, &new_resolve)?; } check_yanked(config, &pkg_set, &new_resolve)?; - ops::resolve_to_string(&tmp_ws, &new_resolve) + ops::resolve_to_string(&tmp_ws, &mut new_resolve) } // Checks that the package has some piece of metadata that a human can diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs index 05ffc0bdaa7..9ed0c76d27b 100644 --- a/src/cargo/ops/lockfile.rs +++ b/src/cargo/ops/lockfile.rs @@ -27,17 +27,17 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult> { } /// Generate a toml String of Cargo.lock from a Resolve. -pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult { +pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoResult { let (_orig, out, _ws_root) = resolve_to_string_orig(ws, resolve)?; Ok(out) } -pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult<()> { - let (orig, out, ws_root) = resolve_to_string_orig(ws, resolve)?; +pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoResult<()> { + let (orig, mut out, ws_root) = resolve_to_string_orig(ws, resolve)?; // If the lock file contents haven't changed so don't rewrite it. This is // helpful on read-only filesystems. - if let Some(orig) = orig { + if let Some(orig) = &orig { if are_equal_lockfiles(orig, &out, ws) { return Ok(()); } @@ -62,6 +62,16 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult< ); } + // While we're updating the lock file anyway go ahead and update its + // encoding to whatever the latest default is. That way we can slowly roll + // 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. + if resolve.version() < ResolveVersion::default() { + resolve.set_version(ResolveVersion::default()); + out = serialize_resolve(resolve, orig.as_deref()); + } + // Ok, if that didn't work just write it out ws_root .open_rw("Cargo.lock", ws.config(), "Cargo.lock file") @@ -76,7 +86,7 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult< fn resolve_to_string_orig( ws: &Workspace<'_>, - resolve: &Resolve, + resolve: &mut Resolve, ) -> CargoResult<(Option, String, Filesystem)> { // Load the original lock file if it exists. let ws_root = Filesystem::new(ws.root().to_path_buf()); @@ -86,7 +96,11 @@ fn resolve_to_string_orig( f.read_to_string(&mut s)?; Ok(s) }); + let out = serialize_resolve(resolve, orig.as_ref().ok().map(|s| &**s)); + Ok((orig.ok(), out, ws_root)) +} +fn serialize_resolve(resolve: &Resolve, orig: Option<&str>) -> String { let toml = toml::Value::try_from(resolve).unwrap(); let mut out = String::new(); @@ -100,7 +114,7 @@ fn resolve_to_string_orig( out.push_str(extra_line); out.push('\n'); // and preserve any other top comments - if let Ok(orig) = &orig { + if let Some(orig) = orig { let mut comments = orig.lines().take_while(|line| line.starts_with('#')); if let Some(first) = comments.next() { if first != marker_line { @@ -156,11 +170,11 @@ fn resolve_to_string_orig( out.pop(); } } - - Ok((orig.ok(), out, ws_root)) + out } -fn are_equal_lockfiles(mut orig: String, current: &str, ws: &Workspace<'_>) -> bool { +fn are_equal_lockfiles(orig: &str, current: &str, ws: &Workspace<'_>) -> bool { + let mut orig = orig.to_string(); if has_crlf_line_endings(&orig) { orig = orig.replace("\r\n", "\n"); } diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 7d6065374c9..05b3f5961e3 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -165,7 +165,7 @@ fn resolve_with_registry<'cfg>( registry: &mut PackageRegistry<'cfg>, ) -> CargoResult { let prev = ops::load_pkg_lockfile(ws)?; - let resolve = resolve_with_previous( + let mut resolve = resolve_with_previous( registry, ws, &ResolveOpts::everything(), @@ -176,7 +176,7 @@ fn resolve_with_registry<'cfg>( )?; if !ws.is_ephemeral() && ws.require_optional_deps() { - ops::write_pkg_lockfile(ws, &resolve)?; + ops::write_pkg_lockfile(ws, &mut resolve)?; } Ok(resolve) } diff --git a/tests/testsuite/lockfile_compat.rs b/tests/testsuite/lockfile_compat.rs index 7dec021058f..dbda8e0466a 100644 --- a/tests/testsuite/lockfile_compat.rs +++ b/tests/testsuite/lockfile_compat.rs @@ -29,16 +29,14 @@ fn oldest_lockfile_still_works_with_command(cargo_command: &str) { name = "bar" version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "[..]" [[package]] name = "foo" version = "0.0.1" dependencies = [ - "bar [..]", + "bar", ] - -[metadata] -"[..]" = "[..]" "#; let old_lockfile = r#" @@ -132,14 +130,14 @@ fn totally_wild_checksums_works() { .file( "Cargo.toml", r#" - [project] - name = "foo" - version = "0.0.1" - authors = [] + [project] + name = "foo" + version = "0.0.1" + authors = [] - [dependencies] - bar = "0.1.0" - "#, + [dependencies] + bar = "0.1.0" + "#, ) .file("src/lib.rs", "") .file( @@ -175,16 +173,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index" name = "bar" version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "[..]" [[package]] name = "foo" version = "0.0.1" dependencies = [ - "bar [..]", + "bar", ] - -[metadata] -"[..]" = "[..]" "#, &lock, ); @@ -722,3 +718,47 @@ Caused by: .with_status(101) .run(); } + +#[cargo_test] +fn preserve_old_format_if_no_update_needed() { + let cksum = Package::new("bar", "0.1.0").publish(); + let lockfile = format!( + r#"# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "bar" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + +[[package]] +name = "foo" +version = "0.0.1" +dependencies = [ + "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[metadata] +"checksum bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "{}" +"#, + cksum + ); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "0.1.0" + "#, + ) + .file("src/lib.rs", "") + .file("Cargo.lock", &lockfile) + .build(); + + p.cargo("build --locked").run(); +}