Skip to content

Commit

Permalink
fix: update --breaking now handles mixed renaming and pinning.
Browse files Browse the repository at this point in the history
  • Loading branch information
torhovland committed Jun 27, 2024
1 parent f096a94 commit f41bdc1
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 14 deletions.
36 changes: 32 additions & 4 deletions src/cargo/ops/cargo_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ fn upgrade_dependency(
return Ok(dependency);
}

let Some(new_req_string) = upgrade_requirement(&current.to_string(), latest)? else {
let Some((new_req_string, _)) = upgrade_requirement(&current.to_string(), latest)? else {
trace!("skipping dependency `{name}` because the version requirement didn't change");
return Ok(dependency);
};
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
8 changes: 5 additions & 3 deletions src/cargo/util/toml_mut/upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::CargoResult;
pub(crate) fn upgrade_requirement(
req: &str,
version: &semver::Version,
) -> CargoResult<Option<String>> {
) -> CargoResult<Option<(String, semver::VersionReq)>> {
let req_text = req.to_string();
let raw_req = semver::VersionReq::parse(&req_text)
.expect("semver to generate valid version requirements");
Expand Down Expand Up @@ -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)))
}
}
}
Expand Down Expand Up @@ -103,7 +103,9 @@ mod test {
#[track_caller]
fn assert_req_bump<'a, O: Into<Option<&'a str>>>(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);
}
Expand Down
11 changes: 4 additions & 7 deletions tests/testsuite/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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" }
"#]],
);

Expand All @@ -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]
Expand All @@ -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"
"#]],
);
}

0 comments on commit f41bdc1

Please sign in to comment.