update: silent failure on non-matching package specs with --breaking #16276
Conversation
rustbot
added
Command-update
S-waiting-on-review
src/cargo/ops/cargo_update.rs
Outdated
| // 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()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this here and not at the very bottom of the function when we actually make the change? This ends up duplicating the to_update check and considers it matched even when we won't upgrade it.
There was a problem hiding this comment.
This question seems to still be outstanding
There was a problem hiding this comment.
If I move this to the bottom, specs matching direct dependencies that cannot be upgraded will now error instead of being silently skipped, which will require updating 6 existing tests to expect the new error behavior.
Are we okay with that?
There was a problem hiding this comment.
The question isn't about "are we ok updating tests", particularly with this feature being unstable but "what is the right behavior in these situations?" When should we silently do nothing vs error?
There was a problem hiding this comment.
In my opinion, when a user explicitly requests --breaking foo, silently doing nothing leaves them confused about whether their request was processed. Erroring provides clear feedback about why the upgrade couldn't happen.
There was a problem hiding this comment.
Errors and warnings do. The question then is what should be a blocker. Erroring is a safer route as we are generally free to turn errors into success cases later but not the other way around.
One potential exception is we may not want to error if you run the command two times in a row, ie we should consider being idempotent. That means if there isn't a newer version available or if the version requirement remains unchanged, that might warrant a note (not even a warning).
If we do expand the error to cover the other cases, we should consider whether we should change things up to record why a dependency was skipped (e.g. "can't upgrade a renamed dependency"). This is more complicated and adds more testing in response. For example, what do we do report if a dependency is skipped for different reasons (one workspace member has it pinned, another has it renamed).
There was a problem hiding this comment.
I've noted this in the tracking issue (#12425) so we can revisit this.
charmitro
force-pushed
the
c-update-missing-packages
branch
from
e4ba48c to
cfde2ac
Compare
This comment has been minimized.
This comment has been minimized.
charmitro
force-pushed
the
c-update-missing-packages
branch
from
cfde2ac to
19a788a
Compare
|
@epage thanks for the review. I believe I resolved all. Also added a commit before with preproducing the issue Happy to make more changes if needed. |
charmitro
force-pushed
the
c-update-missing-packages
branch
from
19a788a to
dc73967
Compare
charmitro
force-pushed
the
c-update-missing-packages
branch
from
dc73967 to
314ff8a
Compare
charmitro
force-pushed
the
c-update-missing-packages
branch
from
314ff8a to
3a1d9bf
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
…packages This test demonstrates the current behavior where invalid package specifications passed to `cargo update --breaking` are silently ignored instead of reporting an error. The test covers: 1. Non-existent packages are silently ignored 2. Mix of valid and invalid packages processes only valid ones 3. Transitive dependencies are silently ignored 4. Renamed, non-semver, no-breaking-update dependencies are silently ignored Split the existing test into separate cases for transitive vs renamed/non-semver/no-breaking-update dependencies. A subsequent commit will fix this behavior and update these tests to verify proper error reporting. Signed-off-by: Charalampos Mitrodimas <[email protected]>
When using `cargo update --breaking <spec>`, package specifications that don't match any upgradeable dependency are now properly reported as errors instead of being silently ignored. The implementation tracks which specs match direct dependencies during the upgrade process. After processing all workspace members, it validates that each requested spec either: 1. Matched a direct registry dependency (and was processed), or 2. Exists in the lockfile but cannot be upgraded Specs that match neither category produce clear error messages: - "did not match any packages" for completely non-existent packages - "matched a package... but did not match any direct dependencies" for transitive/non-upgradeable packages, with a note explaining that --breaking can only upgrade direct dependencies Multiple errors are collected and reported together for better UX. This fixes the confusing behavior where users could specify packages that don't exist without receiving any feedback.
charmitro
force-pushed
the
c-update-missing-packages
branch
from
3a1d9bf to
145bec3
Compare
|
Thanks! |
rustbot
removed
the
S-waiting-on-review
3 participants
When using 'cargo update -Zunstable-options --breaking ', package specifications that don't match any dependency in the workspace or lockfile are silently ignored instead of reporting an error. This leads to confusing behavior where users think they're updating a package that doesn't exist.
Fix this by tracking which specs match dependencies during the upgrade process. After processing all workspace members, verify that each requested spec matched at least one direct dependency or exists in the lockfile as a transitive dependency. Report an error for any spec that matches neither.
The fix preserves existing behavior for renamed dependencies and non-registry sources while ensuring proper error reporting for genuinely missing packages.
Closes #16258