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
Reconsider a better API to replace direct Homebrew::Livecheck::Strategy
calls within formulae and casks
#16194
Comments
@Bo98 There are 18 casks that called At present, these are used in scenarios where the |
Homebrew::Livecheck::Strategy::*
calls within casksHomebrew::Livecheck::Strategy
calls within casks
So I guess the API improvement that could be done here is a way to specify a series of strategies rather than being restricted to just the one strategy. |
Homebrew::Livecheck::Strategy
calls within casksHomebrew::Livecheck::Strategy
calls within formulae and casks
@Bo98 Is it worth updating some of the offending casks to at least use |
Probably yeah. It's necessary for them to at least work mostly properly without I believe |
An idea of what that could look like: livecheck do
url "https://www.winzip.com/en/download/"
regex(/href=.*?winzipmacedition[._-]?v?(\d+)\.dmg/i)
strategy :page_match do |page, regex|
major_version = page[regex, 1]
next if major_version.blank?
"https://download.winzip.com/winzipmacedition#{major_version}.dmg"
end.then_strategy :extract_plist
# or alternatively:
end
strategy :extract_plist
end livecheck do
url "https://chromiumdash.appspot.com/fetch_releases?channel=Stable&platform=Mac"
regex(/(\d+\.\d+\.\d+\.\d+)/i)
strategy :json do |json|
# Find the v8 commit hash for the newest Chromium release version
v8_hash = json.max_by { |item| Version.new(item["version"]) }.dig("hashes", "v8")
next if v8_hash.blank?
"https://chromium.googlesource.com/v8/v8.git/+/#{v8_hash}"
end.then_strategy :page_match do |page, regex|
page.scan(regex).map(&:first)
end
end |
In terms of how the DSL should look, would it be too confusing to just have multiple strategy blocks and then just evaluate them in the order they were declared. That would probably be the simplest option. livecheck do
url "https://chromiumdash.appspot.com/fetch_releases?channel=Stable&platform=Mac"
regex(/(\d+\.\d+\.\d+\.\d+)/i)
strategy :json do |json|
# Find the v8 commit hash for the newest Chromium release version
v8_hash = json.max_by { |item| Version.new(item["version"]) }.dig("hashes", "v8")
next if v8_hash.blank?
"https://chromium.googlesource.com/v8/v8.git/+/#{v8_hash}"
end
strategy :page_match do |page, regex|
page.scan(regex).map(&:first)
end
end |
This is a nice idea. I'm fine with either DSL but agreed that this should happen sooner rather than later and is desirable, thanks @Bo98! |
Yeah, my idea doesn't really make much sense since it doesn't address passing of information from the first strategy to the second one which is needed here. Are we sure that this only requires a new DSL or would it also mean changing how livecheck works internally as well? It now seems to me like something that's not worth the effort. Maybe just add a linter check for the |
To elaborate a little more we can't really have a separate or chained method because the matches from the first strategy block are often needed to build the URL for the second strategy block. It might be possible to have a nested strategy method though which could do the trick. It would essentially work like Using the Beforelivecheck do
url :homepage
regex(%r{href=.*?/uploads/(\d+)/(\d+)/iBabel\.zip}i)
strategy :page_match do |page, regex|
match = page.match(regex)
cask = CaskLoader.load(__FILE__)
download_url = "https://macinchem.org/wp-content/uploads/#{match[1]}/#{match[2]}/iBabel.zip"
app_version = Homebrew::Livecheck::Strategy::ExtractPlist.find_versions(cask: cask,
url: download_url)[:matches].values.max
next if app_version.blank?
"#{app_version.to_s.split(",").first},#{match[1]},#{match[2]}"
end
end Afterlivecheck do
url :homepage
regex(%r{href=.*?/uploads/(\d+)/(\d+)/iBabel\.zip}i)
strategy :page_match do |page, regex|
match = page.match(regex)
download_url = "https://macinchem.org/wp-content/uploads/#{match[1]}/#{match[2]}/iBabel.zip"
app_version = strategy_matches(:extract_plist, url: download_url).values.max
next if app_version.blank?
"#{app_version.to_s.split(",").first},#{match[1]},#{match[2]}"
end
end The uses of |
Not sure I understand the problem you're describing really. Each return value can replace the original URL. url = (initial url, e.g. stable.url)
return nil if strategies.empty?
strategies.each do |strategy|
url = run_strategy(url) # internally does its comparisons etc then calls DSL block
end
version = url # last url is version Seem doable, though I guess it could be argued that it looks "weird" having differing return values.
That's fine too if sufficiently documented and it works for all the cases described. Might look better too but I've not checked all the scenarios. |
Coming to this a little late, so pardon the information dump. I went through Use of I have some stashed work to allow us to call I wasn't previously aware of the four casks with The other cask ( It's technically possible to support successive Even if it's technically feasible, the mental model of multiple A public method like the If that's an agreeable solution, I'm happy to work on an implementation. I'm not sure that we can avoid using |
|
That's correct. At the moment, the issue is that
We could pass The tricky bit is that we would have to use |
This seems to make a lot more sense than trying to reload the cask inside blocks that live inside a cask (which I think is a smell). I've personally found it very surprising the few times I discovered (then forgot, and then re-discovered) that livecheck blocks (or, at least, blocks inside livecheck blocks) have a scope that's different from the formula. |
The only reason we need to access the cask within the cask is because of the current API. With a better public API like If there's another use case separate from that though, then it makes sense to maybe change what |
For service blocks we already make the formula available to allow us to get path information from it (#13705). In this case, we don't even need to expose it as a method but just have it exist in the same scope so that we can use it in the internal |
This seems like a good model to emulate here 👍🏻 |
Verification
brew install wget
. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.Provide a detailed description of the proposed feature
21 casks in Homebrew/cask & Homebrew/cask-versions contain direct calls to
Homebrew::Livecheck::Strategy
methods.An additional 8 formulae use
Homebrew::Livecheck::Strategy.page_content
(which at least is a better API compared to the rest).It's not a lot thankfully, but I don't think these are great because:
Homebrew::Livecheck::Strategy::ExtractPlist.find_versions
) is horrific in these contexts, requiringCaskLoader.load
to load the cask itself again.What is the motivation for the feature?
Seeing
CaskLoader.load("winzip")
insidewinzip
itself, which is problematic and causes issues.How will the feature be relevant to at least 90% of Homebrew users?
It realistically won't given its scoped to livecheck only which is maintainer-only, but an improved API will have better maintainability and avoids bugs associated by easy misuse, particularly when things change in
brew
without the knowledge these exist. A formula/cask not reloading itself is an example of one such assumption thatbrew
makes, unless you do so in a very specific way (i.e. using__FILE__
, and even that isn't guaranteed to work universally if loading from a contents string is used).What alternatives to the feature have been considered?
If we prefer it this way, we could just mark them as public API, as long as other improvements are made to avoid
CaskLoader
.The text was updated successfully, but these errors were encountered: