fix: Use ops::update_lockfile for consistency with non-breaking update.

Author: torhovland

src/bin/cargo/commands/update.rs

@@ -1,8 +1,11 @@
+use std::collections::HashMap;
+
 use crate::command_prelude::*;
 
 use anyhow::anyhow;
 use cargo::ops::{self, UpdateOptions};
 use cargo::util::print_available_packages;
+use tracing::trace;
 
 pub fn cli() -> Command {
     subcommand("update")
@@ -92,28 +95,38 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
     let update_opts = UpdateOptions {
         recursive: args.flag("recursive"),
         precise: args.get_one::<String>("precise").map(String::as_str),
+        breaking: args.flag("breaking"),
         to_update,
         dry_run: args.dry_run(),
         workspace: args.flag("workspace"),
         gctx,
     };
 
-    if args.flag("breaking") {
-        gctx.cli_unstable()
-            .fail_if_stable_opt("--breaking", 12425)?;
-
-        let upgrades = ops::upgrade_manifests(&mut ws, &update_opts.to_update)?;
-        ops::resolve_ws(&ws, update_opts.dry_run)?;
-        ops::write_manifest_upgrades(&ws, &upgrades, update_opts.dry_run)?;
+    let breaking_update = update_opts.breaking; // or if doing a breaking precise update, coming in #14140.
 
-        if update_opts.dry_run {
-            update_opts
-                .gctx
-                .shell()
-                .warn("aborting update due to dry run")?;
+    // We are using the term "upgrade" here, which is the typical case, but it
+    // can also be a downgrade (in the case of a precise update). In general, it
+    // is a change to a version req matching a precise version.
+    let upgrades = if breaking_update {
+        if update_opts.breaking {
+            gctx.cli_unstable()
+                .fail_if_stable_opt("--breaking", 12425)?;
         }
+
+        trace!("allowing breaking updates");
+        ops::upgrade_manifests(&mut ws, &update_opts.to_update)?
     } else {
-        ops::update_lockfile(&ws, &update_opts)?;
+        HashMap::new()
+    };
+
+    ops::update_lockfile(&ws, &update_opts, &upgrades)?;
+    ops::write_manifest_upgrades(&ws, &upgrades, update_opts.dry_run)?;
+
+    if update_opts.dry_run {
+        update_opts
+            .gctx
+            .shell()
+            .warn("aborting update due to dry run")?;
     }
 
     Ok(())

src/cargo/ops/cargo_update.rs

@@ -9,6 +9,7 @@ use crate::ops;
 use crate::sources::source::QueryKind;
 use crate::util::cache_lock::CacheLockMode;
 use crate::util::context::GlobalContext;
+use crate::util::interning::InternedString;
 use crate::util::toml_mut::dependency::{MaybeWorkspace, Source};
 use crate::util::toml_mut::manifest::LocalManifest;
 use crate::util::toml_mut::upgrade::upgrade_requirement;
@@ -20,12 +21,24 @@ use std::cmp::Ordering;
 use std::collections::{BTreeMap, HashMap, HashSet};
 use tracing::{debug, trace};
 
-pub type UpgradeMap = HashMap<(String, SourceId), Version>;
+/// A map of all breaking upgrades which is filled in by
+/// upgrade_manifests/upgrade_dependency when going through workspace member
+/// manifests, and later used by write_manifest_upgrades in order to know which
+/// upgrades to write to manifest files on disk. Also used by update_lockfile to
+/// know which dependencies to keep unchanged if any have been upgraded (we will
+/// do either breaking or non-breaking updates, but not both).
+pub type UpgradeMap = HashMap<
+    // The key is a tuple of the package name and the source id of the package.
+    (InternedString, SourceId),
+    // The value is the upgraded version of the package.
+    Version,
+>;
 
 pub struct UpdateOptions<'a> {
     pub gctx: &'a GlobalContext,
     pub to_update: Vec<String>,
     pub precise: Option<&'a str>,
+    pub breaking: bool,
     pub recursive: bool,
     pub dry_run: bool,
     pub workspace: bool,
@@ -49,7 +62,11 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> {
     Ok(())
 }
 
-pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoResult<()> {
+pub fn update_lockfile(
+    ws: &Workspace<'_>,
+    opts: &UpdateOptions<'_>,
+    upgrades: &UpgradeMap,
+) -> CargoResult<()> {
     if opts.recursive && opts.precise.is_some() {
         anyhow::bail!("cannot specify both recursive and precise simultaneously")
     }
@@ -165,7 +182,13 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
         .filter(|s| !s.is_registry())
         .collect();
 
-    let keep = |p: &PackageId| !to_avoid_sources.contains(&p.source_id()) && !to_avoid.contains(p);
+    let keep = |p: &PackageId| {
+        (!to_avoid_sources.contains(&p.source_id()) && !to_avoid.contains(p))
+                // In case of `--breaking` (or precise upgrades, coming in
+                // #14140), we want to keep all packages unchanged that didn't
+                // get upgraded.
+                    || (opts.breaking && !upgrades.contains_key(&(p.name(), p.source_id())))
+    };
 
     let mut resolve = ops::resolve_with_previous(
         &mut registry,
@@ -185,11 +208,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
         opts.precise.is_some(),
         &mut registry,
     )?;
-    if opts.dry_run {
-        opts.gctx
-            .shell()
-            .warn("not updating lockfile due to dry run")?;
-    } else {
+    if !opts.dry_run {
         ops::write_pkg_lockfile(ws, &mut resolve)?;
     }
     Ok(())
@@ -361,7 +380,7 @@ fn upgrade_dependency(
             .status_with_color("Upgrading", &upgrade_message, &style::GOOD)?;
     }
 
-    upgrades.insert((name.to_string(), dependency.source_id()), latest.clone());
+    upgrades.insert((name, dependency.source_id()), latest.clone());
 
     let req = OptVersionReq::Req(VersionReq::parse(&latest.to_string())?);
     let mut dep = dependency.clone();
@@ -433,7 +452,7 @@ pub fn write_manifest_upgrades(
                     continue;
                 };
 
-                let Some(latest) = upgrades.get(&(name.to_owned(), source_id)) else {
+                let Some(latest) = upgrades.get(&(name.into(), source_id)) else {
                     trace!("skipping dependency without an upgrade: {name}");
                     continue;
                 };

tests/testsuite/update.rs

@@ -925,7 +925,7 @@ fn dry_run_update() {
 [LOCKING] 1 package to latest compatible version
 [UPDATING] serde v0.1.0 -> v0.1.1
 [NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
-[WARNING] not updating lockfile due to dry run
+[WARNING] aborting update due to dry run
 
 "#]])
         .run();
@@ -1524,7 +1524,7 @@ fn report_behind() {
 [LOCKING] 1 package to latest compatible version
 [UPDATING] breaking v0.1.0 -> v0.1.1 (latest: v0.2.0)
 [NOTE] pass `--verbose` to see 2 unchanged dependencies behind latest
-[WARNING] not updating lockfile due to dry run
+[WARNING] aborting update due to dry run
 
 "#]])
         .run();
@@ -1537,7 +1537,7 @@ fn report_behind() {
 [UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1)
 [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0)
 [NOTE] to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`
-[WARNING] not updating lockfile due to dry run
+[WARNING] aborting update due to dry run
 
 "#]])
         .run();
@@ -1549,7 +1549,7 @@ fn report_behind() {
 [UPDATING] `dummy-registry` index
 [LOCKING] 0 packages to latest compatible versions
 [NOTE] pass `--verbose` to see 3 unchanged dependencies behind latest
-[WARNING] not updating lockfile due to dry run
+[WARNING] aborting update due to dry run
 
 "#]])
         .run();
@@ -1562,7 +1562,7 @@ fn report_behind() {
 [UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1)
 [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0)
 [NOTE] to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`
-[WARNING] not updating lockfile due to dry run
+[WARNING] aborting update due to dry run
 
 "#]])
         .run();
@@ -1912,10 +1912,13 @@ fn update_breaking() {
 [UPDATING] multiple-registries v2.0.0 (registry `alternative`) -> v3.0.0
 [UPDATING] multiple-registries v1.0.0 -> v2.0.0
 [UPDATING] multiple-source-types v1.0.0 -> v2.0.0
+[REMOVING] multiple-versions v1.0.0
+[REMOVING] multiple-versions v2.0.0
 [ADDING] multiple-versions v3.0.0
 [UPDATING] platform-specific v1.0.0 -> v2.0.0
 [UPDATING] shared v1.0.0 -> v2.0.0
 [UPDATING] ws v1.0.0 -> v2.0.0
+[NOTE] pass `--verbose` to see 4 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2108,6 +2111,7 @@ fn update_breaking_specific_packages() {
 [UPDATING] transitive-compatible v1.0.0 -> v1.0.1
 [UPDATING] transitive-incompatible v1.0.0 -> v2.0.0
 [UPDATING] ws v1.0.0 -> v2.0.0
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2163,6 +2167,8 @@ fn update_breaking_specific_packages_that_wont_update() {
         .masquerade_as_nightly_cargo(&["update-breaking"])
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
+[LOCKING] 0 packages to latest compatible versions
+[NOTE] pass `--verbose` to see 5 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2271,14 +2277,27 @@ fn update_breaking_spec_version() {
     // Spec version not matching our current dependencies
     p.cargo("update -Zunstable-options --breaking [email protected]")
         .masquerade_as_nightly_cargo(&["update-breaking"])
-        .with_stderr_data(str![[r#""#]])
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] package ID specification `[email protected]` did not match any packages
+Did you mean one of these?
+
+  [email protected]
+
+"#]])
         .run();
 
     // Spec source not matching our current dependencies
     p.cargo("update -Zunstable-options --breaking https://alternative.com#[email protected]")
         .masquerade_as_nightly_cargo(&["update-breaking"])
-        .with_stderr_data(str![[r#""#]])
-        .run();
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] package ID specification `https://alternative.com/#[email protected]` did not match any packages
+Did you mean one of these?
+
+  [email protected]
+
+"#]])        .run();
 
     // Accepted spec
     p.cargo("update -Zunstable-options --breaking [email protected]")
@@ -2288,6 +2307,7 @@ fn update_breaking_spec_version() {
 [UPGRADING] incompatible ^1.0 -> ^2.0
 [LOCKING] 1 package to latest compatible version
 [UPDATING] incompatible v1.0.0 -> v2.0.0
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2301,6 +2321,7 @@ fn update_breaking_spec_version() {
 [UPGRADING] incompatible ^2.0 -> ^3.0
 [LOCKING] 1 package to latest compatible version
 [UPDATING] incompatible v2.0.0 -> v3.0.0
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2310,19 +2331,35 @@ fn update_breaking_spec_version() {
         .masquerade_as_nightly_cargo(&["update-breaking"])
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
+[LOCKING] 0 packages to latest compatible versions
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
 
     // Non-existing versions
     p.cargo("update -Zunstable-options --breaking [email protected]")
         .masquerade_as_nightly_cargo(&["update-breaking"])
-        .with_stderr_data(str![[r#""#]])
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] package ID specification `[email protected]` did not match any packages
+Did you mean one of these?
+
+  [email protected]
+
+"#]])
         .run();
 
     p.cargo("update -Zunstable-options --breaking [email protected]")
         .masquerade_as_nightly_cargo(&["update-breaking"])
-        .with_stderr_data(str![[r#""#]])
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] package ID specification `[email protected]` did not match any packages
+Did you mean one of these?
+
+  [email protected]
+
+"#]])
         .run();
 }
 
@@ -2376,6 +2413,7 @@ fn update_breaking_spec_version_transitive() {
 [UPGRADING] dep ^1.0 -> ^3.0
 [LOCKING] 1 package to latest compatible version
 [UPDATING] dep v1.0.0 -> v3.0.0
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2385,6 +2423,8 @@ fn update_breaking_spec_version_transitive() {
         .masquerade_as_nightly_cargo(&["update-breaking"])
         .with_stderr_data(str![[r#"
 [UPDATING] `dummy-registry` index
+[LOCKING] 0 packages to latest compatible versions
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
 
 "#]])
         .run();
@@ -2453,6 +2493,8 @@ fn update_breaking_mixed_compatibility() {
 [UPDATING] `dummy-registry` index
 [UPGRADING] mixed-compatibility ^1.0 -> ^2.0
 [LOCKING] 1 package to latest compatible version
+[REMOVING] mixed-compatibility v1.0.0
+[REMOVING] mixed-compatibility v2.0.0
 [ADDING] mixed-compatibility v2.0.1
 
 "#]])
@@ -2544,6 +2586,7 @@ fn update_breaking_mixed_pinning_renaming() {
 [ADDING] mixed-pinned v2.0.0
 [ADDING] mixed-ws-pinned v2.0.0
 [ADDING] renamed-from v2.0.0
+[NOTE] pass `--verbose` to see 3 unchanged dependencies behind latest
 
 "#]])
         .run();