revert: node must stay a required dependency, not :build - #10
Merged
Conversation
PR #9 scoped `depends_on "node"` to `=> :build`, reasoning that node/npm are only invoked inside `def install`, which never runs on a bottle pour. That's true, but incomplete: Homebrew's own build finalizer (Cleaner#rewrite_shebangs, which calls Language::Node::Shebang.detected_node_shebang) automatically rewrites any `#!/usr/bin/env node` shebang in an installed formula's files to the absolute path of that formula's *required* Node dependency — `formula.deps.select(&:required?)` in language/node.rb — silently skipping :build-scoped ones. rook's published npm package ships `bin/rook.cjs` with exactly that `#!/usr/bin/env node` shebang (a trampoline meant to bootstrap under whatever Node launched it, then re-exec into the bundled runtime). Under the previous unscoped `depends_on "node"`, Homebrew silently rewrote that shebang to Homebrew's own absolute node path when the 0.1.0 bottle was built — which is what made the bottle actually run, since Homebrew's node was also always force-installed alongside it. Scoping to :build stopped forcing that install, but did nothing about the already-published 0.1.0 bottle, which still has the old absolute path hardcoded in. Confirmed live: a fresh `brew install lambdatest/rook/rook` on a real macOS-14 and ubuntu-latest GitHub Actions runner (no Homebrew node preinstalled) both failed identically pouring that exact bottle: macos-arm: /opt/homebrew/bin/rook: /opt/homebrew/opt/node/bin/node: bad interpreter: No such file or directory linux-x64: /home/linuxbrew/.linuxbrew/bin/rook: cannot execute: required file not found Worse than the original complaint (extra Node installed, but working): this made the bottle fail to execute at all on any machine without Homebrew's node already present. Reverting to keep installs working; the actual fix for the extra-Node-install complaint needs to be more than a one-line dependency scope change — either accept it as a load-bearing implementation detail of this shebang-rewrite mechanism and correct the formula's caveats text (which currently overstates how self-contained this is), or replace the Homebrew-path launcher with a real shell-script wrapper like the curl/tarball path already uses, so it never depends on `env node` at runtime. Left as a follow-up, not decided here. brew-smoke.yml's dependency-scope check is inverted to assert node STAYS required, with the mechanism cited inline, so re-attempting the :build scoping trips CI before a real install does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
samyakLambda
added a commit
that referenced
this pull request
Aug 17, 2026
#11) Real fix for #497 (the extra-Node-install complaint) — not the caveats-text option, the shell-wrapper option. Both were on the table after the revert in #10; this is the one I picked, with reasoning below. ## Why this, not documenting the waste as intentional `rook`'s published npm package ships `bin/rook.cjs`: a small trampoline with a `#!/usr/bin/env node` shebang, designed to bootstrap under whatever Node launched it, resolve the bundled runtime via `require.resolve`, and re-exec into it. Under the old, `:required` `depends_on "node"`, Homebrew's own `Cleaner#rewrite_shebangs` (`language/node.rb`) silently rewrote that shebang to the absolute path of Homebrew's own node at build time — which is what made the bottle work, and also exactly the extra install #497 complains about. The trampoline's whole reason to exist is not knowing at *publish* time whether a bundled runtime will be present. Homebrew's `def install` **does** know — it just finished downloading and chmod'ing that exact binary. So instead of relying on npm's shim plus Homebrew's shebang-rewrite trick, this PR has `def install` write its own launcher directly: ```sh #!/bin/sh exec "<absolute path to the bundled node binary>" "<absolute path to dist/cli.js>" "$@" ``` Only in the success path (bundled runtime installed and verified) — the existing `opoo`-and-fall-back-to-system-Node path is untouched, since npm's original trampoline is exactly the right behavior there. This is strictly better than either alternative on the table: - **Actually fixes #497**: nothing in the installed tree has an `env node` shebang left for Homebrew to rewrite (or fail to) — `depends_on "node"` goes back to `=> :build`, this time correctly, since nothing depends on the rewrite happening. - **No changes needed to the private monorepo** — `bin/rook.cjs` is untouched; it's still exactly right for npm-direct/internal installs. - **Simpler signal handling than the trampoline it replaces, for this path**: `exec` replaces the shell's own process image, so a signal sent to the launcher's PID lands on the bundled node process natively, same PID, no relay. `bin/rook.cjs`'s ~150 lines of SIGINT/SIGTERM/SIGHUP forwarding exist specifically because `spawn()` creates a *child* process that needs that relay — `exec` in a shell script doesn't have that problem to begin with. Absolute paths baked into an installed script this way are ordinary, relocatable Homebrew practice — any formula that writes a wrapper referencing `#{libexec}` does the same; nothing new here. ## Verification Local `brew install --build-bottle` turned out to be broken in my environment for reasons unrelated to this change — I confirmed this by installing the **already CI-verified, currently-live** formula (pre-this-PR) and getting the identical `FormulaUnavailableError`. Rather than fight that, I pulled the real published `@testmuai/rook` and bundled-runtime npm packages, reconstructed `def install`'s exact `pkg_dir` layout by hand, and ran the actual generated wrapper end to end with `PATH=/usr/bin:/bin` (no node, no Homebrew, nothing): ``` $ env -i PATH="/usr/bin:/bin" ./rook --version 0.1.0 $ env -i PATH="/usr/bin:/bin" ./rook --help Usage: rook [options] [command] ... ``` All 6 `scripts/test-*.sh` harnesses green, `ruby -c`, `actionlint` clean. ## What it takes to actually ship A bottle *pours* a pre-built keg — it never runs `def install`. Merging this alone changes nothing for real users; `def install`'s new code only takes effect once `build-bottles.yml` rebuilds and republishes the 0.1.0 bottle. I'll dispatch that after merge and live-verify `brew-smoke` green on both platforms before considering this done — the same live-verification discipline that caught the #9 regression, this time before declaring victory instead of after. New `test do` assertion added as a structural guard on the actual mechanism (checks the launcher's shebang, not just that `rook --version` happens to work) — `brew-smoke.yml`'s existing "node stays required" guard is flipped back too, this time for the right reason. --- Agent-authored: Claude Sonnet 5, Claude Code CLI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts the
depends_onchange from #9. Merging immediately —mainiscurrently broken for a fresh
brew install lambdatest/rook/rookon anymachine without Homebrew's own
nodealready installed.What went wrong
#9 scoped
depends_on "node"to=> :build, reasoning that node/npmare only invoked inside
def install, which never runs on a bottlepour — true, but incomplete.
Homebrew's own build finalizer (
Cleaner#rewrite_shebangs, viaLanguage::Node::Shebang.detected_node_shebanginlanguage/node.rb) automatically rewrites any#!/usr/bin/env nodeshebang in an installed formula's files to the absolute path of
that formula's required Node dependency —
formula.deps.select(&:required?)— silently skipping:build-scopedones (caught by a rescued
ShebangDetectionError).rook's published npm package shipsbin/rook.cjswith exactly thatshebang — a small trampoline meant to bootstrap under whatever Node
launched it, then re-exec into the bundled runtime. Under the
previous unscoped
depends_on "node", Homebrew silently rewrotethat shebang to Homebrew's own absolute node path when the 0.1.0
bottle was originally built — which is what actually made the bottle
run, since Homebrew's node was also always force-installed alongside
it (the exact behavior #497/#9 set out to remove).
Scoping to
:buildstopped forcing that install, but thealready-published 0.1.0 bottle still has the old absolute path
hardcoded into it — bottles are immutable. Confirmed live, dispatching
brew-smokeagainst #9's merge commit on real GitHub Actions runners:Worse than the original complaint — that was extra-but-working; this
is completely broken on any machine without Homebrew's node already
present.
This PR
Reverts
depends_on "node" => :buildback todepends_on "node",with a long comment on the line explaining why, and inverts
brew-smoke.yml's dependency-scope assertion to guard againstre-making the same mistake (it now fails CI if
nodestops being arequired dependency, citing the mechanism, instead of only failing at
real-install time).
The extra-Node-install complaint (#497's original ask) is not fixed
by this revert — it's back to the original behavior. A real fix
needs to be more than a one-line scope change: either accept the extra
install as a load-bearing side effect of this shebang-rewrite
mechanism and correct the formula's
caveatstext (which currentlyoverstates how self-contained this is), or replace the Homebrew-path
launcher with a real shell-script wrapper — like the curl/tarball path
already uses — so it never depends on
env nodeat runtime at all.Left as a follow-up.
#498 (the piped-installer crash) is unaffected — different file,
unrelated mechanism, unchanged here.
Test status
ruby -c,actionlint, YAML parse, all 6scripts/test-*.shharnesses green. Will re-dispatch a live
brew-smokerun against thisbranch's merge commit to confirm installs work again before
considering this closed.
Agent-authored: Claude Sonnet 5, Claude Code CLI.