-
Notifications
You must be signed in to change notification settings - Fork 2.7k
update: silent failure on non-matching package specs with --breaking #16276
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,6 +238,9 @@ pub fn upgrade_manifests( | |
| let mut registry = ws.package_registry()?; | ||
| registry.lock_patches(); | ||
|
|
||
| // Track which specs have been matched to detect non-existent packages | ||
| let mut matched_specs = HashSet::new(); | ||
|
|
||
| for member in ws.members_mut().sorted() { | ||
| debug!("upgrading manifest for `{}`", member.name()); | ||
|
|
||
|
|
@@ -252,11 +255,38 @@ pub fn upgrade_manifests( | |
| &mut registry, | ||
| &mut upgrades, | ||
| &mut upgrade_messages, | ||
| &mut matched_specs, | ||
| d, | ||
| ) | ||
| })?; | ||
| } | ||
|
|
||
| // Check if any specs were not matched against direct dependencies | ||
| if !to_update.is_empty() { | ||
| // Load the lockfile to check for transitive dependencies | ||
| let previous_resolve = ops::load_pkg_lockfile(ws)?; | ||
|
|
||
| for spec in &to_update { | ||
| if !matched_specs.contains(spec) { | ||
|
Comment on lines
+264
to
+270
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this could be made simpler by replacing |
||
| // Spec didn't match any direct dependencies | ||
| // Check if it matches any package in the lockfile (including transitive deps) | ||
| let matches_lockfile = if let Some(ref resolve) = previous_resolve { | ||
| spec.query(resolve.iter()).is_ok() | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if !matches_lockfile { | ||
|
Comment on lines
+271
to
+279
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we skipping these? They look to be unused otherwise If we do error, we could leverage the logic you wrote to help inform the user that it does exist but that it isn't selectable |
||
| // Spec doesn't match any package at all | ||
| anyhow::bail!( | ||
| "package ID specification `{}` did not match any packages", | ||
| spec | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(upgrades) | ||
| } | ||
|
|
||
|
|
@@ -266,18 +296,30 @@ fn upgrade_dependency( | |
| registry: &mut PackageRegistry<'_>, | ||
| upgrades: &mut UpgradeMap, | ||
| upgrade_messages: &mut HashSet<String>, | ||
| matched_specs: &mut HashSet<PackageIdSpec>, | ||
| dependency: Dependency, | ||
| ) -> CargoResult<Dependency> { | ||
| let name = dependency.package_name(); | ||
| let renamed_to = dependency.name_in_toml(); | ||
|
|
||
| if !to_update.is_empty() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this check present? If its empty, the loop just won't iterate |
||
| // Check if any spec matches this dependency by name | ||
| for spec in to_update.iter() { | ||
| if spec.name() == name.as_str() { | ||
| // Mark this spec as matched (exists in workspace) | ||
| matched_specs.insert(spec.clone()); | ||
| } | ||
| } | ||
| } | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+306
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this here and not at the very bottom of the function when we actually make the change? This ends up duplicating the |
||
|
|
||
| if name != renamed_to { | ||
| trace!("skipping dependency renamed from `{name}` to `{renamed_to}`"); | ||
| return Ok(dependency); | ||
| } | ||
|
|
||
| if !to_update.is_empty() | ||
| && !to_update.iter().any(|spec| { | ||
| if !to_update.is_empty() { | ||
| // Check if this dependency should be upgraded based on the specs | ||
| let should_upgrade = to_update.iter().any(|spec| { | ||
|
Comment on lines
+320
to
+322
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed? |
||
| spec.name() == name.as_str() | ||
| && dependency.source_id().is_registry() | ||
| && spec | ||
|
|
@@ -286,10 +328,12 @@ fn upgrade_dependency( | |
| && spec | ||
| .version() | ||
| .map_or(true, |v| dependency.version_req().matches(&v)) | ||
| }) | ||
| { | ||
| trace!("skipping dependency `{name}` not selected for upgrading"); | ||
| return Ok(dependency); | ||
| }); | ||
|
|
||
| if !should_upgrade { | ||
| trace!("skipping dependency `{name}` not selected for upgrading"); | ||
| return Ok(dependency); | ||
| } | ||
| } | ||
|
|
||
| if !dependency.source_id().is_registry() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2750,3 +2750,48 @@ Caused by: | |
| "#]]) | ||
| .run(); | ||
| } | ||
|
|
||
| #[cargo_test] | ||
| fn update_breaking_missing_package_error() { | ||
|
Comment on lines
+2754
to
+2755
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something our contrib docs encourage is adding tests in the commit before with them passing, showing the old behavior. Then in this commit the test then gets updated to show the new behavior and the diff between them shows how the behavior changed. This does a good job communicating what a PR is doing (and tests the tests) |
||
| Package::new("bar", "1.0.0").publish(); | ||
|
|
||
| let p = project() | ||
| .file( | ||
| "Cargo.toml", | ||
| r#" | ||
| [package] | ||
| name = "foo" | ||
| version = "0.0.1" | ||
| edition = "2015" | ||
| authors = [] | ||
|
|
||
| [dependencies] | ||
| bar = "1.0" | ||
| "#, | ||
| ) | ||
| .file("src/lib.rs", "") | ||
| .build(); | ||
|
|
||
| p.cargo("generate-lockfile").run(); | ||
|
|
||
| // Test that --breaking reports an error for non-existent packages | ||
| p.cargo("update -Zunstable-options --breaking no_such_crate") | ||
| .masquerade_as_nightly_cargo(&["update-breaking"]) | ||
| .with_status(101) | ||
| .with_stderr_data(str![[r#" | ||
| [ERROR] package ID specification `no_such_crate` did not match any packages | ||
|
|
||
| "#]]) | ||
| .run(); | ||
|
|
||
| // Test with multiple packages, one valid and one invalid | ||
| p.cargo("update -Zunstable-options --breaking bar no_such_crate") | ||
| .masquerade_as_nightly_cargo(&["update-breaking"]) | ||
| .with_status(101) | ||
| .with_stderr_data(str![[r#" | ||
| [UPDATING] `dummy-registry` index | ||
| [ERROR] package ID specification `no_such_crate` did not match any packages | ||
|
|
||
| "#]]) | ||
| .run(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we report these all at once rather than just the first?