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

dev-cmd/livecheck: Skip autobumped formulae #18984

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Dec 22, 2024

  • 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?

- Skip formulae that are autobumped by BrewTestBot,
  to avoid useless effort spent by contributors who
  are checking for formulae to bump and then we close
  their PRs.
@p-linnane p-linnane requested a review from samford December 22, 2024 02:36
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

As this is currently implemented, users don't have a way to disable this filtering behavior other than manually modifying this code. livecheck blocks for autobumped formulae/casks regularly break, so we need a way to be able to run these checks locally for debugging/fixing. A CLI option to also check autobumped packages (e.g., --autobump) would be a simple addition.

Beyond that, there may be something to be said for also offering an environment variable that would act like --autobump is always used (e.g., HOMEBREW_LIVECHECK_AUTOBUMP). My situation may be a bit rare (since I do a lot of livecheck block maintenance) but I almost never want to skip autobumped packages, so I would greatly prefer to not have to add a CLI option a lot of the time (and having to completely re-do long runs if I forget).

@@ -14,7 +14,7 @@ class LivecheckCmd < AbstractCommand
Check for newer versions of formulae and/or casks from upstream.
If no formula or cask argument is passed, the list of formulae and
casks to check is taken from `HOMEBREW_LIVECHECK_WATCHLIST` or
`~/.homebrew/livecheck_watchlist.txt`.
`~/.homebrew/livecheck_watchlist.txt`, excluding autobumped formulae.
Copy link
Member

@samford samford Dec 22, 2024

Choose a reason for hiding this comment

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

If we add a way to disable this filtering behavior, this statement may not always be true. It may be better to simply leave out this added language (i.e., overarching assumptions about what livecheck will/won't check apply 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.

Suggested change
`~/.homebrew/livecheck_watchlist.txt`, excluding autobumped formulae.
`~/.homebrew/livecheck_watchlist.txt`.


formulae_and_casks_to_check = formulae_and_casks_to_check.reject do |formula_or_cask|
name = formula_or_cask.respond_to?(:token) ? formula_or_cask.token : formula_or_cask.name
if (autobump_core + autobump_cask).include?(name)
Copy link
Member

@samford samford Dec 22, 2024

Choose a reason for hiding this comment

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

I don't think this logic will work as intended if a formula and cask both share the same name and the formula/cask is only in one of the autobump lists. For example, if the cask is in its autobump list but the formula isn't, the formula would still be skipped like the cask. Instead, we should only be checking the autobump file from the formula_or_cask tap.

I could be wrong but it may be technically possible for a tap other than core/cask to have an autobump.txt file and it seems like we would want to respect that in the same way. Either way, we could take a more generic approach instead of special-casing first-party taps. You may be able to borrow some ideas from Livecheck::load_other_tap_strategies (omitting the logic to skip first-party taps, of course).

The basic idea is to identify the taps that are in use for formulae_and_casks_to_check and then gather the autobump files, mapping them to the tap (as before, only doing this once before filtering packages).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks for reminding me of packages with the same name in both core and cask!

Comment on lines +95 to +96
autobump_core = File.read(autobump_core_path).lines.map(&:strip)
autobump_cask = File.read(autobump_cask_path).lines.map(&:strip)
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
autobump_core = File.read(autobump_core_path).lines.map(&:strip)
autobump_cask = File.read(autobump_cask_path).lines.map(&:strip)
autobump_core = autobump_core_path.read.lines.map(&:strip)
autobump_cask = autobump_cask_path.read.lines.map(&:strip)

or .readlines may make sense


sig { returns(Pathname) }
def autobump_core_path
@autobump_core_path ||= T.let(Tap.fetch("homebrew/core").path/".github/autobump.txt", T.nilable(Pathname))
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
@autobump_core_path ||= T.let(Tap.fetch("homebrew/core").path/".github/autobump.txt", T.nilable(Pathname))
@autobump_core_path ||= T.let(Tap.fetch("homebrew/core").path/".github/autobump.txt", Pathname)

maybe?

@issyl0
Copy link
Member Author

issyl0 commented Dec 23, 2024

OK, thanks @samford! I’ll add HOMEBREW_LIVECHECK_AUTOBUMP and an —-autobump flag or something similarly named.

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.

option to remove BrewTestBot checked formulae from brew livecheck output
6 participants