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.

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
Open
Changes from 1 commit
Commits
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
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
Loading