-
-
Notifications
You must be signed in to change notification settings - Fork 11k
status 2.33.2 (new cask) #210351
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
status 2.33.2 (new cask) #210351
Conversation
7bc5d7e
to
fd54c09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your first contribution to homebrew-cask
, @yakimant! 🎉
I left a few suggestions below.
7ed53fc
to
8da365d
Compare
@khipp can you please review again? Note, I've modified you suggested regex to include Issues it that both with and without without: with: |
We usually strip any prefix from the returned version and include it in the Adding the
What I can think of is to include the prefixed version optionally: url "https://github.com/status-im/status-desktop/releases/download/#{version.csv.third || version.csv.first}/StatusIm-Desktop-#{version.csv.third || version.csv.first}-#{version.csv.second}-#{arch}.dmg",
verified: "github.com/status-im/status-desktop/releases/download/"
name "Status"
desc "Decentralised wallet and messenger"
homepage "https://status.app/"
livecheck do
url :url
regex(/^StatusIm[._-]Desktop[._-](v?(\d+(?:\.\d+)+))[._-](\h+)[._-]#{arch}\.dmg$/i)
strategy :github_latest do |json, regex|
json["assets"]&.map do |asset|
match = asset["name"]&.match(regex)
next if match.blank?
(match[1] == match[2]) ? "#{match[2]},#{match[3]}" : "#{match[2]},#{match[3]},#{match[1]}"
end
end
end Maybe @samford has a better idea on how to approach this. |
Is there something that prevents naming the release artifacts consistently? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something that prevents naming the release artifacts consistently?
Their releases are published by a bot but I imagine the release creation process may have a manual element to it if they end up with repeated (but unpredictable) inconsistencies in tag and/or filename versions.
To add another wrinkle to this issue, there doesn't appear to be any guarantee that the version in the tag and filename will both include/omit the leading v
. For example, the tag for 2.32.1 doesn't use a leading v
but the filename does (https://github.com/status-im/status-desktop/releases/download/2.32.1/StatusIm-Desktop-v2.32.1-aab802-aarch64.dmg).
As Klaus mentioned, including a leading v
in the version
like v2.33.2
can lead to Version
comparison issues, so that's not something we should do. Instead, we append optional parts to the version
, similar to what Klaus suggested but we also have to check the tag name for a leading v
(as they don't always align). We can do that separately (json["tag_name"]
) or simply use a regex that matches both the tag and filename versions from asset["browser_download_url"]
(we tend to use the latter in this scenario).
That said, dealing with two optional components poses a slight challenge. Usually we only deal with one optional component and omit it when it's not present. We can selectively omit the tag version and filename version depending on which has a leading v
but the tricky part is that we still have to include the tag version for padding if the filename version uses a leading v
but the tag version doesn't. That's more information than we need for that case but it's an unfortunate necessity of the version.csv
setup.
[Basically, we can't use 1.2.3,abc,,v1.2.3
because it will produce an error in LivecheckVersion
(as each version
part is passed to Version.new
), so we have to use 1.2.3,abc,1.2.3,v1.2.3
. This is also why we can't reduce these extra version
parts down to just v
, as the third version part would be empty if the tag doesn't include a leading v
.]
I've modified Klaus's example to account for the tag/filename version mismatch and added suggestions for the necessary changes. It's a fairly gnarly setup but that's often how it goes when dealing with upstream inconsistency.
Appreciated very much @yakimant - thank you for going the extra step to get this sorted out. It will make the PR much easier.
No apologies needed, we appreciate you working with us in a friendly and professional way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @yakimant!
Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.
In the following questions
<cask>
is the token of the cask you're submitting.After making any changes to a cask, existing or new, verify:
brew audit --cask --online <cask>
is error-free.brew style --fix <cask>
reports no offenses.Additionally, if adding a new cask:
brew audit --cask --new <cask>
worked successfully.HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask>
worked successfully.brew uninstall --cask <cask>
worked successfully.