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

@@ -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.
 //!

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

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 {
+    /// the stable release of Rust.
+    ///
+    /// 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
     }
-
-    /// 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.
-    ///
-    /// 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/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(());
 

src/cargo/ops/cargo_package.rs

@@ -284,14 +284,14 @@ fn build_lock(ws: &Workspace<'_>) -> CargoResult<String> {
 
     // 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

src/cargo/ops/lockfile.rs

@@ -27,17 +27,17 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {
 }
 
 /// Generate a toml String of Cargo.lock from a Resolve.
-pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &Resolve) -> CargoResult<String> {
+pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoResult<String> {
     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>, 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");
     }

src/cargo/ops/resolve.rs

@@ -165,7 +165,7 @@ fn resolve_with_registry<'cfg>(
     registry: &mut PackageRegistry<'cfg>,
 ) -> CargoResult<Resolve> {
     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)
 }

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