Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More update --breaking tests #14049

Merged
merged 4 commits into from
Jun 27, 2024
Merged

More update --breaking tests #14049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
abc86e2
refactor: Clean up test output and tracing, and introduce variable.
torhovland Jun 24, 2024
8882d05
test: More `update --breaking` tests.
torhovland Jun 12, 2024
f096a94
fix: `update --breaking` now understands package@version.
torhovland Jun 12, 2024
f41bdc1
fix: `update --breaking` now handles mixed renaming and pinning.
torhovland Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 67 additions & 47 deletions src/cargo/ops/cargo_update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Result<Vec<_>, _>>()?;

// 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)?;
Expand All @@ -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,
Expand All @@ -253,7 +258,7 @@ pub fn upgrade_manifests(

fn upgrade_dependency(
gctx: &GlobalContext,
to_update: &Vec<String>,
to_update: &Vec<PackageIdSpec>,
registry: &mut PackageRegistry<'_>,
upgrades: &mut UpgradeMap,
upgrade_messages: &mut HashSet<String>,
Expand All @@ -263,46 +268,48 @@ 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);
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
epage marked this conversation as resolved.
Show resolved Hide resolved
.version()
.map_or(true, |v| dependency.version_req().matches(&v))
})
{
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] = &current.comparators[..] else {
trace!(
"skipping dependency `{}` with multiple version comparators: {:?}",
name,
"skipping dependency `{name}` with multiple version comparators: {:?}",
&current.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);
}

Expand Down Expand Up @@ -332,30 +339,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(&current.to_string(), latest)? else {
trace!(
"skipping dependency `{}` because the version requirement didn't change",
name
);
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);
};

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()) {
Expand All @@ -371,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 All @@ -389,10 +396,7 @@ pub fn write_manifest_upgrades(
.collect::<Vec<_>>();

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()
Expand All @@ -410,41 +414,57 @@ pub fn write_manifest_upgrades(
dep_key_str,
dep_item,
)?;
let name = &dependency.name;
epage marked this conversation as resolved.
Show resolved Hide resolved

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: {}", 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 {
let Some(latest) = upgrades.get(&(name.to_owned(), source_id)) else {
trace!("skipping dependency without an upgrade: {name}");
continue;
};

let Some((new_req_string, new_req)) = upgrade_requirement(current, latest)? else {
trace!(
"skipping dependency without an upgrade: {}",
dependency.name
"skipping dependency `{name}` because the version requirement didn't change"
);
continue;
};

let Some(new_req_string) = upgrade_requirement(current, latest)? else {
let [comparator] = &new_req.comparators[..] else {
trace!(
"skipping dependency `{}` because the version requirement didn't change",
dependency.name
"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;
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;
Expand Down
8 changes: 5 additions & 3 deletions src/cargo/util/toml_mut/upgrade.rs
View file
Open in desktop
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
Loading