Auto merge of #7579 - alexcrichton:lockfile-fmt, r=ehuss

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/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<Resolve> {
+    pub fn into_resolve(self, original: &str, ws: &Workspace<'_>) -> CargoResult<Resolve> {
         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,

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)?;

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
     }
 }

src/cargo/ops/lockfile.rs

@@ -22,7 +22,7 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {
     let resolve = (|| -> CargoResult<Option<Resolve>> {
         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<bool> = (|| {
             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;

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);
 }

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);
+}