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

Update Acceptable/Deprecating/Forks/Versions docs #16606

Merged
merged 9 commits into from Feb 14, 2024

Conversation

MikeMcQuaid
Copy link
Member

As discussed at the AGM:

  • Only allow formula/cask forks to be used when either blessed by the source or used by two other major distributions,
  • Further elaborate and tweak reasons to deprecate/disable formulae.
  • Allow more versioned formulae when widely used.
  • Add a document (based on above) for deprecating/disabling casks.

While we're here:

  • let VSCode Markdown linting remove the trailing ! from an Acceptable Casks heading

docs/Acceptable-Casks.md Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid force-pushed the deprecation-disable-forks-versions-docs branch from bc9da2a to 5a2e8e6 Compare February 7, 2024 17:54
@MikeMcQuaid MikeMcQuaid requested review from a team and bevanjkay February 7, 2024 18:10
@krehel
Copy link
Member

krehel commented Feb 7, 2024

I would like to codify an answer on if we are going to accept archive.org links to Cask downloads for when the original upstream source no longer exists and the file can be obtained.

Historically we've removed the Cask, but it's been put forward as an option recently with the disable/depcreate cycle.

If there are strong reasons from anyone in either direction, would be great to hear.

Personally I am not in favor of this because of lack of upstream support or potentially unknown vulnerability status. I could see how others would want to keep software available, though.

docs/Acceptable-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Versions.md Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member Author

I would like to codify an answer on if we are going to accept archive.org links to Cask downloads for when the original upstream source no longer exists and the file can be obtained.

This seems like a good candidate to change the URL and deprecate rather than immediately remove.

Copy link
Member

@colindean colindean left a comment

Choose a reason for hiding this comment

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

This is good, but I've got some minor suggestions.

docs/Acceptable-Casks.md Outdated Show resolved Hide resolved
docs/Acceptable-Casks.md Outdated Show resolved Hide resolved
docs/Acceptable-Casks.md Show resolved Hide resolved
docs/Acceptable-Formulae.md Outdated Show resolved Hide resolved
docs/Acceptable-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Versions.md Outdated Show resolved Hide resolved
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

I don't want to add too many comments now until some of the comments above are addressed - but it looks good in general, thanks!

docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
As discussed at the AGM:
- Only allow formula/cask forks to be used when either blessed by
  the source or used by two other major distributions,
- Further elaborate and tweak reasons to deprecate/disable formulae.
- Allow more versioned formulae when widely used.
- Add a document (based on above) for deprecating/disabling casks.

While we're here:
- let VSCode Markdown linting remove the trailing `!` from an
  Acceptable Casks heading
@MikeMcQuaid MikeMcQuaid force-pushed the deprecation-disable-forks-versions-docs branch from 121ddb0 to 63e1c54 Compare February 13, 2024 10:13
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Absolutely fantastic work here! This is all much clearer than before.

docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
docs/Deprecating-Disabling-and-Removing-Formulae.md Outdated Show resolved Hide resolved
@@ -42,16 +44,17 @@ A formula should be disabled to indicate to users that the formula cannot be use

The most common reasons for disabling a formula are:

- it cannot be built from source (meaning no bottles can be built)
- it cannot be built from source (meaning no new bottles can be built)
- it has been deprecated for a long time
- the upstream repository has been removed
- the project has no license
Copy link
Member

Choose a reason for hiding this comment

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

Not a suggestion to change, but just to discuss:

Do we still want to treat this differently to the "non-open-source license" case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so as there's often ambiguity here.

@@ -65,21 +68,23 @@ The `because` parameter can be a preset reason (using a symbol) or a custom reas

A formula should be removed if it does not meet our criteria for [acceptable formulae](Acceptable-Formulae.md) or [versioned formulae](Versions.md), has a non-open-source license, or has been disabled for over a year.

**Note: disabled formulae in `homebrew/core` will be automatically removed one year after their disable date.**
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on making this 6 months? That would bring the total deprecation + disable cycle to 1 year for popular formulae.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable but: until we've update the code accordingly the docs should probably be kept correct?

Copy link
Member

@Bo98 Bo98 Feb 14, 2024

Choose a reason for hiding this comment

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

This seems like a good PR to discuss whether there's an agreement to do so however.

I perhaps pointed out the wrong line here - there's earlier lines saying that disabled formulae can't be removed even manually before the 1 year is up (except for unpopular formulae which is 3 months).

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a good PR to discuss whether there's an agreement to do so however.

I'd love to get this PR shipped sooner rather than later once it's correct and then we can iterate further on future PRs.

I perhaps pointed out the wrong line here - there's earlier lines saying that disabled formulae can't be removed even manually before the 1 year is up (except for unpopular formulae which is 3 months).

Could you suggest a wording change?

Copy link
Member

@Bo98 Bo98 Feb 14, 2024

Choose a reason for hiding this comment

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

Could you suggest a wording change?

It's partially on unchanged lines, and GitHub doesn't support that.

But it's a s/a year/six months/ if agreed on.

Copy link
Member Author

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks for review @Bo98! Just to be clear: I expect some code changes to follow fairly quickly from this and the documentation should be updated to reflect what the code cannot handle that.

docs/Deprecating-Disabling-and-Removing-Casks.md Outdated Show resolved Hide resolved
@@ -42,16 +44,17 @@ A formula should be disabled to indicate to users that the formula cannot be use

The most common reasons for disabling a formula are:

- it cannot be built from source (meaning no bottles can be built)
- it cannot be built from source (meaning no new bottles can be built)
- it has been deprecated for a long time
- the upstream repository has been removed
- the project has no license
Copy link
Member Author

Choose a reason for hiding this comment

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

I think so as there's often ambiguity here.

@@ -65,21 +68,23 @@ The `because` parameter can be a preset reason (using a symbol) or a custom reas

A formula should be removed if it does not meet our criteria for [acceptable formulae](Acceptable-Formulae.md) or [versioned formulae](Versions.md), has a non-open-source license, or has been disabled for over a year.

**Note: disabled formulae in `homebrew/core` will be automatically removed one year after their disable date.**
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable but: until we've update the code accordingly the docs should probably be kept correct?

Co-authored-by: Bo Anderson <mail@boanderson.me>
docs/Acceptable-Casks.md Outdated Show resolved Hide resolved
MikeMcQuaid and others added 4 commits February 14, 2024 17:11
Co-authored-by: Bo Anderson <mail@boanderson.me>
Co-authored-by: Michael Cho <michael@michaelcho.dev>
Co-authored-by: Michael Cho <michael@michaelcho.dev>
Co-authored-by: Bo Anderson <mail@boanderson.me>
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Proposing we get this merged. We are 130+ comments deep on this one and a lot of the substantial stuff has been addressed. Anyone that feels strongly about additional changes can open a follow up PR.

@Bo98
Copy link
Member

Bo98 commented Feb 14, 2024

Looks OK overall otherwise

docs/Acceptable-Formulae.md Outdated Show resolved Hide resolved
MikeMcQuaid and others added 3 commits February 14, 2024 19:16
Co-authored-by: Michael Cho <michael@michaelcho.dev>
Co-authored-by: Michael Cho <michael@michaelcho.dev>
Co-authored-by: Bo Anderson <mail@boanderson.me>
@MikeMcQuaid
Copy link
Member Author

Proposing we get this merged. We are 130+ comments deep on this one and a lot of the substantial stuff has been addressed. Anyone that feels strongly about additional changes can open a follow up PR.

This makes sense to me, thanks @p-linnane and to everyone else who reviewed and offered suggestions.

@MikeMcQuaid MikeMcQuaid merged commit 9475258 into master Feb 14, 2024
24 checks passed
@MikeMcQuaid MikeMcQuaid deleted the deprecation-disable-forks-versions-docs branch February 14, 2024 19:29
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet