feat: use API for GitHub latest release strategy

[SMillerDev]

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

---

Part of #15260
This is just the API switch to make sure we don't have to scrape HTML anymore.

[samford]

I ran out of time today but I'll come back to this tomorrow, as there are some areas that will need to be addressed. Off hand, switching to the API will break some existing GithubLatest checks but I'll do comparative runs to find out how many (and which/why).

[MikeMcQuaid]

I'll do comparative runs to find out how many (and which/why).

@samford Thanks for doing these! For the bus factor of the project: can you try and get whatever scripts you're using to do this into a brew command so that they can be run by Not Just You?

[SMillerDev]

I saw a couple that failed locally already because they strictly check for a href. Easy to fix those afterwards though once this is done, livechecks being broken only affect a small subset of people.

[samford]

For the bus factor of the project: can you try and get whatever scripts you're using to do this into a brew command so that they can be run by Not Just You?

For what it's worth, when I do comparative runs there isn't any anything special about the process that precludes anyone from doing the same (i.e., no scripts or arcane knowledge). I've explained the process here and there over the years (Nanda picked it up during GSoC) but it's essentially just diffing multiple brew livecheck --json --verbose runs and looking for meaningful changes:

1. With my local brew repository on the master branch, I run brew livecheck --json --verbose with any affected formulae/taps and redirect the output to a JSON file (e.g., > 01.json).
2. Once that completes, I apply the changes I want to test (e.g., switching to a brew PR branch, etc.) and do the same run again but forward the output to a different JSON file (e.g., > 02.json). [It's best to do these two runs as close together (chronologically) as possible, to minimize upstream version changes (i.e., false positives that have to be verified on the master branch afterward).]
3. Diff the two JSON files however you like (I use Kaleidoscope), scroll through the differences, and keep track of any notable changes.

It's typically a manual process (as above) but, sometimes when I'm looking for specific changes, I might pop into irb, load the two JSON files, and do some comparisons programmatically. In those situations, it's usually just one-off, contextually-specific code that isn't worth saving in a script. It's unexciting work overall but it's necessary to be able to verify changes to livecheck's behavior (mostly related to strategies), catch bugs, identify breakages, etc.

It also may go without saying but it's important to make sure that any changes work properly before doing a JSON livecheck run, since the output isn't visible until the end. For example, I did some manual testing before doing the comparative run on this PR branch and I had to incorporate some fixes to make this work as intended. Even with that, there were some other errors that we'll need to address (but it won't require a full re-run in this case).

---

As this PR currently stands, it will break ~104 existing checks out of 650 (across both formulae and casks).

Most of these fail because they use a livecheck block with a modified version of the existing GithubLatest default regex, which is intended for an HTML context. Fixing these should be pretty simple, though.

There are a few that fail with an error like undefined method `scan' for #<Array:0x00000001098b7f90> (also some for match), which will require some changes to the strategy's logic. I'll look into the issues and suggest changes as part of review.

I've added lists below for reference (I'll try to update this later to use the full name where there are cask name conflicts):

Working -> Different version

List

obs-websocket (5.0.1 -> 4.9.1; newest version is 5.0.1; "latest" release is 4.9.1 but description contains a link to 5.0.1 that the existing check matches; need to check all recent releases to identify the highest version)

Working -> Unable to get versions

List

accord
appium
atari800
augeas
autopsy
azure-cli
bonitastudiocommunity
btcpayserver-vault
camera-live
cataclysm
chamber
chrysalis
clang-format
clojurescript
elk
firefly
flowgrind
genders
gnupg-pkcs11-scd
gopls
gperftools
heimdal
hydrus-network
icarus-verilog
irrtoolset
javacc
josm
jsonschema2pojo
jupyterlab
koka
kona
lbry
libuninameslist
mambaforge
mame
manyverse
mbedtls
miniforge
mp3unicode
mydumper
n1ghtshade
nushell
only-switch
onlyoffice
openimageio
openmsx
openrocket
outline-manager
pingnoo
pkcs11-helper
plover
proftpd
qldds
redpen
ricochet
ricochet-refresh
sapling
sdl2
sdl2_image
sdl2_mixer
sleuthkit
swift
synergy-core
time-to-leave
tmux
turtl
ubports-installer
utm
uuu
vcpkg
vespa-cli
vgmstream
vidcutter
vimediamanager
vlmcsd
wal2json
waterfox-classic
webots
wla-dx
wolfssl
yaws
z3
zesarux

Working -> undefined method `scan' for #<Array:0x00000001098b7f90>

List

arabica
desmume
expat
flame
gauche
gnustep-make
minio
minio-mc
processing
processing3
semeru-jdk11-open
semeru-jdk8-open
temurin19
vine-server
wezterm

Working -> undefined method `match' for #<Array:0x000000010903be70>

List

fvim
home-assistant
semeru-jdk17-open
sequel-ace

Working -> Not Found

List

extraterm

[MikeMcQuaid]

Thanks @samford!

It's typically a manual process (as above) but, sometimes when I'm looking for specific changes, I might pop into irb, load the two JSON files, and do some comparisons programmatically.

Turning this into a script or documentation would be very valuable!

[SMillerDev]

Since there were no real complaints I enabled automerge

[samford]

I'll work on reviewing this today, as there are some changes that I would like to see before this is merged.

[samford]

I didn't have as much time today as I wanted, so I'm still working on code changes (hoping to get them out Wednesday but it may be Thursday). In the interim time, one of the things we need to address before merging this or the GithubRelease PR is how to handle the GitHub token situation. As mentioned before, the limit without a token is 50 requests per hour (5,000 per hour with a token). At present, there are ~279 GithubLatest livecheck blocks in homebrew/core and ~360 in homebrew/cask, so a token is necessary to be able to complete a full tap run of either.

There was a previous idea in the other PR to simply fail if HOMEBREW_GITHUB_API_TOKEN isn't set but I think we can be a little more sophisticated about it. Naively failing fast if HOMEBREW_GITHUB_API_TOKEN isn't set is very easy but it would be a bad experience for some (e.g., first time livecheck users, those who use livecheck sparingly, those who use it in a way that doesn't involve GitHub API requests, etc.).

Warning on larger runs makes sense, as it would allow some basic livecheck usage without a token. We're going to need to keep track of the rate limit response headers and adapt livecheck's behavior to it anyway (i.e., to skip API checks [that are guaranteed to fail] between when the rate limit is exhausted and reset), so assumedly we would display an error when a check fails due to the rate limit. The error message would inform users doing smaller runs without preventing them from using livecheck, so that may be a good balance. [The error message would need to be short, so we may need to add something to the documentation that we can link to (that explains how to resolve the issue, similar to how our manpage explains HOMEBREW_GITHUB_API_TOKEN).]

Extending that idea, we could identify the number of formulae/casks in a run that will use the GitHub API and fail if it's at or above a simple threshold (e.g., 50 without a token, 5,000 with). This wouldn't factor in how much of the rate limit is left (that would require a request to https://api.github.com/rate_limit) but that's probably fine under normal circumstances. We could potentially check the rate limit for large runs (e.g., if there are more than 1,000 API checks in a run) to make sure API checks won't fail in the middle of the run but we can save that for later (i.e., it mostly applies to maintainers and enthusiastic contributors).

In general, we need to keep in mind that livecheck is also used in other parts of Homebrew (brew bump, cask audits, test-bot, etc.), so it's worth looking into how this issue may affect those areas as well. For example, if livecheck uses the same token on CI that's used for making API requests that are necessary for CI to function, then that may push us towards exhausting the rate limit and causing CI to fail. [If possible, it would be a good idea to use a separate token for livecheck on CI to avoid that possibility.]

[SMillerDev]

Yeah nah, I don't want to rewrite the way brew deals with rate limits in case someone doesn't have a token set. If we get actual complaints we can always revisit it, but you're currently proposing a shitton of work in unrelated parts of brew for the benefit of avoiding something that will make the brew UX better for users if they do it. I'd rather drop the change request.

[MikeMcQuaid]

Yeah nah, I don't want to rewrite the way brew deals with rate limits in case someone doesn't have a token set.

Agreed.

If we get actual complaints we can always revisit it, but you're currently proposing a shitton of work in unrelated parts of brew for the benefit of avoiding something that will make the brew UX better for users if they do it.

Agreed.

I'd rather drop the change request.

Please don't. @samford I'd love you to show a little more flexibility here. I think what you're asking of @SMillerDev here is a little unreasonable. I'm also in the camp of "let's get to something with no known problems and, if we hit rate limit issues in CI, let's revert or fix forward" rather than blocking this PR on problems that may not actually manifest.

[samford]

I looked at GitHub::API and saw that it contains RateLimitExceededError, so I ran livecheck without a token and we do get a RateLimitExceededError when we hit the rate limit (I initially assumed that livecheck would fail with an opaque Unable to get versions error without any indication of why). Due to this, we automatically get an okay error message (though the token doesn't need any special permissions for livecheck purposes):

Error: firefly: GitHub API Error: API rate limit exceeded for XXX.XXX.XXX.XXX. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
Try again in 58 minutes 30 seconds, or:
Create a GitHub personal access token:
https://github.com/settings/tokens/new?scopes=gist,repo,workflow&description=Homebrew
echo 'export HOMEBREW_GITHUB_API_TOKEN=your_token_here' >> ~/.zshrc

This is fine for --debug runs but we need something brief for normal and --json runs (e.g., "GitHub API rate limit exceeded"). This would probably involve catching the error somewhere and overriding the message for non-debug runs. I can work on this and either push a commit or add a suggestion to handle it.

---

Of the previously-mentioned requests, the only one I don't think we can reasonably defer is preventing GitHub API requests once the rate limit is exhausted. This is basic behavior when using any API with a rate limit and the related GitHub documentation specifically states, "If you are rate limited, you should not try your request until after the time specified by the x-ratelimit-reset time."

I haven't looked into how we may address that, so I'm not sure whether we can modify the GitHub class to keep track of the rate limit reset time or if we need to handle this in livecheck.

---

I'll have some time to work on reviewing the changes tomorrow (Thursday). However, before I dig further into reviewing this, it's worth mentioning that the GitHub API isn't the only way of moving GithubLatest away from matching tag URLs in the HTML content of the "latest" page.

I have a local branch where I've modified GithubLatest to use HeaderMatch#find_versions internally to pull the tag name from the Location header and then extract the version using the regex (we already do this for a few weird cask checks). With this approach, we wouldn't have to worry about a rate limit and the changes to the strategy are very simple and straightforward (it only modifies DEFAULT_REGEX and #find_versions).

I believe there are fewer than 10 GithubLatest livecheck blocks that check something other than the tag name (e.g., release name, description, date, etc.) but we can just handle those exceptions with PageMatch.

Thoughts on this alternative approach?

[SMillerDev]

My thoughts are mostly: GitHub provides an API for exactly what we want why parse headers or html? To avoid a possible rate limit?

I just don't see any added value in your alternatives.

[samford]

GitHub provides an API for exactly what we want why parse headers or html? To avoid a possible rate limit?

Yep, the rate limit has always been the primary reason we've avoided the GitHub API in livecheck. That and sometimes there are API incidents (making it temporarily inaccessible) while the website is still accessible during those outages. In all the time we've used GithubLatest, I don't think I've ever seen it fail because of a GitHub incident (and the alternative approach maintains that reliability).

My opinion has always been that the API would be nice (though not necessary) if we could reasonably use it but the rate limit was always too much of a deterrent to me. There may be some growing pains here but maybe it will mostly be fine if we continue to be careful to only use GithubLatest when it's necessary. If it really blows stuff up or is too much of an ongoing pain, we can always revert and take a different approach.

Anyway, I figured I would mention the alternative, just in case.

---

If it's good with you, I'll push some commits for straightforward changes that won't benefit from discussion (e.g., updating the documentation comments, small changes to align this with other strategies, etc.), to save you from having to incorporate a bunch of suggestions. Anything that we may want to discuss (not a lot, I don't think) I'll create a review comment/suggestion for and I can push commits once we agree.

Does that sound good to you?

[SMillerDev]

Yep, the rate limit has always been the primary reason we've avoided the GitHub API in livecheck. That and sometimes there are API incidents (making it temporarily inaccessible) while the website is still accessible during those outages.

In those cases you can use livecheck but not brew install so not sure it's a case we should be optimising for.

In all the time we've used GithubLatest, I don't think I've ever seen it fail because of a GitHub incident (and the alternative approach maintains that reliability).

But it did fail because they updated the website to lazyload downloads.

My opinion has always been that the API would be nice (though not necessary) if we could reasonably use it but the rate limit was always too much of a deterrent to me.

With the amount of github_latest formula, I think we'll be fine. But really the only way to see is to try.

If it's good with you, I'll push some commits for straightforward changes that won't benefit from discussion (e.g., updating the documentation comments, small changes to align this with other strategies, etc.), to save you from having to incorporate a bunch of suggestions. Anything that we may want to discuss (not a lot, I don't think) I'll create a review comment/suggestion for and I can push commits once we agree.

I think it would be nicer to add that to the github_releases PR since that way we can allow this change to test the waters for the releases one

[samford]

But it did fail because they updated the website to lazyload downloads.

This only affected casks that were checking release assets and some of them could be updated to simply check the tag instead. After dealing with the issue, we only have a handful where it's necessary to check release assets.

But yeah, I agree with this point. It wouldn't have been an issue if we were checking the GitHub API. I said at the time that checking the API is the easiest solution but wanted to look into other options before dealing with the rate limit.

But really the only way to see is to try.

👍

I think it would be nicer to add that to the github_releases PR since that way we can allow this change to test the waters for the releases one

I'm not sure I understand. If you're suggesting merging this as-is and doing follow-up changes in the GithubRelease PR, then sorry, that's a hard no. This isn't ready to merge as-is (e.g., at least one of the changes I want to see modifies the fundamental functionality of this) and there's no reason to merge it prematurely. Merging it before GithubReleases for testing purposes makes sense but we can do that when we're done with changes.

Merging this will immediately break ~100 existing livecheck blocks (~56 formulae, ~50 casks) [out of ~650 checks] and we should have draft [bulk] PRs ready in the related repositories that we can merge to prevent the breakages, otherwise it's going to be a mess. Some (maybe most) of the changes will be simple/straightforward but there may be others that take a little more work and there's no value in rushing to merge this PR. I'll take care of the related changes but I don't think there's any value in merging this before that's ready.

The change in this PR doesn't provide any immediate benefit but it does create issues, so we should take the time to do this right.

[SMillerDev]

This isn't ready to merge as-is (e.g., at least one of the changes I want to see modifies the fundamental functionality of this)

Then that would be good as a suggestion because I don't see any fundamental changes that it still needs. It works and it works well in my tests.

You were previously only talking about trivial changes though, so I'm not sure where the fundamental problems come from and why you haven't mentioned them for over two weeks.

[samford]

I've pushed a few commits to take care of simple changes that I don't think would benefit from discussion (comment if you disagree with anything but it all feels very benign). These review suggestions pertain to changes that affect the function of the strategy, one of which is a notable bugfix (i.e., this uses the full regex match as the version instead of the first capture group, so we get some versions like v1.2.3).

I'll work on the other two items that I mentioned previously when I have a moment. Massaging the RateLimitExceededError output for non-debug runs should be fairly simple. If preventing requests when we've reached the rate limit ends up being overly complicated or time-consuming, then we can take care of it in a follow-up PR (but we shouldn't put it off indefinitely).

Besides that, I need to work through the livecheck blocks that this will break and create draft PRs before we merge this. Almost all of them just need very simple regex tweaks, so it shouldn't take much work but I want to take care of it before this is merged (instead of having to put out fires afterward).

[SMillerDev]

Concerns about array vs object are addressed, tag_name is checked first and the regex only takes the first match. I think we're good to go.

[samford]

I'm still not convinced that we should check name (the release title) by default, even as a fallback. I haven't seen any evidence that this would be a beneficial change (while it can be detrimental) and it doesn't solve any problem that we currently face.

It won't apply in 99+% of cases and, if it does, there's a chance it will only serve to surface an incorrect or incomplete version string. For example, here are a few recent "latest" releases in core/cask where the matched version from the title doesn't correspond to the version we need:

• librealsense 2.53.1: The "Intel® RealSense™ SDK 2.0 (v2.53.1)" title contains a matchable version that appears before the software version, so "2.0" would be returned by default instead of the version we need.
• monero 0.18.2.2: The version in the title is "2.2" but the full version we need is "0.18.2.2".
• dungeon-crawl-stone-soup-console 0.30.0: The version in the title is "0.30" but the full version we need is "0.30.0".
• ios-app-signer 1.14: The version in the title is "1.14.0" but, strangely enough, the version we need is "1.14".

I've seen plenty of issues like these in release titles because they're not as strictly formatted as tags. If we ever come across the unlikely case that we need to check release titles for version information instead of tags, it should be handled in a livecheck block (like any other outlier).

[MikeMcQuaid]

We've recently had a release and this has been through (too many) rounds of review so let's merge it and, if we cannot fix forward in a timely fashion and this blocks CI and/or we get user reports, we can revert.

Thanks @SMillerDev for the PR and @samford for the detailed review (but try to do it in a single pass next time 🙇🏻).