Skip to content

Commit

Permalink
fix: Use ops::update_lockfile for consistency with non-breaking update.
Browse files Browse the repository at this point in the history
  • Loading branch information
@torhovland
torhovland committed Jul 21, 2024
1 parent 8c8ad48 commit 324ad02
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 32 deletions.
39 changes: 26 additions & 13 deletions src/bin/cargo/commands/update.rs
View file
Original file line number Diff line number Diff line change
@@ -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")
Expand Down Expand Up @@ -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(())
Expand Down
68 changes: 58 additions & 10 deletions src/cargo/ops/cargo_update.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -20,12 +21,25 @@ 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 package identifier consisting of the name and the source id.
(InternedString, SourceId),
// The value is the original version requirement before upgrade, and the
// upgraded version.
(VersionReq, 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,
Expand All @@ -49,7 +63,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")
}
Expand Down Expand Up @@ -91,7 +109,38 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
let mut registry = ws.package_registry()?;
let mut to_avoid = HashSet::new();

if opts.to_update.is_empty() {
if opts.breaking {
// We don't necessarily want to update all specified packages. If we are
// doing a breaking update (or precise upgrades, coming in #14140), we
// don't want to touch any packages that have no breaking updates. So we
// want to only avoid all packages that got upgraded.

for name in opts.to_update.iter() {
// We still want to query any specified package, for the sake of
// outputting errors if they don't exist.
previous_resolve.query(name)?;
}

for ((name, source_id), (version_req, _)) in upgrades.iter() {
if let Some(matching_dep) = previous_resolve.iter().find(|dep| {
dep.name() == *name
&& dep.source_id() == *source_id
&& version_req.matches(dep.version())
}) {
let spec = PackageIdSpec::new(name.to_string())
.with_url(source_id.url().clone())
.with_version(matching_dep.version().clone().into())
.to_string();
let pid = previous_resolve.query(&spec)?;
to_avoid.insert(pid);
} else {
// Should never happen
anyhow::bail!(
"no package named `{name}` with source `{source_id}` and version matching `{version_req}` in the previous lockfile",
)
}
}
} else if opts.to_update.is_empty() {
if !opts.workspace {
to_avoid.extend(previous_resolve.iter());
to_avoid.extend(previous_resolve.unused_patches());
Expand Down Expand Up @@ -185,11 +234,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(())
Expand Down Expand Up @@ -361,7 +406,10 @@ 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()),
(current.clone(), latest.clone()),
);

let req = OptVersionReq::Req(VersionReq::parse(&latest.to_string())?);
let mut dep = dependency.clone();
Expand Down Expand Up @@ -433,7 +481,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;
};
Expand Down
62 changes: 53 additions & 9 deletions tests/testsuite/update.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -2271,13 +2277,27 @@ fn update_breaking_spec_version() {
// 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#""#]])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] package ID specification `incompatible@2.0.0` did not match any packages
Did you mean one of these?
incompatible@1.0.0
"#]])
.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#""#]])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] package ID specification `https://alternative.com/#incompatible@1.0.0` did not match any packages
Did you mean one of these?
incompatible@1.0.0
"#]])
.run();

// Accepted spec
Expand All @@ -2288,6 +2308,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();
Expand All @@ -2301,6 +2322,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();
Expand All @@ -2310,19 +2332,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 incompatible@9.0.0")
.masquerade_as_nightly_cargo(&["update-breaking"])
.with_stderr_data(str![[r#""#]])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] package ID specification `incompatible@9.0.0` did not match any packages
Did you mean one of these?
incompatible@3.0.0
"#]])
.run();

p.cargo("update -Zunstable-options --breaking compatible@9.0.0")
.masquerade_as_nightly_cargo(&["update-breaking"])
.with_stderr_data(str![[r#""#]])
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] package ID specification `compatible@9.0.0` did not match any packages
Did you mean one of these?
compatible@1.0.0
"#]])
.run();
}

Expand Down Expand Up @@ -2376,6 +2414,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();
Expand All @@ -2385,6 +2424,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();
Expand Down Expand Up @@ -2453,6 +2494,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
"#]])
Expand Down Expand Up @@ -2544,6 +2587,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();
Expand Down

0 comments on commit 324ad02

Please sign in to comment.