Livecheck: Use Homebrew curl based on root domain #13292
Conversation
At the moment, `#use_homebrew_curl?` can only be true for a `homepage` or `stable`/cask `url` with `using: :homebrew_curl`. If the checked URL differs from these URLs, livecheck won't use brewed curl. This limitation prevents livecheck from using brewed curl for a `livecheck` block URL that's a string literal (not a symbol for a `#checkable_url` like `:stable`, `:head`, `:url`). `libzip` was the original formula referenced in the related brew issue and it meets this criterion, so it doesn't appear to be handled by the existing `#use_homebrew_curl?` implementation. Additionally, the existing behavior can cause livecheck to unnecessarily use brewed curl for a completely different website (e.g., `cubelib`, `otf2`). For example, if the `stable` URL has `using: :homebrew_curl` and the `livecheck` block has `url :homepage`, livecheck will use brewed curl when checking the `homepage`. If these are completely different domains/servers, it's unlikely that we would need to use brewed curl when checking the `homepage`, so this particular behavior may not be beneficial. This commit reimplements `use_homebrew_curl?` to apply brewed curl when the checked URL's root domain is the same as the root domain of an aforementioned formula/cask URL with `using: :homebrew_curl`. For example, this looser approach would allow a `livecheck` block checking `https://www.example.com/downloads/` to use brewed curl if the `stable` URL was `https://downloads.example.com/example.zip` with `using: :homebrew_curl`. These could be different servers but, based on related formulae, this looseness is necessary for the moment. This approach aims to resolve both issues, allowing brewed curl to be applied to a slightly broader range of URLs (i.e., not limited to just the `#checkable_urls`) while also helping to avoid unnecessarily applying brewed curl when it's less likely to be useful (completely different domains). Neither approach is perfect but this one may be more useful in the interim time. Depending on how this looser approach works in practice, we may want to consider returning to a stricter approach once we have something like `using: :homebrew_curl` in `livecheck` blocks (this is forthcoming). Being explicit in a `livecheck` block is the most reliable approach (i.e., only use brewed curl when needed), so we could favor that and pare down the automated approach to only what's needed to support implicit checks (i.e., with no `livecheck` block). Of course, it's also possible to drop the automated approach entirely and simply require a `livecheck` block in this scenario but we can decide on how to handle this when the time comes.
|
Review period will end on 2022-05-18 at 04:54:12 UTC. |
BrewTestBot
added
the
waiting for feedback
MikeMcQuaid
added
the
critical
BrewTestBot
removed
the
waiting for feedback
|
Review period skipped due to |
|
@MikeMcQuaid @samford i think this means brew is missing some vendored gems now. See the last linked ticket. |
|
@SMillerDev Reverted now. |
|
I certainly missed something here. I know we usually have Potentially affected areas that I'm aware of:
|
@samford don't worry about it, it happens! brew/.github/workflows/tests.yml Lines 149 to 168 in f56cd09 The fix here I think is adjusting Stretch goals: figure out why that test didn't work right and fix that too 😅 |
Makes sense (and good to know going forward). Are we thinking |
|
@samford unless the gem is |
huge, sorry! |
|
From what I'm seeing, we should only need the To be sure, I removed everything from my local I'll get a PR up shortly to reinstate this (with related fixes), so the reversion hopefully doesn't affect FX's GCC work in homebrew/core. |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?At the moment,
#use_homebrew_curl?can only be true for ahomepageorstable/caskurlwithusing: :homebrew_curl. If the checked URL differs from these URLs, livecheck won't use brewed curl. This limitation prevents livecheck from using brewed curl for alivecheckblock URL that's a string literal (not a symbol for a#checkable_urllike:stable,:head,:url).libzipwas the original formula referenced in the related brew issue and it meets this criterion, so it doesn't appear to be handled by the existing#use_homebrew_curl?implementation.Additionally, the existing behavior can cause livecheck to unnecessarily use brewed curl for a completely different website (e.g.,
cubelib,otf2). For example, if thestableURL hasusing: :homebrew_curland thelivecheckblock hasurl :homepage, livecheck will use brewed curl when checking thehomepage. If these are completely different domains/servers, it's unlikely that we would need to use brewed curl when checking thehomepage, so this particular behavior may not be beneficial.This commit reimplements
use_homebrew_curl?to apply brewed curl when the checked URL's root domain is the same as the root domain of an aforementioned formula/cask URL withusing: :homebrew_curl. For example, this looser approach would allow alivecheckblock checkinghttps://www.example.com/downloads/to use brewed curl if thestableURL washttps://downloads.example.com/example.zipwithusing: :homebrew_curl. These could be different servers but, based on related formulae, this looseness is necessary for the moment.This approach aims to resolve both issues, allowing brewed curl to be applied to a slightly broader range of URLs (i.e., not limited to just the
#checkable_urls) while also helping to avoid unnecessarily applying brewed curl when it's less likely to be useful (completely different domains). Neither approach is perfect but this one may be more useful in the interim time.Besides that, this reworks, expands, and reorganizes the related tests a bit. There's still some room for improvement here and I may take another pass at it but I wanted to get this PR up in the interim time, as there's an open PR in homebrew/core that fails CI due to a related
livechecksituation and it should be resolved by this.One thing to note is that this implementation uses
addressableto identify the root domain of a URL. Doing this accurately requires a TLD-aware approach, as some top-level domains use more than one part (e.g.,co.uk), so we can't simply assume that the last two parts (e.g.,example.com) are the root domain.This gem is already in our dependency tree and this simply adds it to our
Gemfile. There's a feature that I plan to implement eventually where this would also be useful, so I hope using this gem inbrewis amenable. If not, do let me know of any alternatives, as the implementation relies on being able to identify the root domain (and matching the full host doesn't meet the same needs).Depending on how this looser approach works in practice, we may want to consider returning to a stricter approach once we have something like
using: :homebrew_curlinlivecheckblocks (this is forthcoming). Being explicit in alivecheckblock is the most reliable approach (i.e., only use brewed curl when needed), so we could favor that and pare down the automated approach to only what's needed to support implicit checks (i.e., with nolivecheckblock). Of course, it's also possible to drop the automated approach entirely and simply require alivecheckblock in this scenario but we can decide on how to handle this when the time comes.