fix(brew): infer formula version from the GitHub release tag path - #13327
waynehoover wants to merge 3 commits into
Conversation
The tap formula shim inferred a version from the download URL's
basename only, so a release asset named for its platform rather than
its version failed outright:
could not infer formula version; add an explicit version declaration
goku is the reported case. Its stable URL ends in goku-arm.zip, which
carries no version at all, while the tag segment immediately before it
does.
Port Homebrew's GitHub releases URL parser and try it before the
basename pattern, which is also the order Homebrew uses in
VERSION_PARSERS (Library/Homebrew/version.rb). The order matters
independently of the missing-version case: when a filename carries a
partial version followed by a platform suffix, the basename pattern
over-captures it, so apfel-1.10.0-arm64-macos.tar.gz was yielding
1.10.0-arm64-macos as the version.
Verified against the 253 tap formulae installed locally: 244 unchanged,
goku newly resolves, 8 formulae corrected from a platform-suffixed
version string to the bare version, 0 regressions. All 9 now agree
exactly with Homebrew's own Version.detect on the same URL.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughChangesGitHub release version inference
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The version inference change preserves explicit versions and fallback behavior while covering release-tag versions absent from asset names. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def version_from_url_path(url) | ||
| match = url.to_s.match(GITHUB_RELEASE_URL) | ||
| match && match[1] | ||
| end | ||
|
|
||
| def version_from_basename(url) | ||
| basename = File.basename(url.to_s).sub(/\.(tar\.(gz|xz|bz2|zst)|tgz|txz|zip|gz)\z/i, "") | ||
| match = basename.match(/(?:^|[-_v])([0-9]+(?:\.[0-9A-Za-z]+)+(?:[-_.][0-9A-Za-z]+)*)/) | ||
| match && match[1] | ||
| end | ||
|
|
||
| def inferred_version(url) | ||
| version_from_url_path(url) || version_from_basename(url) |
There was a problem hiding this comment.
The existing metadata test declares an explicit version, so it bypasses this new path parser and its precedence over basename inference. Without focused cases for platform-only filenames, supported tag prefixes, and meaningful filename suffixes, later regex changes could silently reintroduce incorrect version inference. Adding regression coverage would protect the behavior introduced here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed, and done. Pushed infers_version_from_github_release_tag_path in tap.rs, which uses the same shim-invocation path as the existing extracts_formula_metadata_without_homebrew test but with formulae that declare no version, so the URL heuristics are actually exercised.
Covered: a platform-named asset with no version in the filename, a filename carrying a partial version plus a platform suffix, a filename with a trailing -1, the bare/r/v/V tag prefixes, a non-release URL still reading the filename, and an explicit version still winning over both. Every expectation matches what Homebrew's own Version.detect returns for the same URL.
The existing metadata test declares an explicit version, so it never
exercises either URL heuristic. Add cases that pin them directly:
- a platform-named asset whose filename has no version at all
- a filename carrying a partial version plus a platform suffix,
where reading the basename yields "1.10.0-arm64-macos"
- a filename with a trailing "-1", which Homebrew also discards once
a numeric tag is present
- the bare, r, v and V tag prefixes
- a non-release URL, which still reads the filename
- an explicit version, which still wins over both
Every expectation matches what Homebrew's own Version.detect returns
for the same URL.
1 participant
The problem
The tap formula shim infers a version from the basename of the stable URL only. A GitHub release asset is routinely named for its platform rather than its version, so those formulae fail outright:
gokuis the case I hit. Its stable URL is:File.basenamegivesgoku-arm, which contains no version. The version is in the tag path segment immediately before it.The fix
Port Homebrew's GitHub releases URL parser and try it before the basename pattern, which is the order Homebrew itself uses:
The ordering is not incidental. It also fixes a second, quieter bug: when a filename carries a partial version followed by a platform suffix, the basename pattern over-captures the suffix.
apfel-1.10.0-arm64-macos.tar.gzwas yielding1.10.0-arm64-macosas the version string. Those formulae did not error, they just installed under a wrong version.Verification
Ran the shim against all 253 tap formulae installed on this machine, before and after:
goku)The 8 corrected are
apfel,apfel-gui,apfel-run,apfel-tag,auge,bgbgone,ohr,translate, each going from<version>-arm64-macosto<version>.All 9 changed formulae were then cross-checked against Homebrew's own
Version.detecton the same URL, and every one now agrees exactly. Cases covered include bare tags with no prefix,v/V/rprefixes, plain tarballs, and/archive/refs/tags/URLs (both unchanged), plus a URL with no version anywhere (still nil, still raises).Deliberately not covered
This ports one of Homebrew's parsers, not all of them.
hid_bootloader_clistill cannot be inferred because its tag isLUFA-210130, which is not numeric-with-dots, and it needs a different parser.The other remaining failures in that sweep are unrelated to version inference. They are two separate shim gaps, in the same family as #13053 and #13163:
uninitialized constant Languagefrominclude Language::Python::Virtualenv(qmk,apfel-mcp)uninitialized constant CurlDownloadStrategyandrequire "download_strategy"(acli,ecctl, and the kodehealth tap)To be explicit, since these are easy to conflate: this PR does not fix
qmk. I am happy to send theLanguage::*shim as a follow-up.Summary by CodeRabbit
randvprefixes.