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.

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

Clarify Implications of Cargo Yank #11071

Closed
wants to merge 7 commits into from
Closed

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Sep 10, 2022

What does this PR try to resolve?

I found the documentation for cargo yank was not especially clear on the implications of yanking a crate, and I have seen this causing confusion within the community - tafia/quick-xml#475.

On a somewhat related note, I have been observing lots more crates getting yanked recently and this is resulting in a fair amount of dependency upgrade busywork. I think/hope part of this is a documentation issue.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2022
@tustvold tustvold changed the title Update cargo-yank.md Clarify Implications of Cargo Yank Sep 10, 2022
@Mingun
Copy link

Mingun commented Sep 10, 2022

I think it would be useful to add an example here

@adamreichold
Copy link

If the implications are discussed, I think mentioning the usual course of action (publishing a semver-compatible release before yanking) would be nice as well.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

The first sentence is too long for me to get it at the first glimpse. Is it possible to make it simpler?

Besides, I believe we should follow the step here to generate docs.

Comment on lines 18 to 23
However, yanking a release will prevent cargo from selecting that version
when determining the version of a dependency to use. If there are no longer
any compatible versions that haven't been yanked, cargo will return an error.

The only exception to this is crates locked to a specific version by a lockfile,
these will still be able to download the yanked version to use it.
Copy link

Choose a reason for hiding this comment

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

Suggested change
However, yanking a release will prevent cargo from selecting that version
when determining the version of a dependency to use. If there are no longer
any compatible versions that haven't been yanked, cargo will return an error.
The only exception to this is crates locked to a specific version by a lockfile,
these will still be able to download the yanked version to use it.
Best practice is to yank a release *only* when it contains a serious flaw, such as
a security vulnerability, and when there is a newer semver compatible release
published. You do not need to yank a version to suggest users of your crate upgrade.
Cargo will not use a yanked version for any new project or checkout without a
pre existing lockfile, and will generate an error if there are no longer
any compatible versions for your crate.
For example, the `foo` crate published version `0.22.0` and another crate `bar`
declared a dependency on version `foo = 0.22`. Now `foo` releases a new, but
not semver compatibile, version `0.23.0`, and finds a critical issue with `0.22.0`.
If `0.22.0` is yanked, no new project or checkout without an existing lockfile will be
able to use crate `bar` as it relies on `0.22`
In this case, the maintainers of `foo` should first publish a semver compatible version
such as `0.22.1` prior to yanking `0.22.0` so that `bar` and all projects that depend
on `bar` will continue to work

Copy link

Choose a reason for hiding this comment

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

Cargo will not use a yanked version for any new project

I think, that the concept of "new" requires clarification. When project is considered as "new"?

Copy link

Choose a reason for hiding this comment

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

Practically speaking, I think "new" means any rust project that does not already have a Cargo.lock file.

I believe the basic guidance is that "libraries" (e.g. stuff published to crates.io that other applications can depend on) should not have a Cargo.lock: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

Choose a reason for hiding this comment

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

such as a security vulnerability

I think accidentally breaking semver compatibility might deserve mention there as well it can wreak havoc in downstream projects.

Copy link

Choose a reason for hiding this comment

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

Best practice is to yank a release only when it contains a serious flaw, such as
a security vulnerability, and when there is a newer semver compatible release
published.

Even for security flaws: publish a new patch version and if you're really concerned, file a rustsec warning. No yanking required. The downstream user can then decide if they are even affected (e.g. for flaws that only affect certain platforms) and -- depending on how critical the flaw is -- update in a cadence that matches THEIR workflow. Remember: yanking a crate is a choice that the downstream user CANNOT make.

A good example that justifies yanking are license/copyright issues (e.g. you accidentally included code that cannot be used under the crate's license; or you've included personally identifiable information in some tests). Apart from that, I cannot think of a good reason to yank a crate. There's semver and patch releases provide a clear path forward and are THE tool the use in nearly every case.

Copy link

Choose a reason for hiding this comment

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

Probably that explanation also should appear when you try to click "Yank" button on crates.io or run cargo yank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a slightly tweaked version of this, PTAL

src/doc/man/cargo-yank.md Outdated Show resolved Hide resolved
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thank you for posting this. Looks nice to me so far!

src/doc/man/cargo-yank.md Outdated Show resolved Hide resolved
src/doc/man/cargo-yank.md Outdated Show resolved Hide resolved
src/doc/man/cargo-yank.md Outdated Show resolved Hide resolved
@weihanglo weihanglo added A-documenting-cargo-itself Area: Cargo's documentation Command-yank labels Sep 13, 2022
@ehuss
Copy link
Contributor

ehuss commented Sep 26, 2022

r? @weihanglo

@rust-highfive rust-highfive assigned weihanglo and unassigned ehuss Sep 26, 2022
Comment on lines +18 to +22
Crates should only be yanked in exceptional circumstances, for example, license/copyright issues, accidental
inclusion of [PII](https://en.wikipedia.org/wiki/Personal_data), credentials, etc... In the case of security
vulnerabilities, [RustSec](https://rustsec.org/) is typically a less disruptive mechanism to inform users
and encourage them to upgrade, and avoids the possibility of significant downstream disruption irrespective
of susceptibility to the vulnerability in question.
Copy link
Member

@weihanglo weihanglo Sep 27, 2022

Choose a reason for hiding this comment

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

How do you feel putting them in the end of this DESCRIPTION section, under a subsection heading like ### When to yank or something better?

Just trying to make it look less lengthy and more organized. The content itself is really well-written! Thank you!

Comment on lines +34 to +36
In this case, the maintainers of `foo` should first publish a semver compatible version
such as `0.22.1` prior to yanking `0.22.0` so that `bar` and all projects that depend
on `bar` will continue to work.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph concludes the example really well, thought it feels like somewhat a duplicate of line 49-50. A challenge in documentation is putting the right amount. Could we find a way to merge them and make it more concise? It is not a blocker on merging this PR though.

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2022
@weihanglo
Copy link
Member

Friendly ping @tustvold. Just checking in to see if you are still interested in working on this, or if you had any questions. Thank you :)

@weihanglo
Copy link
Member

This PR is indeed valuable, and I want it to move forward!!
I posted a PR #11862 with only moving paragraphs around. Thanks everyone for participating the discussion and mostly @tustvold for writing all these down. Close in favor of #11862.

@weihanglo weihanglo closed this Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation Command-yank S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants