Skip to content
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

calibrite-profiler 1.3.3 #187392

Merged
merged 2 commits into from
Oct 6, 2024
Merged

Conversation

MTCoster
Copy link
Contributor

@MTCoster MTCoster commented Oct 3, 2024

Also add a comment to the cask to explain the original intent and prevent the same mistaken change from being made again.

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:


Depends on Homebrew/brew#18488; there's no way currently to describe an exception as "prerelease or stable is fine" in github_prerelease_allowlist.json.

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.

The first-party download page links to the newest release on GitHub, so we can check that without having to worry about the pre-release issue. It's currently pointing to 1.3.3 despite it being "pre-release" on GitHub (as expected). I pushed a commit to update the livecheck block accordingly.

I converted this to a draft until the related brew PR is done, so this won't get merged prematurely.

@samford samford marked this pull request as draft October 4, 2024 14:14
@MTCoster
Copy link
Contributor Author

MTCoster commented Oct 4, 2024

The first-party download page links to the newest release on GitHub, so we can check that without having to worry about the pre-release issue. It's currently pointing to 1.3.3 despite it being "pre-release" on GitHub (as expected). I pushed a commit to update the livecheck block accordingly.

That works too! You can go ahead and rewrite history here to replace my original revert commit with yours if you like.

With this change I guess we no longer depend on Homebrew/brew#18488, but there's a separate discussion going on over there anyway so they can be independent now.

@samford samford added the livecheck Issues or PRs related to livecheck label Oct 4, 2024
@samford samford changed the title Revert "calibrite-profiler: move livecheck to GithubLatest" (#175776) calibrite-profiler 1.3.3 Oct 4, 2024
Upstream sometimes marks releases as "pre-release" on GitHub that are
advertised on the first-party download page as the latest stable
release, so we have to add this to `github_prerelease_allowlist.json`.
@samford
Copy link
Member

samford commented Oct 4, 2024

I'm not very knowledgeable about the prerelease allowlist behavior for casks but I updated this to use all, as that may allow us to move this forward in the interim time.

This works but our cask maintainers will have to confirm whether this is the appropriate way to handle this particular situation (i.e., allowing versions that are "pre-release" on GitHub, as we're tracking the first-party download page in the livecheck block and presumably upstream won't update it to link to an unstable version). Alternatively, we can use a specific version (1.3.3) but this would require us to update the version in the allowlist with almost every release.

Normally I would say that we should ask upstream to not mark stable releases as "pre-release" if they're advertised as stable on the first-party download page but they haven't ever responded to issues in the GitHub repository, so it may not be worth the effort.

@samford samford marked this pull request as ready for review October 4, 2024 14:31
Upstream sometimes marks a release as "pre-release" on GitHub but the
first-party download page links to it as the latest stable version.
This checks the download page, which links to the latest dmg file on
GitHub without having to worry about latest/pre-release.
@MTCoster
Copy link
Contributor Author

MTCoster commented Oct 4, 2024

I'm not very knowledgeable about the prerelease allowlist behavior for casks but I updated this to use all, [...]

Is the exception actually needed? From what I understand, it's only used for :github_* livechecks.

@samford
Copy link
Member

samford commented Oct 4, 2024

Is the exception actually needed? From what I understand, it's only used for :github_* livechecks.

Yes, formulae/casks using a GitHub tag/release in the url are audited to make sure that the release isn't marked as "pre-release" on GitHub (the modified code in the brew PR relates to the audit). github_prerelease_allowlist.json is used to add an exception to avoid this audit failure. Without that addition, brew audit will fail with v1.3.3 is a GitHub pre-release. [You can test this locally by removing the added line from the allowlist file and running brew audit --strict --online calibrite-profiler.]

That audit doesn't interact with livecheck blocks (i.e., the livecheck block URL/strategy isn't a factor) and it's just a coincidence that formulae/casks using a GitHub release asset often use the GithubLatest/GithubReleases strategies to identify versions.

Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thanks!

@p-linnane p-linnane merged commit b3a6576 into Homebrew:master Oct 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip livecheck Issues or PRs related to livecheck
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants