fix(brew): define Homebrew's Language::* mixins in both shims - #13328
waynehoover wants to merge 3 commits into
Conversation
A formula pulls a Language mixin into its class body. qmk, for example:
class Qmk < Formula
include Language::Python::Virtualenv
That is a constant reference rather than a method call, so neither
shim's method_missing DSL fallback covers it. The NameError aborts
evaluation while the class body is still running:
Formula/qmk.rb:2:in '<class:Qmk>': uninitialized constant
Qmk::Language (NameError)
Metadata extraction therefore never reaches the url, version or
depends_on declarations below the include, and the source-build shim
cannot get far enough to report what it does not support.
Define the namespace brew actually ships (Java, Node, Perl, PHP and
Python, plus their Shebang and Virtualenv submodules) as empty modules
in both shims. The mixins only contribute install-time helpers, so
empty is the correct shape: metadata extraction now succeeds, and an
actual call to one of those helpers still fails loudly and specifically
through Formula#method_missing:
Error: formula uses `virtualenv_install_with_resources (install-time
helper)`, which mise's source-build shim does not support
Verified against the 253 tap formulae installed locally: qmk and
apfel-mcp newly extract, 251 unchanged, 0 regressions.
|
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)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe Brew build and metadata shims now define matching empty ChangesBrew Language namespace shims
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change consistently resolves the supported Language namespace constants while preserving intentional failures for unsupported install-time helpers. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
|
For wider context on where this fits: I opened #13330, which traces the whole chain for Short version, so it is clear what this PR does and does not buy: this closes the first of four gaps. |
Two gaps, matching the two ways this can regress. resolves_language_mixin_constants runs the metadata shim over a formula that includes every Language mixin from its class body, and asserts the url, version and depends_on declared below those includes are read. A NameError on the first include would hide all of them. language_mixin_definitions_match_across_shims asserts the two shims define the same namespace. They each carry their own copy, and a mixin that resolves while extracting metadata but not during a source build would fail late, after the download, rather than at evaluation.
The helper scanned for the first unindented `end`, which does span the whole block today because every nested `end` is indented. That is a property of the current formatting rather than of the structure, so reindenting the block would silently narrow the comparison instead of failing. Track nesting depth instead, and trim each line so reindentation alone is not reported as a difference. Also assert the extracted block still contains the Perl, PHP, Python and Virtualenv definitions: two spans truncated identically would compare equal while covering nothing.
1 participant
The problem
A formula pulls one of Homebrew's
Language::*mixins into its class body.qmk, for example:That is a constant reference, not a method call, so neither shim's
method_missingDSL fallback covers it. The NameError aborts evaluation while the class body is still being read, on line 2:Two consequences. Metadata extraction never reaches the
url,versionordepends_ondeclarations sitting below the include, so the formula cannot be resolved at all. And the source-build shim cannot get far enough to report what it does not support, so the user gets a bare NameError pointing at anincludeline instead of an explanation.The fix
Define the namespace brew actually ships (
Library/Homebrew/language/*.rb: Java, Node, Perl, PHP and Python, plus theirShebangandVirtualenvsubmodules) as empty modules in both shims.Empty is the correct shape rather than a shortcut: these mixins contribute only install-time helpers. Nothing the metadata shim extracts depends on them, and in the build shim an actual call still fails loudly and specifically through the existing
Formula#method_missingpath:That message is the one this shim is designed to produce for an unimplemented helper, and it replaces a NameError that named neither the feature nor the reason.
This follows the split already documented in
shim.rb: unknown class-level DSL warns and continues, unknown install-time helpers fail loudly. A missing constant fell through both, which is the gap being closed.Verification
Ran the metadata shim against all 253 tap formulae installed on this machine, before and after:
qmk1.1.8,apfel-mcp0.2.0)qmknow yields its version and all 17 dependencies. The build-shim half was verified separately by loadingshim.rb's definitions, defining a formula shaped likeqmk, and confirming theincludeevaluates and the helper call raisesShimUnsupportedErrorrather thanNameError.Scope
Same family as #13053 and #13163. This closes the
Language::*constants only.The other constant gap visible in that sweep is download strategies, which fails the same way and is not addressed here:
uninitialized constant CurlDownloadStrategy(acli,ecctl)cannot load such file -- download_strategyfrom a tap's ownrequire(kodehealth tap)Happy to follow up with that one if the approach here looks right. It needs slightly more thought than empty modules, since
using:already has real handling for resources.Independent of #13327, which fixes version inference and touches a different part of the same file.
Summary by CodeRabbit
Bug Fixes
Tests