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

add yanking guidance to the readme faq #102636

Closed
wants to merge 5 commits into from
Closed

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Mar 11, 2024

  • I changed the header levels because in the readme the Q-A text formatting is poorly differentiated
  • Added guidance on yanking (RFC)

README.md Outdated Show resolved Hide resolved
README.md Outdated
1) adding a standard patch release will not resolve the issue i.e. say `v2.10.0` was released using a feature not on julia `v1.6` but the compat entry for julia
was not raised in the release. In this case releasing a `v2.10.1` with the corrected julia compat would not solve the issue as on julia v1.6 Pkg would still
resolve the broken `v2.10.0`, and as a minor bump, reverting the code changes would not be valid in a patch bump.
2) there are security issues with the release
Copy link
Contributor

Choose a reason for hiding this comment

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

I would list this first.
And I would say more along the lines of:

The primary reason yanking exists is to make a release impossible to install.
This is intended to be used for circumstances when having this release installed poses a risk to security or safety.
It is not intended for avoiding people installing releases with bugs, for that use a patch release -- potentially after doing nothing more than revert . All patch releases prior to the most recent have known bugs by definition.

There is however, a special category of bugged releases that can not be resolved by having a patch release. These also need to be resolved by yanking. That special category is when the compat bounds have been set too wide. [Insert your example here]

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good. updated

README.md Outdated Show resolved Hide resolved
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

I do think we should not add more stringent guidelines around yanking. Explanation on the effect of it and what it does is great.

This feels like we are trying to fix a process failure (e.g. Pkg unable to handle yanking) with a guideline change.

README.md Outdated
that Pkg will no longer consider them to exist and not resolve those versions, but that version cannot be re-registed in General.

The primary reason yanking exists is to make a release impossible to install.
This is intended to be used for circumstances when having this release installed poses a risk to security or safety.
Copy link
Member

Choose a reason for hiding this comment

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

How is security or safety defined?

README.md Outdated

The primary reason yanking exists is to make a release impossible to install.
This is intended to be used for circumstances when having this release installed poses a risk to security or safety.
It is not intended for avoiding people installing releases with bugs, for that use a patch release -- potentially after doing nothing more than revert. All patch releases prior to the most recent have known bugs by definition.
Copy link
Member

Choose a reason for hiding this comment

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

While it is true that all patches prior have bugs, the goal of a patch release is to fix a bug. I agree that it would be weird to yank all prior variants of that release.

What should we do with a patch release that introduces a new bug? (And who determines criticalness?) The yank that has caused these recent discussions is was an atomic release (one that only changed one thing) and that introduced (from the kinds of usage the packages sees) a severe bug.

A new patch release reverting the change was made and the version was yanked. Modulo the judgement of the maintainer on severity that seems to be the exact purpose of yanking?

Copy link
Contributor

Choose a reason for hiding this comment

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

What should we do with a patch release that introduces a new bug?

Ask maintainers to make a new patch release.

Modulo the judgement of the maintainer on severity that seems to be the exact purpose of yanking?

A severe package/usage bug is not generally a security vulnerability. A package merely not working correctly is different to that package causing security vulnerabilities down the road for users of that package.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Releases that have already merged can be marked as "yanked" by adding `yanked = true` under the release entry in `Versions.toml`. This will mean
that Pkg will no longer consider them to exist and not resolve those versions, but that version cannot be re-registed in General.

The primary reason yanking exists is to make a release impossible to install. This is intended to be used for circumstances when having this
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The primary reason yanking exists is to make a release impossible to install. This is intended to be used for circumstances when having this
The reason yanking exists is to make a release impossible to install. This is intended to be used for circumstances when having this

Are there any other reasons?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the second reason is because the package manager generally will chose to use an older patch release if that is what is required for compatibility with something else.
Which is a problem if the bug in the older patch release is incorrect compat bounds.

(I have argued that for this reason Pkg shouldn't actually install older patch releases without a special flag, but people disagreed, so yanking is often required when your bug is with compat bounds)

Copy link
Member

Choose a reason for hiding this comment

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

I think of the compat reason as a subset of "make a release impossible to install."

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess the "primary" here refers to the following sentence, not the sentence it is a part of?

Copy link
Member

@LilithHafner LilithHafner Mar 11, 2024

Choose a reason for hiding this comment

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

Suggested change
The primary reason yanking exists is to make a release impossible to install. This is intended to be used for circumstances when having this
The reason yanking exists is to make a release impossible to install. This is primarily intended to be used for circumstances when having this

Copy link
Contributor

Choose a reason for hiding this comment

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

we should add this, plus a footote aon "install" saying

To be precise it will still be possible to install if using the same manifest.
However any new manifest, such as those created by `Pkg.test` or `] up SomeUnrelatedPackage`, or `resolve` after changing julia version, will fail.

This will address @MasonProtter 's comment.

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

I think that these are good guidelines for folks making PRs to yank versions.

For folks deciding whether to merge yank PRs, I think a good guideline is: if the person making the PR has the authority to act on behalf of the package, go ahead and merge it. The only cost to the general registry is the maintenance burden of verifying the authority of the person making the PR, and clicking merge. All other costs are born by the package's users and it's the responsibility of the package author(s) to consider those costs. If they put in the work to make the PR, we can put in the (minimal) work to merge.

Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Sukera <[email protected]>
Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

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

oops, forgot to leave this comment

README.md Outdated
### When is yanking a release appropriate?

Releases that have already merged can be marked as "yanked" by adding `yanked = true` under the release entry in `Versions.toml`. This will mean
that Pkg will no longer consider them to exist and not resolve those versions, but that version cannot be re-registed in General. Yanking should
Copy link
Member

Choose a reason for hiding this comment

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

We should mention that existing Manifests will continue to instantiate, but new manifests won't resolve to this version (important aspect of yanking!)

Copy link
Contributor

This pull request has been inactive for 30 days and will be automatically closed 7 days from now. If this pull request should not be closed, please either (1) fix the AutoMerge issues and re-trigger Registrator, which will automatically update the pull request, or (2) post a comment explaining why you would like this pull request to be manually merged. [noblock]

@github-actions github-actions bot added the stale label Apr 11, 2024
@oxinabox oxinabox removed the stale label Apr 11, 2024
@IanButterworth
Copy link
Member Author

Can I ask that someone take this over please.

@LilithHafner LilithHafner self-assigned this Apr 11, 2024
LilithHafner added a commit to LilithHafner/General that referenced this pull request Apr 12, 2024
Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Sukera <[email protected]>
@LilithHafner
Copy link
Member

Moved to #104800

ericphanson added a commit that referenced this pull request Apr 17, 2024
* Original PR (#102636)

Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Sukera <[email protected]>

* clarify that and why yanked versions can still instantiate

* Major update

* Apply suggestions from code review

Co-authored-by: Eric Hanson <[email protected]>

---------

Co-authored-by: Ian Butterworth <[email protected]>
Co-authored-by: Sukera <[email protected]>
Co-authored-by: Eric Hanson <[email protected]>
@DilumAluthge DilumAluthge deleted the ib/yank_guidance branch May 5, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants