From abc86e24c83eca8d407122d1fab6352c2cbd3927 Mon Sep 17 00:00:00 2001 From: Tor Hovland <55164+torhovland@users.noreply.github.com> Date: Mon, 24 Jun 2024 10:13:13 +0200 Subject: [PATCH 1/4] refactor: Clean up test output and tracing, and introduce variable. --- src/cargo/ops/cargo_update.rs | 60 +++++++++++------------------------ tests/testsuite/update.rs | 15 ++++----- 2 files changed, 25 insertions(+), 50 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 463af374ede..d4da624efbd 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -263,46 +263,37 @@ fn upgrade_dependency( let renamed_to = dependency.name_in_toml(); if name != renamed_to { - trace!( - "skipping dependency renamed from `{}` to `{}`", - name, - renamed_to - ); + trace!("skipping dependency renamed from `{name}` to `{renamed_to}`"); return Ok(dependency); } if !to_update.is_empty() && !to_update.contains(&name.to_string()) { - trace!("skipping dependency `{}` not selected for upgrading", name); + trace!("skipping dependency `{name}` not selected for upgrading"); return Ok(dependency); } if !dependency.source_id().is_registry() { - trace!("skipping non-registry dependency: {}", name); + trace!("skipping non-registry dependency: {name}"); return Ok(dependency); } let version_req = dependency.version_req(); let OptVersionReq::Req(current) = version_req else { - trace!( - "skipping dependency `{}` without a simple version requirement: {}", - name, - version_req - ); + trace!("skipping dependency `{name}` without a simple version requirement: {version_req}"); return Ok(dependency); }; let [comparator] = ¤t.comparators[..] else { trace!( - "skipping dependency `{}` with multiple version comparators: {:?}", - name, + "skipping dependency `{name}` with multiple version comparators: {:?}", ¤t.comparators ); return Ok(dependency); }; if comparator.op != Op::Caret { - trace!("skipping non-caret dependency `{}`: {}", name, comparator); + trace!("skipping non-caret dependency `{name}`: {comparator}"); return Ok(dependency); } @@ -332,30 +323,21 @@ fn upgrade_dependency( }; let Some(latest) = latest else { - trace!( - "skipping dependency `{}` without any published versions", - name - ); + trace!("skipping dependency `{name}` without any published versions"); return Ok(dependency); }; if current.matches(&latest) { - trace!( - "skipping dependency `{}` without a breaking update available", - name - ); + trace!("skipping dependency `{name}` without a breaking update available"); return Ok(dependency); } let Some(new_req_string) = upgrade_requirement(¤t.to_string(), latest)? else { - trace!( - "skipping dependency `{}` because the version requirement didn't change", - name - ); + trace!("skipping dependency `{name}` because the version requirement didn't change"); return Ok(dependency); }; - let upgrade_message = format!("{} {} -> {}", name, current, new_req_string); + let upgrade_message = format!("{name} {current} -> {new_req_string}"); trace!(upgrade_message); if upgrade_messages.insert(upgrade_message.clone()) { @@ -389,10 +371,7 @@ pub fn write_manifest_upgrades( .collect::>(); for manifest_path in manifest_paths { - trace!( - "updating TOML manifest at `{:?}` with upgraded dependencies", - manifest_path - ); + trace!("updating TOML manifest at `{manifest_path:?}` with upgraded dependencies"); let crate_root = manifest_path .parent() @@ -410,31 +389,28 @@ pub fn write_manifest_upgrades( dep_key_str, dep_item, )?; + let name = &dependency.name; let Some(current) = dependency.version() else { - trace!("skipping dependency without a version: {}", dependency.name); + trace!("skipping dependency without a version: {name}"); continue; }; let (MaybeWorkspace::Other(source_id), Some(Source::Registry(source))) = (dependency.source_id(ws.gctx())?, dependency.source()) else { - trace!("skipping non-registry dependency: {}", dependency.name); + trace!("skipping non-registry dependency: {name}"); continue; }; - let Some(latest) = upgrades.get(&(dependency.name.to_owned(), source_id)) else { - trace!( - "skipping dependency without an upgrade: {}", - dependency.name - ); + let Some(latest) = upgrades.get(&(name.to_owned(), source_id)) else { + trace!("skipping dependency without an upgrade: {name}"); continue; }; let Some(new_req_string) = upgrade_requirement(current, latest)? else { trace!( - "skipping dependency `{}` because the version requirement didn't change", - dependency.name + "skipping dependency `{name}` because the version requirement didn't change" ); continue; }; @@ -444,7 +420,7 @@ pub fn write_manifest_upgrades( source.version = new_req_string; dep.source = Some(Source::Registry(source)); - trace!("upgrading dependency {}", dependency.name); + trace!("upgrading dependency {name}"); dep.update_toml(&crate_root, &mut dep_key, dep_item); manifest_has_changed = true; any_file_has_changed = true; diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 3f39f8c45aa..974eac1bf6e 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -1893,8 +1893,7 @@ fn update_breaking() { [workspace.dependencies] ws = "2.0" # This line gets partially rewritten - -"#]], + "#]], ); let foo_manifest = p.read_file("foo/Cargo.toml"); @@ -1983,12 +1982,12 @@ fn update_breaking_specific_packages() { .file( "Cargo.toml", r#" - [workspace] - members = ["foo", "bar"] + [workspace] + members = ["foo", "bar"] - [workspace.dependencies] - ws = "1.0" - "#, + [workspace.dependencies] + ws = "1.0" + "#, ) .file( "foo/Cargo.toml", @@ -2019,7 +2018,7 @@ fn update_breaking_specific_packages() { just-bar = "1.0" shared = "1.0" ws.workspace = true - "#, + "#, ) .file("bar/src/lib.rs", "") .build(); From 8882d0504eef97ccc919e1788ad54af644289ac1 Mon Sep 17 00:00:00 2001 From: Tor Hovland <55164+torhovland@users.noreply.github.com> Date: Wed, 12 Jun 2024 09:18:32 +0200 Subject: [PATCH 2/4] test: More `update --breaking` tests. --- tests/testsuite/update.rs | 531 +++++++++++++++++++++++++++++++++++++- 1 file changed, 530 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 974eac1bf6e..a3d5cb3882c 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -1722,6 +1722,7 @@ fn update_breaking() { Package::new("yanked", "1.0.0").publish(); Package::new("ws", "1.0.0").publish(); Package::new("shared", "1.0.0").publish(); + Package::new("multiple-locations", "1.0.0").publish(); Package::new("multiple-versions", "1.0.0").publish(); Package::new("multiple-versions", "2.0.0").publish(); Package::new("alternative-1", "1.0.0") @@ -1736,6 +1737,9 @@ fn update_breaking() { .alternative(true) .publish(); Package::new("multiple-source-types", "1.0.0").publish(); + Package::new("platform-specific", "1.0.0").publish(); + Package::new("dev", "1.0.0").publish(); + Package::new("build", "1.0.0").publish(); let p = project() .file( @@ -1771,6 +1775,7 @@ fn update_breaking() { yanked = "1.0" # Comment ws.workspace = true # Comment shared = "1.0" # Comment + multiple-locations = { path = "../multiple-locations", version = "1.0" } # Comment multiple-versions = "1.0" # Comment alternative-1 = { registry = "alternative", version = "1.0" } # Comment multiple-registries = "1.0" # Comment @@ -1780,6 +1785,15 @@ fn update_breaking() { [dependencies.alternative-2] # Comment version = "1.0" # Comment registry = "alternative" # Comment + + [target.'cfg(unix)'.dependencies] + platform-specific = "1.0" # Comment + + [dev-dependencies] + dev = "1.0" # Comment + + [build-dependencies] + build = "1.0" # Comment "#, ) .file("foo/src/lib.rs", "") @@ -1800,6 +1814,17 @@ fn update_breaking() { "#, ) .file("bar/src/lib.rs", "") + .file( + "multiple-locations/Cargo.toml", + r#" + [package] + name = "multiple-locations" + version = "1.0.0" + edition = "2015" + authors = [] + "#, + ) + .file("multiple-locations/src/lib.rs", "") .file( "multiple-source-types/Cargo.toml", r#" @@ -1821,6 +1846,7 @@ fn update_breaking() { Package::new("less-than", "1.0.1").publish(); Package::new("renamed-from", "1.0.1").publish(); Package::new("ws", "1.0.1").publish(); + Package::new("multiple-locations", "1.0.1").publish(); Package::new("multiple-versions", "1.0.1").publish(); Package::new("multiple-versions", "2.0.1").publish(); Package::new("alternative-1", "1.0.1") @@ -1829,6 +1855,9 @@ fn update_breaking() { Package::new("alternative-2", "1.0.1") .alternative(true) .publish(); + Package::new("platform-specific", "1.0.1").publish(); + Package::new("dev", "1.0.1").publish(); + Package::new("build", "1.0.1").publish(); Package::new("incompatible", "2.0.0").publish(); Package::new("pinned", "2.0.0").publish(); @@ -1838,6 +1867,7 @@ fn update_breaking() { Package::new("yanked", "2.0.0").yanked(true).publish(); Package::new("ws", "2.0.0").publish(); Package::new("shared", "2.0.0").publish(); + Package::new("multiple-locations", "2.0.0").publish(); Package::new("multiple-versions", "3.0.0").publish(); Package::new("alternative-1", "2.0.0") .alternative(true) @@ -1851,6 +1881,9 @@ fn update_breaking() { .alternative(true) .publish(); Package::new("multiple-source-types", "2.0.0").publish(); + Package::new("platform-specific", "2.0.0").publish(); + Package::new("dev", "2.0.0").publish(); + Package::new("build", "2.0.0").publish(); p.cargo("update -Zunstable-options --breaking") .masquerade_as_nightly_cargo(&["update-breaking"]) @@ -1867,14 +1900,20 @@ fn update_breaking() { [UPGRADING] multiple-registries ^1.0 -> ^2.0 [UPGRADING] multiple-versions ^1.0 -> ^3.0 [UPGRADING] ws ^1.0 -> ^2.0 -[LOCKING] 9 packages to latest compatible versions +[UPGRADING] dev ^1.0 -> ^2.0 +[UPGRADING] build ^1.0 -> ^2.0 +[UPGRADING] platform-specific ^1.0 -> ^2.0 +[LOCKING] 12 packages to latest compatible versions [UPDATING] alternative-1 v1.0.0 (registry `alternative`) -> v2.0.0 [UPDATING] alternative-2 v1.0.0 (registry `alternative`) -> v2.0.0 +[UPDATING] build v1.0.0 -> v2.0.0 +[UPDATING] dev v1.0.0 -> v2.0.0 [UPDATING] incompatible v1.0.0 -> v2.0.0 [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 [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 @@ -1920,6 +1959,7 @@ fn update_breaking() { yanked = "1.0" # Comment ws.workspace = true # Comment shared = "2.0" # Comment + multiple-locations = { path = "../multiple-locations", version = "1.0" } # Comment multiple-versions = "3.0" # Comment alternative-1 = { registry = "alternative", version = "2.0" } # Comment multiple-registries = "2.0" # Comment @@ -1929,6 +1969,15 @@ fn update_breaking() { [dependencies.alternative-2] # Comment version = "2.0" # Comment registry = "alternative" # Comment + + [target.'cfg(unix)'.dependencies] + platform-specific = "2.0" # Comment + + [dev-dependencies] + dev = "2.0" # Comment + + [build-dependencies] + build = "2.0" # Comment "#]], ); @@ -2063,3 +2112,483 @@ fn update_breaking_specific_packages() { "#]]) .run(); } + +#[cargo_test] +fn update_breaking_specific_packages_that_wont_update() { + Package::new("compatible", "1.0.0").publish(); + Package::new("renamed-from", "1.0.0").publish(); + Package::new("non-semver", "1.0.0").publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("transitive-compatible", "1.0.0").build()) + .add_dep(Dependency::new("transitive-incompatible", "1.0.0").build()) + .publish(); + Package::new("transitive-compatible", "1.0.0").publish(); + Package::new("transitive-incompatible", "1.0.0").publish(); + + let crate_manifest = r#" + # Check if formatting is preserved + + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + compatible = "1.0" # Comment + renamed-to = { package = "renamed-from", version = "1.0" } # Comment + non-semver = "~1.0" # Comment + bar = "1.0" # Comment + "#; + + let p = project() + .file("Cargo.toml", crate_manifest) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + let lock_file = p.read_file("Cargo.lock"); + + Package::new("compatible", "1.0.1").publish(); + Package::new("renamed-from", "1.0.1").publish(); + Package::new("non-semver", "1.0.1").publish(); + Package::new("transitive-compatible", "1.0.1").publish(); + Package::new("transitive-incompatible", "1.0.1").publish(); + + Package::new("renamed-from", "2.0.0").publish(); + Package::new("non-semver", "2.0.0").publish(); + Package::new("transitive-incompatible", "2.0.0").publish(); + + p.cargo("update -Zunstable-options --breaking compatible renamed-from non-semver transitive-compatible transitive-incompatible") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index + +"#]]) + .run(); + + let crate_manifest_after = p.read_file("Cargo.toml"); + assert_e2e().eq(&crate_manifest_after, crate_manifest); + + let lock_file_after = p.read_file("Cargo.lock"); + assert_e2e().eq(&lock_file_after, lock_file); + + p.cargo( + "update compatible renamed-from non-semver transitive-compatible transitive-incompatible", + ) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[LOCKING] 5 packages to latest compatible versions +[UPDATING] compatible v1.0.0 -> v1.0.1 +[UPDATING] non-semver v1.0.0 -> v1.0.1 (latest: v2.0.0) +[UPDATING] renamed-from v1.0.0 -> v1.0.1 (latest: v2.0.0) +[UPDATING] transitive-compatible v1.0.0 -> v1.0.1 +[UPDATING] transitive-incompatible v1.0.0 -> v1.0.1 (latest: v2.0.0) + +"#]]) + .run(); +} + +#[cargo_test] +fn update_breaking_without_lock_file() { + Package::new("compatible", "1.0.0").publish(); + Package::new("incompatible", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + compatible = "1.0" # Comment + incompatible = "1.0" # Comment + "#, + ) + .file("src/lib.rs", "") + .build(); + + Package::new("compatible", "1.0.1").publish(); + Package::new("incompatible", "1.0.1").publish(); + + Package::new("incompatible", "2.0.0").publish(); + + p.cargo("update -Zunstable-options --breaking") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] incompatible ^1.0 -> ^2.0 +[LOCKING] 3 packages to latest compatible versions + +"#]]) + .run(); +} + +#[cargo_test] +fn update_breaking_spec_version() { + Package::new("compatible", "1.0.0").publish(); + Package::new("incompatible", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + compatible = "1.0" # Comment + incompatible = "1.0" # Comment + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + Package::new("compatible", "1.0.1").publish(); + Package::new("incompatible", "1.0.1").publish(); + + Package::new("incompatible", "2.0.0").publish(); + + // Invalid spec + p.cargo("update -Zunstable-options --breaking incompatible@foo") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Spec version not matching our current dependencies + p.cargo("update -Zunstable-options --breaking incompatible@2.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Spec source not matching our current dependencies + p.cargo("update -Zunstable-options --breaking https://alternative.com#incompatible@1.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Accepted spec + p.cargo("update -Zunstable-options --breaking incompatible@1.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Accepted spec, full format + Package::new("incompatible", "3.0.0").publish(); + p.cargo("update -Zunstable-options --breaking https://github.com/rust-lang/crates.io-index#incompatible@2.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Spec matches a dependency that will not be upgraded + p.cargo("update -Zunstable-options --breaking compatible@1.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // Non-existing versions + p.cargo("update -Zunstable-options --breaking incompatible@9.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + p.cargo("update -Zunstable-options --breaking compatible@9.0.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); +} + +#[cargo_test] +fn update_breaking_spec_version_transitive() { + Package::new("dep", "1.0.0").publish(); + Package::new("dep", "1.1.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + dep = "1.0" + bar = { path = "bar", version = "0.0.1" } + "#, + ) + .file("src/lib.rs", "") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + dep = "1.1" + "#, + ) + .file("bar/src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + Package::new("dep", "1.1.1").publish(); + Package::new("dep", "2.0.0").publish(); + + // Will upgrade the direct dependency + p.cargo("update -Zunstable-options --breaking dep@1.0") + .masquerade_as_nightly_cargo(&["update-breaking"]) + // FIXME: Should upgrade a dependency here. + .with_stderr_data(str![[r#""#]]) + .run(); + + // But not the transitive one, because bar is not a workspace member + p.cargo("update -Zunstable-options --breaking dep@1.1") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#""#]]) + .run(); + + // A non-breaking update is different, as it will update transitive dependencies + p.cargo("update dep@1.1") + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[LOCKING] 1 package to latest compatible version +[UPDATING] dep v1.1.0 -> v1.1.1 (latest: v2.0.0) + +"#]]) + .run(); +} + +#[cargo_test] +fn update_breaking_mixed_compatibility() { + Package::new("mixed-compatibility", "1.0.0").publish(); + Package::new("mixed-compatibility", "2.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["foo", "bar"] + "#, + ) + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-compatibility = "1.0" + "#, + ) + .file("foo/src/lib.rs", "") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-compatibility = "2.0" + "#, + ) + .file("bar/src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + Package::new("mixed-compatibility", "2.0.1").publish(); + + p.cargo("update -Zunstable-options --breaking") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] mixed-compatibility ^1.0 -> ^2.0 +[LOCKING] 1 package to latest compatible version +[ADDING] mixed-compatibility v2.0.1 + +"#]]) + .run(); +} + +#[cargo_test] +fn update_breaking_mixed_pinning_renaming() { + Package::new("mixed-pinned", "1.0.0").publish(); + Package::new("mixed-ws-pinned", "1.0.0").publish(); + Package::new("renamed-from", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["pinned", "unpinned", "mixed"] + + [workspace.dependencies] + mixed-ws-pinned = "=1.0" + "#, + ) + .file( + "pinned/Cargo.toml", + r#" + [package] + name = "pinned" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-pinned = "=1.0" + mixed-ws-pinned.workspace = true + renamed-to = { package = "renamed-from", version = "1.0" } + "#, + ) + .file("pinned/src/lib.rs", "") + .file( + "unpinned/Cargo.toml", + r#" + [package] + name = "unpinned" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-pinned = "1.0" + mixed-ws-pinned = "1.0" + renamed-from = "1.0" + "#, + ) + .file("unpinned/src/lib.rs", "") + .file( + "mixed/Cargo.toml", + r#" + [package] + name = "mixed" + version = "0.0.1" + edition = "2015" + authors = [] + + [target.'cfg(windows)'.dependencies] + mixed-pinned = "1.0" + + [target.'cfg(unix)'.dependencies] + mixed-pinned = "=1.0" + "#, + ) + .file("mixed/src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + + Package::new("mixed-pinned", "2.0.0").publish(); + Package::new("mixed-ws-pinned", "2.0.0").publish(); + Package::new("renamed-from", "2.0.0").publish(); + + p.cargo("update -Zunstable-options --breaking") + .masquerade_as_nightly_cargo(&["update-breaking"]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] mixed-pinned ^1.0 -> ^2.0 +[UPGRADING] mixed-ws-pinned ^1.0 -> ^2.0 +[UPGRADING] renamed-from ^1.0 -> ^2.0 +[LOCKING] 3 packages to latest compatible versions +[ADDING] mixed-pinned v2.0.0 +[ADDING] mixed-ws-pinned v2.0.0 +[ADDING] renamed-from v2.0.0 + +"#]]) + .run(); + + let root_manifest = p.read_file("Cargo.toml"); + assert_e2e().eq( + &root_manifest, + // FIXME: The pinned dependency should not be upgraded. + str![[r#" + + [workspace] + members = ["pinned", "unpinned", "mixed"] + + [workspace.dependencies] + mixed-ws-pinned = "=2.0" + "#]], + ); + + let pinned_manifest = p.read_file("pinned/Cargo.toml"); + assert_e2e().eq( + &pinned_manifest, + // FIXME: The pinned and renamed dependencies should not be upgraded. + str![[r#" + + [package] + name = "pinned" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-pinned = "=2.0" + mixed-ws-pinned.workspace = true + renamed-to = { package = "renamed-from", version = "2.0" } + "#]], + ); + + let unpinned_manifest = p.read_file("unpinned/Cargo.toml"); + assert_e2e().eq( + &unpinned_manifest, + str![[r#" + + [package] + name = "unpinned" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + mixed-pinned = "2.0" + mixed-ws-pinned = "2.0" + renamed-from = "2.0" + "#]], + ); + + let mixed_manifest = p.read_file("mixed/Cargo.toml"); + assert_e2e().eq( + &mixed_manifest, + // FIXME: The pinned dependency should not be upgraded. + str![[r#" + + [package] + name = "mixed" + version = "0.0.1" + edition = "2015" + authors = [] + + [target.'cfg(windows)'.dependencies] + mixed-pinned = "2.0" + + [target.'cfg(unix)'.dependencies] + mixed-pinned = "=2.0" + "#]], + ); +} From f096a9446f2f87e996a4e19a5029bcc425d25e86 Mon Sep 17 00:00:00 2001 From: Tor Hovland <55164+torhovland@users.noreply.github.com> Date: Wed, 12 Jun 2024 13:03:02 +0200 Subject: [PATCH 3/4] fix: `update --breaking` now understands package@version. --- src/cargo/ops/cargo_update.rs | 22 ++++++++++++++++--- tests/testsuite/update.rs | 41 +++++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index d4da624efbd..5ffc14f0732 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -222,6 +222,11 @@ pub fn upgrade_manifests( let mut upgrades = HashMap::new(); let mut upgrade_messages = HashSet::new(); + let to_update = to_update + .iter() + .map(|s| PackageIdSpec::parse(s)) + .collect::, _>>()?; + // Updates often require a lot of modifications to the registry, so ensure // that we're synchronized against other Cargos. let _lock = gctx.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; @@ -239,7 +244,7 @@ pub fn upgrade_manifests( .try_map_dependencies(|d| { upgrade_dependency( &gctx, - to_update, + &to_update, &mut registry, &mut upgrades, &mut upgrade_messages, @@ -253,7 +258,7 @@ pub fn upgrade_manifests( fn upgrade_dependency( gctx: &GlobalContext, - to_update: &Vec, + to_update: &Vec, registry: &mut PackageRegistry<'_>, upgrades: &mut UpgradeMap, upgrade_messages: &mut HashSet, @@ -267,7 +272,18 @@ fn upgrade_dependency( return Ok(dependency); } - if !to_update.is_empty() && !to_update.contains(&name.to_string()) { + if !to_update.is_empty() + && !to_update.iter().any(|spec| { + spec.name() == name.as_str() + && dependency.source_id().is_registry() + && spec + .url() + .map_or(true, |url| url == dependency.source_id().url()) + && spec + .version() + .map_or(true, |v| dependency.version_req().matches(&v)) + }) + { trace!("skipping dependency `{name}` not selected for upgrading"); return Ok(dependency); } diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index a3d5cb3882c..77cccad40fb 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -2261,7 +2261,11 @@ fn update_breaking_spec_version() { // Invalid spec p.cargo("update -Zunstable-options --breaking incompatible@foo") .masquerade_as_nightly_cargo(&["update-breaking"]) - .with_stderr_data(str![[r#""#]]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] expected a version like "1.32" + +"#]]) .run(); // Spec version not matching our current dependencies @@ -2279,20 +2283,35 @@ fn update_breaking_spec_version() { // Accepted spec p.cargo("update -Zunstable-options --breaking incompatible@1.0.0") .masquerade_as_nightly_cargo(&["update-breaking"]) - .with_stderr_data(str![[r#""#]]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] incompatible ^1.0 -> ^2.0 +[LOCKING] 1 package to latest compatible version +[UPDATING] incompatible v1.0.0 -> v2.0.0 + +"#]]) .run(); // Accepted spec, full format Package::new("incompatible", "3.0.0").publish(); p.cargo("update -Zunstable-options --breaking https://github.com/rust-lang/crates.io-index#incompatible@2.0.0") .masquerade_as_nightly_cargo(&["update-breaking"]) - .with_stderr_data(str![[r#""#]]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] incompatible ^2.0 -> ^3.0 +[LOCKING] 1 package to latest compatible version +[UPDATING] incompatible v2.0.0 -> v3.0.0 + +"#]]) .run(); // Spec matches a dependency that will not be upgraded p.cargo("update -Zunstable-options --breaking compatible@1.0.0") .masquerade_as_nightly_cargo(&["update-breaking"]) - .with_stderr_data(str![[r#""#]]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index + +"#]]) .run(); // Non-existing versions @@ -2352,14 +2371,22 @@ fn update_breaking_spec_version_transitive() { // Will upgrade the direct dependency p.cargo("update -Zunstable-options --breaking dep@1.0") .masquerade_as_nightly_cargo(&["update-breaking"]) - // FIXME: Should upgrade a dependency here. - .with_stderr_data(str![[r#""#]]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index +[UPGRADING] dep ^1.0 -> ^2.0 +[LOCKING] 1 package to latest compatible version +[ADDING] dep v2.0.0 + +"#]]) .run(); // But not the transitive one, because bar is not a workspace member p.cargo("update -Zunstable-options --breaking dep@1.1") .masquerade_as_nightly_cargo(&["update-breaking"]) - .with_stderr_data(str![[r#""#]]) + .with_stderr_data(str![[r#" +[UPDATING] `[..]` index + +"#]]) .run(); // A non-breaking update is different, as it will update transitive dependencies From f41bdc142bab53dd22b2e86f3a2f270d10908ba6 Mon Sep 17 00:00:00 2001 From: Tor Hovland <55164+torhovland@users.noreply.github.com> Date: Thu, 20 Jun 2024 12:12:54 +0200 Subject: [PATCH 4/4] fix: `update --breaking` now handles mixed renaming and pinning. --- src/cargo/ops/cargo_update.rs | 36 ++++++++++++++++++++++++++---- src/cargo/util/toml_mut/upgrade.rs | 8 ++++--- tests/testsuite/update.rs | 11 ++++----- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 5ffc14f0732..c3d367311e1 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -348,7 +348,7 @@ fn upgrade_dependency( return Ok(dependency); } - let Some(new_req_string) = upgrade_requirement(¤t.to_string(), latest)? else { + let Some((new_req_string, _)) = upgrade_requirement(¤t.to_string(), latest)? else { trace!("skipping dependency `{name}` because the version requirement didn't change"); return Ok(dependency); }; @@ -369,8 +369,17 @@ fn upgrade_dependency( Ok(dep) } -/// Update manifests with upgraded versions, and write to disk. Based on cargo-edit. -/// Returns true if any file has changed. +/// Update manifests with upgraded versions, and write to disk. Based on +/// cargo-edit. Returns true if any file has changed. +/// +/// Some of the checks here are duplicating checks already done in +/// upgrade_manifests/upgrade_dependency. Why? Let's say upgrade_dependency has +/// found that dependency foo was eligible for an upgrade. But foo can occur in +/// multiple manifest files, and even multiple times in the same manifest file, +/// and may be pinned, renamed, etc. in some of the instances. So we still need +/// to check here which dependencies to actually modify. So why not drop the +/// upgrade map and redo all checks here? Because then we'd have to query the +/// registries again to find the latest versions. pub fn write_manifest_upgrades( ws: &Workspace<'_>, upgrades: &UpgradeMap, @@ -407,6 +416,11 @@ pub fn write_manifest_upgrades( )?; let name = &dependency.name; + if let Some(renamed_to) = dependency.rename { + trace!("skipping dependency renamed from `{name}` to `{renamed_to}`"); + continue; + } + let Some(current) = dependency.version() else { trace!("skipping dependency without a version: {name}"); continue; @@ -424,13 +438,27 @@ pub fn write_manifest_upgrades( continue; }; - let Some(new_req_string) = upgrade_requirement(current, latest)? else { + let Some((new_req_string, new_req)) = upgrade_requirement(current, latest)? else { trace!( "skipping dependency `{name}` because the version requirement didn't change" ); continue; }; + let [comparator] = &new_req.comparators[..] else { + trace!( + "skipping dependency `{}` with multiple version comparators: {:?}", + name, + new_req.comparators + ); + continue; + }; + + if comparator.op != Op::Caret { + trace!("skipping non-caret dependency `{}`: {}", name, comparator); + continue; + } + let mut dep = dependency.clone(); let mut source = source.clone(); source.version = new_req_string; diff --git a/src/cargo/util/toml_mut/upgrade.rs b/src/cargo/util/toml_mut/upgrade.rs index 5bf7dacee4b..13d5563450c 100644 --- a/src/cargo/util/toml_mut/upgrade.rs +++ b/src/cargo/util/toml_mut/upgrade.rs @@ -7,7 +7,7 @@ use crate::CargoResult; pub(crate) fn upgrade_requirement( req: &str, version: &semver::Version, -) -> CargoResult> { +) -> CargoResult> { let req_text = req.to_string(); let raw_req = semver::VersionReq::parse(&req_text) .expect("semver to generate valid version requirements"); @@ -40,7 +40,7 @@ pub(crate) fn upgrade_requirement( if new_req_text == req_text { Ok(None) } else { - Ok(Some(new_req_text)) + Ok(Some((new_req_text, new_req))) } } } @@ -103,7 +103,9 @@ mod test { #[track_caller] fn assert_req_bump<'a, O: Into>>(version: &str, req: &str, expected: O) { let version = semver::Version::parse(version).unwrap(); - let actual = upgrade_requirement(req, &version).unwrap(); + let actual = upgrade_requirement(req, &version) + .unwrap() + .map(|(actual, _req)| actual); let expected = expected.into(); assert_eq!(actual.as_deref(), expected); } diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 77cccad40fb..1e74e32f93c 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -2551,21 +2551,19 @@ fn update_breaking_mixed_pinning_renaming() { let root_manifest = p.read_file("Cargo.toml"); assert_e2e().eq( &root_manifest, - // FIXME: The pinned dependency should not be upgraded. str![[r#" [workspace] members = ["pinned", "unpinned", "mixed"] [workspace.dependencies] - mixed-ws-pinned = "=2.0" + mixed-ws-pinned = "=1.0" "#]], ); let pinned_manifest = p.read_file("pinned/Cargo.toml"); assert_e2e().eq( &pinned_manifest, - // FIXME: The pinned and renamed dependencies should not be upgraded. str![[r#" [package] @@ -2575,9 +2573,9 @@ fn update_breaking_mixed_pinning_renaming() { authors = [] [dependencies] - mixed-pinned = "=2.0" + mixed-pinned = "=1.0" mixed-ws-pinned.workspace = true - renamed-to = { package = "renamed-from", version = "2.0" } + renamed-to = { package = "renamed-from", version = "1.0" } "#]], ); @@ -2602,7 +2600,6 @@ fn update_breaking_mixed_pinning_renaming() { let mixed_manifest = p.read_file("mixed/Cargo.toml"); assert_e2e().eq( &mixed_manifest, - // FIXME: The pinned dependency should not be upgraded. str![[r#" [package] @@ -2615,7 +2612,7 @@ fn update_breaking_mixed_pinning_renaming() { mixed-pinned = "2.0" [target.'cfg(unix)'.dependencies] - mixed-pinned = "=2.0" + mixed-pinned = "=1.0" "#]], ); }