Fix Non-actionable error messages #15879
Conversation
…space publish rate limiting
…maining packages in workspace
rustbot
added
A-interacts-with-crates.io
| @@ -2461,7 +2461,7 @@ fn api_other_error() { | |||
| [PACKAGING] foo v0.0.1 ([ROOT]/foo) | |||
| [PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) | |||
| [UPLOADING] foo v0.0.1 ([ROOT]/foo) | |||
| [ERROR] failed to publish to registry at http://127.0.0.1:[..]/ | |||
| [ERROR] failed to publish `foo` v0.0.1 to registry at http://127.0.0.1:[..]/ | |||
There was a problem hiding this comment.
Like in #15765, could you split adding the package name to the error in a commit before adding the unpublished packages?
src/cargo/ops/registry/publish.rs
Outdated
| if let Some(context) = workspace_context { | ||
| format!( | ||
| "failed to publish `{}` v{} to registry at {}; {}", | ||
| pkg.name(), | ||
| pkg.version(), | ||
| registry.host(), | ||
| context | ||
| ) | ||
| } else { | ||
| format!( | ||
| "failed to publish `{}` v{} to registry at {}", | ||
| pkg.name(), | ||
| pkg.version(), | ||
| registry.host() | ||
| ) | ||
| } |
There was a problem hiding this comment.
but we do for a lot of errors is include the separator for optional context in that context and use an empty string instead of None, so we don't need a conditional like this but always include the context.
tests/testsuite/publish.rs
Outdated
| @@ -4507,7 +4507,7 @@ fn workspace_publish_rate_limit_error() { | |||
| [COMPILING] package_c v0.1.0 ([ROOT]/foo/target/package/package_c-0.1.0) | |||
| [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s | |||
| [UPLOADING] package_a v0.1.0 ([ROOT]/foo/package_a) | |||
| [ERROR] failed to publish to registry at http://127.0.0.1:[..]/ | |||
| [ERROR] failed to publish `package_a` v0.1.0 to registry at http://127.0.0.1:[..]/; the following crates have not been published yet: package_b v0.1.0, package_c v0.1.0 | |||
There was a problem hiding this comment.
In #15765, the additional packages were on a separate line
[ERROR] failed to publish package 'a' to registry at http://127.0.0.1:[..]/
Remaining packages to publish: b v0.1.0 and c v0.1.0
Likely what this should look like is
[ERROR] failed to publish `package_a` v0.1.0 to registry at http://127.0.0.1:[..]/
note: the following crates have not been published yet:
package_b v0.1.0
package_c v0.1.0
| @@ -210,7 +212,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { | |||
| // `b`, and we uploaded `a` and `b` but only confirmed `a`, then on | |||
| // the following pass through the outer loop nothing will be ready for | |||
| // upload. | |||
| for pkg_id in plan.take_ready() { | |||
| for pkg_id in plan.take_ready().into_iter() { | |||
There was a problem hiding this comment.
The change from for pkg_id in plan.take_ready() to for pkg_id in plan.take_ready().into_iter() was needed because of a Rust type inference issue.
The take_ready() method returns a BTreeSet<PackageId>, and Rust couldn't automatically determine which iterator implementation to use (there are multiple: IntoIterator, Iter, IterMut). By explicitly calling .into_iter(), we tell Rust to use the consuming iterator, which is what we want since take_ready() takes ownership of the set.
This makes the code more explicit and avoids ambiguity in type inference.
There was a problem hiding this comment.
I just did a checkout of your branch and deleted the into_iter and it built just fine. In fact, for is defined as calling IntoIterator::into_iter
src/cargo/ops/registry/publish.rs
Outdated
| // Also include any packages that are still waiting for confirmation | ||
| for id in to_confirm.iter().filter(|id| **id != pkg_id) { | ||
| let pkg = &pkg_dep_graph.packages[&id].0; | ||
| let entry = format!("{} v{}", pkg.name(), pkg.version()); | ||
| if !remaining.contains(&entry) { | ||
| remaining.push(entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we include these? They have been published, we just don't know if the server has fully finished processing it yet or not.
As is, the current message is misleading. It says a package hasn't been published yet. As a user, that means I need to go in and publish it but then I'll get an error saying its published.
src/cargo/ops/registry/publish.rs
Outdated
| let mut remaining: Vec<_> = original_packages | ||
| .iter() | ||
| .filter(|id| **id != pkg_id) | ||
| .map(|id| { | ||
| let pkg = &pkg_dep_graph.packages[&id].0; | ||
| format!("{} v{}", pkg.name(), pkg.version()) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
If I'm reading this correctly, this is grabbing the list of all packages, including those that have already been published (except for the current package)
|
I don't understand why we have a couple of similar PR open for the same issue. Shouldn't we consolidate all the discussions in a single place? |
…ead of None, eliminating the need for conditional logic
…the confusion of packages that are already uploaded but waiting for server confirmation.
…owing packages that truly need to be published, excluding those that have already been uploaded and are just waiting for server confirmation.
What does this PR try to resolve?
This PR fixes issue #15754 where workspace publish failures provided non-actionable error messages. Previously, when publishing a workspace failed (due to rate limiting or other errors), users only saw generic errors like
[ERROR] failed to publish to registrywithout knowing which package failed or what packages remained to be published.How to test and review this PR?
Testing: Run
cargo test workspace_publish_rate_limit_errorto see the improved behavior, thencargo test publishto ensure all tests pass.Review:
The fix transforms error messages from generic
failed to publish to registryto actionablefailed to publish 'package_a' v0.1.0; the following crates have not been published yet: package_b v0.1.0, package_c v0.1.0. This improvement applies consistently across all publish error scenarios, giving users clear information about what went wrong and what remains to be done.