Skip to content

Fix Non-actionable error messages #15879

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+187 −14
Draft

Fix Non-actionable error messages #15879

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
50f8ecf
test:Add test demonstrating non-actionable error messages during work…
ojuschugh1 Aug 22, 2025
5c5fef4
fix(add):Improve publish error messages to show failed package and re…
ojuschugh1 Aug 22, 2025
07f44ac
fix:additional packages were on a separate line
ojuschugh1 Aug 22, 2025
e440470
fix:always include the context separator and use an empty string inst…
ojuschugh1 Aug 22, 2025
358df9c
fix: now provides accurate, actionable information to users without …
ojuschugh1 Aug 22, 2025
f4a4804
fix now provides accurate, actionable information to users by only sh…
ojuschugh1 Aug 22, 2025
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
72 changes: 64 additions & 8 deletions src/cargo/ops/registry/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
)?;

let mut plan = PublishPlan::new(&pkg_dep_graph.graph);
// Store the original list of packages to be published for error reporting
let original_packages: BTreeSet<_> = plan.iter().collect();
// May contains packages from previous rounds as `wait_for_any_publish_confirmation` returns
// after it confirms any packages, not all packages, requiring us to handle the rest in the next
// iteration.
Expand All @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let (pkg, (_features, tarball)) = &pkg_dep_graph.packages[&pkg_id];
opts.gctx.shell().status("Uploading", pkg.package_id())?;

Expand All @@ -236,15 +238,52 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
)?));
}

transmit(
// Prepare workspace context for error message
let workspace_context = if original_packages.len() > 1 {
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();
Copy link
Contributor

@epage epage Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

// 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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if !remaining.is_empty() {
Some(format!(
"the following crates have not been published yet: {}",
remaining.join(", ")
))
} else {
None
}
} else {
None
};

let transmit_result = transmit(
opts.gctx,
ws,
pkg,
tarball.file(),
&mut registry,
source_ids.original,
opts.dry_run,
)?;
workspace_context,
);

if let Err(e) = transmit_result {
return Err(e);
}

to_confirm.insert(pkg_id);

if !opts.dry_run {
Expand Down Expand Up @@ -450,12 +489,13 @@ fn verify_unpublished(
source.describe()
))?;
} else {
bail!(
// Return error instead of bail! so it can be wrapped with enhanced context
return Err(anyhow::anyhow!(
"crate {}@{} already exists on {}",
pkg.name(),
pkg.version(),
source.describe()
);
));
}
}

Expand Down Expand Up @@ -632,6 +672,7 @@ fn transmit(
registry: &mut Registry,
registry_id: SourceId,
dry_run: bool,
workspace_context: Option<String>,
) -> CargoResult<()> {
let new_crate = prepare_transmit(gctx, ws, pkg, registry_id)?;

Expand All @@ -641,9 +682,24 @@ fn transmit(
return Ok(());
}

let warnings = registry
.publish(&new_crate, tarball)
.with_context(|| format!("failed to publish to registry at {}", registry.host()))?;
let warnings = registry.publish(&new_crate, tarball).with_context(|| {
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()
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})?;

if !warnings.invalid_categories.is_empty() {
let msg = format!(
Expand Down
18 changes: 9 additions & 9 deletions tests/testsuite/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2260,7 +2260,7 @@ fn api_error_json() {
[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:[..]/
Caused by:
the remote server responded with an error (status 403 Forbidden): you must be logged in
Expand Down Expand Up @@ -2308,7 +2308,7 @@ fn api_error_200() {
[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:[..]/
Caused by:
the remote server responded with an [ERROR] max upload size is 123
Expand Down Expand Up @@ -2356,7 +2356,7 @@ fn api_error_code() {
[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:[..]/
Caused by:
failed to get a 200 OK response, got 400
Expand Down Expand Up @@ -2413,7 +2413,7 @@ fn api_curl_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:[..]/
Caused by:
[52] Server returned nothing (no headers, no data) (Empty reply from server)
Expand Down Expand Up @@ -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:[..]/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in #15765, could you split adding the package name to the error in a commit before adding the unpublished packages?

Caused by:
invalid response body from server
Expand Down Expand Up @@ -3608,7 +3608,7 @@ fn invalid_token() {
[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:[..]/
Caused by:
token contains invalid characters.
Expand Down Expand Up @@ -4484,8 +4484,8 @@ fn workspace_publish_rate_limit_error() {
.file("package_c/src/lib.rs", "")
.build();

// This demonstrates the current non-actionable error message
// The user doesn't know which package failed or what packages remain to be published
// This demonstrates the improved error message after the fix
// The user now knows which package failed and what packages remain to be published
p.cargo("publish --workspace")
.replace_crates_io(registry.index_url())
.with_status(101)
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Caused by:
failed to get a 200 OK response, got 429
Expand Down
Loading