Skip to content
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

Fix docs for cargo publish --dry-run vs cargo package #14550

Closed
wants to merge 1 commit into from
Closed

Fix docs for cargo publish --dry-run vs cargo package #14550

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/doc/src/reference/publishing.md
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This sounds more like a regression from the development of -Zpackage-workspace. This starts happening since nightly-2024-09-17, which contains the cargo submodule update (rust-lang/rust#130377).

Copy link
Member

Choose a reason for hiding this comment

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

This is it: 6ede1e2 in #14448. The fix might need to consider the presence of --dry-run.

Copy link
Member

Choose a reason for hiding this comment

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

Changing the doc like this PR is also fine, if we consider they are not equivalent anymore.

@epage since you were involved in #14448, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

This moved an error that happens after --dry-run stops to before. I view this as a positive improvement, helping to catch errors earlier.

As for the docs, it depends on what you mean by equivalent. @trevyn in the future, please start with an issue. This might seem like an obvious documentation bug but we need to dig more into your situation to really understand what should be updated, whether docs, something else, etc. In general, there are differences between cargo publish -n and cargo package and we added another (and --workspace has more). So the question is what you read from "equivalent" that you found this to be wrong and needed to be updated.

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 interpret "equivalent" to mean identical in function, which is not the case for the above example. (see https://doc.rust-lang.org/std/cmp/trait.Eq.html 😂)

My use case is to run publish --dry-run in CI, partly as an additional verification, but primarily to report the compressed package size.

I agree that publish --dry-run erroring here is an improvement, and happily switched to package, which works great for me.

I found the docs helpful to find this alternative, but in my mind and usage the two are indeed no longer "equivalent". :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This section of documentation is telling users how to prepare for publish by doing a one-off command,. In that context, cargo publish -n and cargo package are similar.

What command to run in CI to catch problems during development is a different situation that this documentation is not speaking to.

However, I'm not too sure the value of calling out cargo package here. imo that would be the reason would remove the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial reaction too, but this section of the docs was the first thing I found when my --dry-run failed and I was trying to figure out what to do about it, and package was indeed the solution.

Will defer to your judgement of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"which is similar" instead of "which is equivalent"?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, i interpret "equivalent" to mean identical in function,

Agree. They used to be “more equivalent” before -Zpackage-worksapce came out. I also agree that we should remove cargo package entirely from that sentence.

but this section of the docs was the first thing I found when my --dry-run failed and I was trying to figure out what to do about it, and package was indeed the solution.

An alternative to this is that we could expand the doc of cargo publish --dry-run a bit. We could document about it checks what cargo package does plus some extra stuff. If people don't want to talk to remote registries for those extra checks, use cargo package.

Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ steps:
before adding it.

It is recommended that you first run `cargo publish --dry-run` (or [`cargo
package`] which is equivalent) to ensure there aren't any warnings or errors
before publishing. This will perform the first three steps listed above.
package`]) to ensure there aren't any warnings or errors before publishing.
This will perform the first three steps listed above.

```console
$ cargo publish --dry-run
Expand Down