Skip to content

Fix Non-actionable error messages #15799

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+195 −14
Open

Fix Non-actionable error messages #15799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61f9fcb
tests(publish): add new test case for publish
ojuschugh1 Aug 1, 2025
93c3513
refactor: clean up publish error context and remove unused variables
ojuschugh1 Aug 1, 2025
8ccc0e2
publish: report failing crate and remaining unpublished on workspace …
ojuschugh1 Aug 21, 2025
051a16a
feat: publish: merge workspace context into single error message
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
48 changes: 43 additions & 5 deletions src/cargo/ops/registry/publish.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

refactor: clean up publish error context and remove unused variables

Note that a "refactor" means a change to the code with no user-visible impact. If you wish to use Conventional Commits, this would be better term a fix

Also, the commit title talks about cleanup and removing a variable but the commit is about adding extra context to error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this commit title , first i wanted to complete the implementation in this pr, I will open a new pr with proper final fixes and commits. So that the git history should be clean

Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,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() {
let (pkg, (_features, tarball)) = &pkg_dep_graph.packages[&pkg_id];
opts.gctx.shell().status("Uploading", pkg.package_id())?;

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

transmit(
// Always wrap transmit with context for enhanced error reporting
let transmit_result = transmit(
opts.gctx,
ws,
pkg,
tarball.file(),
&mut registry,
source_ids.original,
opts.dry_run,
)?;
).map_err(|e| {
// Collect remaining packages that have not been published yet
let mut remaining: Vec<_> = plan
.iter()
.filter(|id| *id != pkg_id)
.map(|id| {
let pkg = &pkg_dep_graph.packages[&id].0;
format!("{} v{}", pkg.name(), pkg.version())
})
.collect();
// 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);
}
}

let message = if !remaining.is_empty() {
format!(
"failed to publish `{}` v{}; the following crates have not been published yet: {}",
pkg.name(),
pkg.version(),
remaining.join(", ")
)
} else {
format!("failed to publish `{}` v{}", pkg.name(), pkg.version())
};

e.context(message)
});

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

to_confirm.insert(pkg_id);

if !opts.dry_run {
Expand Down Expand Up @@ -450,12 +487,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
82 changes: 76 additions & 6 deletions tests/testsuite/publish.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Same feedback as #15791 (comment)

This commit should be able to pass as-is but it can't because its including test changes for the following commit

Original file line number Diff line number Diff line change
Expand Up @@ -2261,7 +2261,10 @@ 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
Caused by:
failed to publish 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 @@ -2309,7 +2312,10 @@ 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
Caused by:
failed to publish 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 @@ -2357,7 +2363,10 @@ 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
Caused by:
failed to publish 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.

#15791 (comment) is not resolved

Why are we reporting two failed to publish messages? Shouldn't we just put all of this information in the one failed to publish message?

Copy link
Contributor

Choose a reason for hiding this comment

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

From @ojuschugh1 at https://github.com/rust-lang/cargo/pull/15799/files#r2252608902

Hi @epage so regarding this https://github.com/rust-lang/cargo/pull/15799/files#r2252499850 , I found out that already in some places like here there are more than one failed to publish messages so i thought to keep like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify what you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi , @epage We show two lines because Cargo prints the error chain: the top-level is a workspace-specific summary (which crate failed + what’s left), and the “Caused by” line is the transport/server detail. This keeps it readable and preserves single-crate behavior. If you prefer one line, I can collapse it and update tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we extend the original error to include the context? Seems like that would provide much better results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will try to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epage , I have fixed this. Kindly please review and let me know, if anything else you want me to do.

Caused by:
failed to get a 200 OK response, got 400
Expand Down Expand Up @@ -2414,7 +2423,10 @@ 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
Caused by:
failed to publish 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 @@ -2462,7 +2474,10 @@ 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
Caused by:
failed to publish to registry at http://127.0.0.1:[..]/
Caused by:
invalid response body from server
Expand Down Expand Up @@ -3609,7 +3624,10 @@ 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
Caused by:
failed to publish to registry at http://127.0.0.1:[..]/
Caused by:
token contains invalid characters.
Expand Down Expand Up @@ -4330,6 +4348,58 @@ fn all_unpublishable_packages() {
.run();
}

#[cargo_test]
fn workspace_publish_failure_reports_remaining_packages() {
use cargo_test_support::project;
let p = project()
.file(
"Cargo.toml",
r#"
[workspace]
members = ["a", "b", "c"]
"#,
)
.file(
"a/Cargo.toml",
r#"
[package]
name = "a"
version = "0.1.0"
edition = "2021"
"#,
)
.file("a/src/lib.rs", "")
.file(
"b/Cargo.toml",
r#"
[package]
name = "b"
version = "0.1.0"
edition = "2021"
"#,
)
.file("b/src/lib.rs", "")
.file(
"c/Cargo.toml",
r#"
[package]
name = "c"
version = "0.1.0"
edition = "2021"
"#,
)
.file("c/src/lib.rs", "")
.build();

// Simulate a publish failure for package 'a' (e.g., already published)
// The error message should match the actual output from the verify step
p.cargo("publish --workspace")
.env("CARGO_REGISTRY_TOKEN", "dummy-token")
.with_status(101)
.with_stderr_contains("[ERROR] crate a@0.1.0 already exists on crates.io index")
.run();
}

#[cargo_test]
fn checksum_changed() {
let registry = RegistryBuilder::new().http_api().http_index().build();
Expand Down
Loading