Turn the new lock file format on by default

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