From 366ae3f7bae7e84f6d0d2f9e31f19b0e19c84f4b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 8 Nov 2019 15:48:04 -0800 Subject: [PATCH 1/3] Turn the new lock file format on by default This commit enables the support added in #7070 by default. This means that gradually over time all `Cargo.lock` files will be migrated to the new format. Cargo shipped with Rust 1.38.0 supports this new lock file format, so any project using Rust 1.38.0 or above will converge quickly onto the new lock file format and continue working. The main benefit of the new format is to be more friendly to git merge conflicts. Information is deduplicated throughout the lock file to avoid verbose `depedencies` lists and the `checksum` data is all listed inline with `[[package]]`. This has been deployed with rust-lang/rust for some time now and it subjectively at least seems to have greatly reduced the amount of bouncing that happens for touching `Cargo.lock`. --- src/cargo/core/resolver/resolve.rs | 25 ++++++++- tests/testsuite/lockfile_compat.rs | 89 +++++++++++++++--------------- tests/testsuite/publish.rs | 2 +- tests/testsuite/resolve.rs | 2 +- 4 files changed, 68 insertions(+), 50 deletions(-) diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index dcb7cd44361..00d7eb5f0b1 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -15,7 +15,6 @@ use super::encode::Metadata; /// /// Each instance of `Resolve` also understands the full set of features used /// for each package. -#[derive(PartialEq)] pub struct Resolve { /// A graph, whose vertices are packages and edges are dependency specifications /// from `Cargo.toml`. We need a `Vec` here because the same package @@ -358,6 +357,26 @@ unable to verify that `{0}` is the same as when the lockfile was generated } } +impl PartialEq for Resolve { + fn eq(&self, other: &Resolve) -> bool { + macro_rules! compare { + ($($fields:ident)* | $($ignored:ident)*) => { + let Resolve { $($fields,)* $($ignored,)* } = self; + $(drop($ignored);)* + $($fields == &other.$fields)&&* + } + } + compare! { + // fields to compare + graph replacements reverse_replacements empty_features features + checksums metadata unused_patches public_dependencies + | + // fields to ignore + version + } + } +} + impl fmt::Debug for Resolve { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "graph: {:?}", self.graph)?; @@ -376,7 +395,7 @@ impl ResolveVersion { /// previous `Cargo.lock` files, and generally matches with what we want to /// encode. pub fn default() -> ResolveVersion { - ResolveVersion::V1 + ResolveVersion::V2 } /// Returns whether this encoding version is "from the future". @@ -385,7 +404,7 @@ impl ResolveVersion { /// intended to become the default "soon". pub fn from_the_future(&self) -> bool { match self { - ResolveVersion::V2 => true, + ResolveVersion::V2 => false, ResolveVersion::V1 => false, } } diff --git a/tests/testsuite/lockfile_compat.rs b/tests/testsuite/lockfile_compat.rs index abe4e4397f3..faf35e4356e 100644 --- a/tests/testsuite/lockfile_compat.rs +++ b/tests/testsuite/lockfile_compat.rs @@ -10,6 +10,14 @@ fn oldest_lockfile_still_works() { } } +fn assert_lockfiles_eq(expected: &str, actual: &str) { + for (l, r) in expected.lines().zip(actual.lines()) { + assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); + } + + assert_eq!(expected.lines().count(), actual.lines().count()); +} + fn oldest_lockfile_still_works_with_command(cargo_command: &str) { Package::new("bar", "0.1.0").publish(); @@ -19,16 +27,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 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "bar", ] - -[metadata] -"checksum bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "[..]" "#; let old_lockfile = r#" @@ -65,11 +71,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" p.cargo(cargo_command).run(); let lock = p.read_lockfile(); - for (l, r) in expected_lockfile.lines().zip(lock.lines()) { - assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); - } - - assert_eq!(lock.lines().count(), expected_lockfile.lines().count()); + assert_lockfiles_eq(expected_lockfile, &lock); } #[cargo_test] @@ -115,11 +117,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" p.cargo("build --locked").run(); let lock = p.read_lockfile(); - for (l, r) in old_lockfile.lines().zip(lock.lines()) { - assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); - } - - assert_eq!(lock.lines().count(), old_lockfile.lines().count()); + assert_lockfiles_eq(&old_lockfile, &lock); } #[cargo_test] @@ -166,26 +164,24 @@ source = "registry+https://github.com/rust-lang/crates.io-index" p.cargo("build").run(); let lock = p.read_lockfile(); - assert!(lock.starts_with( - r#" -# This file is automatically @generated by Cargo. + assert_lockfiles_eq( + 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" +checksum = "[..]" [[package]] name = "foo" version = "0.0.1" dependencies = [ - "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "bar", ] - -[metadata] -"# - .trim() - )); +"#, + &lock, + ); } #[cargo_test] @@ -413,22 +409,16 @@ fn current_lockfile_format() { 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 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)\", + \"bar\", ] - -[metadata] -\"checksum bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)\" = \"[..]\""; - - for (l, r) in expected.lines().zip(actual.lines()) { - assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); - } - - assert_eq!(actual.lines().count(), expected.lines().count()); +"; + assert_lockfiles_eq(expected, &actual); } #[cargo_test] @@ -447,7 +437,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" name = "foo" version = "0.0.1" dependencies = [ - "bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "bar", ] "#; @@ -472,7 +462,24 @@ dependencies = [ p.cargo("build").run(); let lock = p.read_lockfile(); - assert!(lock.starts_with(lockfile.trim())); + assert_lockfiles_eq( + r#"# [..] +# [..] +[[package]] +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", +] +"#, + &lock, + ); } #[cargo_test] @@ -549,11 +556,7 @@ dependencies = [ p.cargo("fetch").run(); let lock = p.read_lockfile(); - for (l, r) in lockfile.lines().zip(lock.lines()) { - assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); - } - - assert_eq!(lock.lines().count(), lockfile.lines().count()); + assert_lockfiles_eq(&lockfile, &lock); } #[cargo_test] @@ -624,9 +627,5 @@ dependencies = [ p.cargo("fetch").run(); let lock = p.read_lockfile(); - for (l, r) in lockfile.lines().zip(lock.lines()) { - assert!(lines_match(l, r), "Lines differ:\n{}\n\n{}", l, r); - } - - assert_eq!(lock.lines().count(), lockfile.lines().count()); + assert_lockfiles_eq(&lockfile, &lock); } diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index e94cabc07d6..545f2302cf9 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1171,7 +1171,7 @@ fn publish_git_with_version() { name = \"foo\"\n\ version = \"0.1.0\"\n\ dependencies = [\n\ - \x20\"dep1 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)\",\n\ + \x20\"dep1\",\n\ ]\n\ [..]", ), diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index f20008ecf0a..d40094946e8 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -30,5 +30,5 @@ fn minimal_version_cli() { let lock = p.read_lockfile(); - assert!(lock.contains("dep 1.0.0")); + assert!(!lock.contains("1.1.0")); } From 013c1afb9ac899f1cd0706bc61c998497e2b6d93 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 12 Nov 2019 07:19:27 -0800 Subject: [PATCH 2/3] Add support for new lockfile formats on new projects This commit adds support to Cargo and refactors the lockfile versioning slightly. The goal here is that Cargo continually has two thresholds of lockfile formats to create: * One is used for new lock files at all times * The other is used to update old lock files The logic for these two thresholds is appropriately updated throughout Cargo, and then this commit also preserves the previous update where new lock files will get the new format, but old lockfiles will continue to retain the old format by default. --- src/cargo/core/resolver/mod.rs | 2 +- src/cargo/core/resolver/resolve.rs | 77 +++++++++++++++++++----------- tests/testsuite/lockfile_compat.rs | 12 +++-- tests/testsuite/update.rs | 2 +- 4 files changed, 59 insertions(+), 34 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 5e659225831..85d107430d0 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -151,7 +151,7 @@ pub fn resolve( cksums, BTreeMap::new(), Vec::new(), - ResolveVersion::default(), + ResolveVersion::default_for_new_lockfiles(), ); check_cycles(&resolve)?; diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 00d7eb5f0b1..ffd169a88b3 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -1,4 +1,5 @@ use std::borrow::Borrow; +use std::cmp; use std::collections::{HashMap, HashSet}; use std::fmt; use std::iter::FromIterator; @@ -58,9 +59,12 @@ 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(PartialEq, Clone, Debug)] +#[derive(PartialEq, Eq, Clone, Copy, Debug, PartialOrd, Ord)] pub enum ResolveVersion { + // Historical baseline for when this abstraction was added. V1, + // Update around 2019 where `dependencies` arrays got compressed and + // checksums are listed inline. V2, } @@ -209,21 +213,35 @@ 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. We want to preserve those - // features if we find them. + // 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. // - // For this reason if the previous `Cargo.lock` is from the future, or - // otherwise it looks like it's produced with future features we - // understand, then the new resolve will be encoded with the same - // version. Note that new instances of `Resolve` always use the default - // encoding, and this is where we switch it to a future encoding if the - // future encoding isn't yet the default. - if previous.version.from_the_future() { - self.version = previous.version.clone(); - } + // 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(), + ); Ok(()) } @@ -389,23 +407,26 @@ impl fmt::Debug for Resolve { } impl ResolveVersion { - /// The default way to encode `Cargo.lock`. + /// The default way to encode new `Cargo.lock` files. /// - /// This is used for new `Cargo.lock` files that are generated without a - /// previous `Cargo.lock` files, and generally matches with what we want to - /// encode. - pub fn default() -> ResolveVersion { + /// 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 } - /// Returns whether this encoding version is "from the future". + /// 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. /// - /// This means that this encoding version is not currently the default but - /// intended to become the default "soon". - pub fn from_the_future(&self) -> bool { - match self { - ResolveVersion::V2 => false, - ResolveVersion::V1 => false, - } + /// 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 } } diff --git a/tests/testsuite/lockfile_compat.rs b/tests/testsuite/lockfile_compat.rs index faf35e4356e..2f0c34c4d30 100644 --- a/tests/testsuite/lockfile_compat.rs +++ b/tests/testsuite/lockfile_compat.rs @@ -27,14 +27,16 @@ 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#" @@ -171,14 +173,16 @@ 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, ); diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 1f7a2ed594b..93f27e34fff 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -572,7 +572,7 @@ fn preserve_top_comment() { let mut lines = lockfile.lines().collect::>(); lines.insert(2, "# some other comment"); let mut lockfile = lines.join("\n"); - lockfile.push_str("\n"); // .lines/.join loses the last newline + lockfile.push_str("\n\n"); // .lines/.join loses the last newline println!("saving Cargo.lock contents:\n{}", lockfile); p.change_file("Cargo.lock", &lockfile); From c37a46ced4d5f90e719cf603ed316367ddde2f20 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 18 Nov 2019 07:27:18 -0800 Subject: [PATCH 3/3] Handle a case where `cargo new` oscillates --- src/cargo/core/resolver/encode.rs | 26 +++++++++++++++++++++++++- src/cargo/ops/lockfile.rs | 4 ++-- tests/testsuite/new.rs | 11 +++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 04d9ebd38b4..5966b24111e 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -132,7 +132,7 @@ impl EncodableResolve { /// `features`. Care should be taken when using this Resolve. One of the /// primary uses is to be used with `resolve_with_previous` to guide the /// resolver to create a complete Resolve. - pub fn into_resolve(self, ws: &Workspace<'_>) -> CargoResult { + pub fn into_resolve(self, original: &str, ws: &Workspace<'_>) -> CargoResult { let path_deps = build_path_deps(ws); let mut checksums = HashMap::new(); @@ -333,6 +333,30 @@ impl EncodableResolve { unused_patches.push(id); } + // We have a curious issue where in the "v1 format" we buggily had a + // trailing blank line at the end of lock files under some specific + // conditions. + // + // Cargo is trying to write new lockfies in the "v2 format" but if you + // have no dependencies, for example, then the lockfile encoded won't + // really have any indicator that it's in the new format (no + // dependencies or checksums listed). This means that if you type `cargo + // new` followed by `cargo build` it will generate a "v2 format" lock + // file since none previously existed. When reading this on the next + // `cargo build`, however, it generates a new lock file because when + // reading in that lockfile we think it's the v1 format. + // + // To help fix this issue we special case here. If our lockfile only has + // one trailing newline, not two, *and* it only has one package, then + // this is actually the v2 format. + if original.ends_with("\n") + && !original.ends_with("\n\n") + && version == ResolveVersion::V1 + && g.iter().count() == 1 + { + version = ResolveVersion::V2; + } + Ok(Resolve::new( g, replacements, diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs index 4c7c03c1d18..db05e859014 100644 --- a/src/cargo/ops/lockfile.rs +++ b/src/cargo/ops/lockfile.rs @@ -22,7 +22,7 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult> { let resolve = (|| -> CargoResult> { let resolve: toml::Value = cargo_toml::parse(&s, f.path(), ws.config())?; let v: resolver::EncodableResolve = resolve.try_into()?; - Ok(Some(v.into_resolve(ws)?)) + Ok(Some(v.into_resolve(&s, ws)?)) })() .chain_err(|| format!("failed to parse lock file at: {}", f.path().display()))?; Ok(resolve) @@ -172,7 +172,7 @@ fn are_equal_lockfiles(mut orig: String, current: &str, ws: &Workspace<'_>) -> b let res: CargoResult = (|| { let old: resolver::EncodableResolve = toml::from_str(&orig)?; let new: resolver::EncodableResolve = toml::from_str(current)?; - Ok(old.into_resolve(ws)? == new.into_resolve(ws)?) + Ok(old.into_resolve(&orig, ws)? == new.into_resolve(current, ws)?) })(); if let Ok(true) = res { return true; diff --git a/tests/testsuite/new.rs b/tests/testsuite/new.rs index 345a2531034..49e059746e8 100644 --- a/tests/testsuite/new.rs +++ b/tests/testsuite/new.rs @@ -527,3 +527,14 @@ fn new_with_reference_link() { let contents = fs::read_to_string(paths::root().join("foo/Cargo.toml")).unwrap(); assert!(contents.contains("# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html")) } + +#[cargo_test] +fn lockfile_constant_during_new() { + cargo_process("new foo").env("USER", "foo").run(); + + cargo_process("build").cwd(&paths::root().join("foo")).run(); + let before = fs::read_to_string(paths::root().join("foo/Cargo.lock")).unwrap(); + cargo_process("build").cwd(&paths::root().join("foo")).run(); + let after = fs::read_to_string(paths::root().join("foo/Cargo.lock")).unwrap(); + assert_eq!(before, after); +}