Skip to content

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

Merged
merged 1 commit into from
May 15, 2025
Merged

status 2.33.2 (new cask) #210351

merged 1 commit into from
May 15, 2025

Conversation

yakimant
Copy link
Contributor

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:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • Checked the cask was not already refused (add your cask's name to the end of the search field).
  • 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.

@BrewTestBot BrewTestBot added new cask missing zap Cask is missing a zap stanza, please add one. labels Apr 28, 2025
@BrewTestBot BrewTestBot removed the missing zap Cask is missing a zap stanza, please add one. label Apr 28, 2025
Copy link
Member

@khipp khipp left a 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.

@yakimant yakimant force-pushed the add_status_cask branch 2 times, most recently from 7ed53fc to 8da365d Compare May 8, 2025 12:10
@yakimant yakimant requested a review from khipp May 8, 2025 12:10
@yakimant
Copy link
Contributor Author

yakimant commented May 8, 2025

@khipp can you please review again?

Note, I've modified you suggested regex to include v in the version if it's there.
The reason is that, if it's not included - download will fail.

Issues it that both with and without v version exist unfortunately:

without: https//github.com/status-im/status-desktop/releases/download/2.33.2/StatusIm-Desktop-2.33.2-16fe57-aarch64.dmg

with: https://github.com/status-im/status-desktop/releases/download/v2.33.1/StatusIm-Desktop-v2.33.1-85b284-aarch64.dmg

@khipp
Copy link
Member

khipp commented May 8, 2025

We usually strip any prefix from the returned version and include it in the url if required.

Adding the v to the cask's version will prevent upgrading from a non-prefixed to a prefixed version due to how version comparison is handled:

  • v2.33.2 -> v2.33.3 will work
  • v2.33.2 -> 2.33.3 will work
  • 2.33.2 -> 2.33.3 will work
  • 2.33.2 -> v2.33.3 won't be recognized as a newer version

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.

@krehel
Copy link
Member

krehel commented May 8, 2025

Is there something that prevents naming the release artifacts consistently?

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.

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.

@yakimant
Copy link
Contributor Author

@khipp @krehel @samford thanks for you feedback

I talked to the developers and they promisses to keep versions without v in:

  • git tags
  • github releases
  • filenames

Sorry for taking you time, but I learned a lot, hope it will be usefull in the future.

I removed v from both livecheck and download url.

@krehel
Copy link
Member

krehel commented May 13, 2025

I talked to the developers and they promisses to keep versions without v in:

  • git tags
  • github releases
  • filenames

Appreciated very much @yakimant - thank you for going the extra step to get this sorted out. It will make the PR much easier.

Sorry for taking you time, but I learned a lot, hope it will be usefull in the future.

No apologies needed, we appreciate you working with us in a friendly and professional way.

@yakimant yakimant requested review from samford, krehel and khipp May 15, 2025 15:24
Copy link
Member

@khipp khipp left a comment

Choose a reason for hiding this comment

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

Thank you, @yakimant!

@khipp khipp added this pull request to the merge queue May 15, 2025
Merged via the queue into Homebrew:master with commit fc6736b May 15, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants