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 deprecate! and disable! to casks #16292

Merged
merged 9 commits into from
Dec 17, 2023
Merged

Add deprecate! and disable! to casks #16292

merged 9 commits into from
Dec 17, 2023

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Dec 4, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR adds deprecate! and disable! DSLs to casks. There already is a way to indicate something similar using caveats, so I've prepped us to deprecate that in favor of using the same deprecate! and disable! DSLs that we use for formulae. To do this, right now using caveats { discontinued } will set deprecated? to true, so I've moved all of the checks for discontinued? to deprecated? or disabled?.

This PR ended up being way larger than expected, so I've made it a draft to gauge people's thoughts and can split it up into bits if needed.

@Rylan12 Rylan12 added the cask Homebrew Cask label Dec 4, 2023
Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

In general, the code looks good. I left a few nits but, of course, feel free to ignore them.

Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/installer.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/installer.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Outdated Show resolved Hide resolved
Comment on lines +333 to +339
"deprecated" => deprecated?,
"deprecation_date" => deprecation_date,
"deprecation_reason" => deprecation_reason,
"disabled" => disabled?,
"disable_date" => disable_date,
"disable_reason" => disable_reason,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like disabled and deprecated are really only necessary on the formula side to aid with HTML generation for formulae.brew.sh. Probably be a good idea to add a follow-up to enable that for casks too.

Copy link
Member

@Bo98 Bo98 Dec 17, 2023

Choose a reason for hiding this comment

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

If we want to slim this, we could easily make the logic on the formule.brew.sh side be deprecation_date <= today.

Copy link
Member

@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.

Looking good so far! Would be good to get this and the relevant deprecations into 4.2.0 if possible.

Library/Homebrew/cask/dsl.rb Show resolved Hide resolved
@@ -289,7 +289,7 @@ def audit_latest_with_auto_updates

sig { params(livecheck_result: T.any(NilClass, T::Boolean, Symbol)).void }
def audit_hosting_with_livecheck(livecheck_result: audit_livecheck_version)
return if cask.discontinued?
return if cask.deprecated? || cask.disabled?
Copy link
Member

@reitermarkus reitermarkus Dec 11, 2023

Choose a reason for hiding this comment

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

I think we also need a new (strict?) audit for usage of caveats :discontinued.

@Rylan12 Rylan12 marked this pull request as ready for review December 17, 2023 03:52
@Rylan12
Copy link
Member Author

Rylan12 commented Dec 17, 2023

Sorry for the delay, I've had exams this week and have been busy.

I've made updates as discussed in the comments. Mainly, I've set it so that the only pre-set reason for casks right now is :discontinued.

I know the goal is to add the deprecations before releasing 4.2.0, but they can't be added until all of the old references in homebrew/cask have been fixed, and those can't be updated until this PR has made it into a new tag. So, unless we want to make a new tag pre-4.2.0 (which seems difficult given the deprecation PRs and Ruby 3 rollout), I think we may have to wait to deprecate in the next cycle. @Bo98 @MikeMcQuaid curious to hear your thoughts.

I'll update formulae.brew.sh and add an audit to switch to deprecate! in a separate PR.

Otherwise, I think this is ready!

@Bo98
Copy link
Member

Bo98 commented Dec 17, 2023

So, unless we want to make a new tag pre-4.2.0 (which seems difficult given the deprecation PRs and Ruby 3 rollout), I think we may have to wait to deprecate in the next cycle.

Yeah the Ruby 3 stuff is now non-revertable, and while a off-branch tag is possible this is probably too large/risky for that.

Co-authored-by: Bo Anderson <[email protected]>
@Bo98
Copy link
Member

Bo98 commented Dec 17, 2023

A follow-up necessary as well would be documentation, as well as a policy for how deprecations should work. I was going to propose discussing a rethink of our deprecation policy for formulae at the AGM so we could also talk about cask stuff there too and decide whether it should be the same or different to the formula policy (any thresholds will probably be different at least).

Copy link
Member

@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 @Rylan12!

@Bo98 if you're OK with this landing as-is for the Cask DSL to work: I'd love to get this into 4.2.0 (tomorrowish) and please feel free to merge. If not, no worries and can wait for a later tag.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 17, 2023

I'm going to go ahead and merge so this can sit on master for a little before 4.2.0 tomorrow, and I'll keep an eye out for issues.

@Rylan12 Rylan12 merged commit 4793677 into Homebrew:master Dec 17, 2023
31 checks passed
@Rylan12 Rylan12 deleted the cask-deprecate-disable branch December 17, 2023 20:03
@Rylan12
Copy link
Member Author

Rylan12 commented Dec 17, 2023

ThatsJustCheesy added a commit to GetOvert/Overt that referenced this pull request Dec 19, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants