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/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 dcb7cd44361..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; @@ -15,7 +16,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 @@ -59,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, } @@ -210,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(()) } @@ -358,6 +375,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)?; @@ -370,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 { - ResolveVersion::V1 + /// 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 => true, - 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/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/lockfile_compat.rs b/tests/testsuite/lockfile_compat.rs index abe4e4397f3..2f0c34c4d30 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(); @@ -24,11 +32,11 @@ 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 [..]", ] [metadata] -"checksum bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "[..]" +"[..]" = "[..]" "#; let old_lockfile = r#" @@ -65,11 +73,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 +119,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,9 +166,8 @@ 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" @@ -179,13 +178,14 @@ 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 [..]", ] [metadata] -"# - .trim() - )); +"[..]" = "[..]" +"#, + &lock, + ); } #[cargo_test] @@ -413,22 +413,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 +441,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 +466,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 +560,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 +631,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/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); +} 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")); } 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);