refactor: use hermetic_launcher to invoke node - #2974
Conversation
397e281 to
a70e9c1
Compare
|
This is a pretty cool change. Would allow us to put the js_binary targets directly onto distroless images that don't have |
a70e9c1 to
9c6dc24
Compare
Coverage used to keep a js_binary off the hermetic launcher for two reasons. One of them is gone: #2984 moved lcov report generation into a node exit hook, so nothing is left that needs a process outliving the program. The other is NODE_V8_COVERAGE. node opens its V8 coverage connection during startup, before any --require preload runs, so launcher.cjs cannot turn coverage on for the process it is already in, and the stub has no way to set an environment variable. It starts node again with the variable set instead -- in place through process.execve on the nodes that have it, which is what the launcher script's `exec` did and costs the same nothing, and by forking on older ones. Only the first node in the tree does that. Finding NODE_V8_COVERAGE already set is what says a process is not it, so the re-executed process runs straight through, and so does every child, each of which node gives a coverage connection of its own. Without that condition the re-exec recurses forever rather than merely reporting twice. The report generator's path has no channel from the rule either, so it is derived from the preload's own runfiles path, the way setUpNode already finds the node wrapper. js_binary puts coverage.js in the runfiles exactly when it decided the target reports coverage, so whether it is there is the same answer to the same question. reExecOnEntryPoint generalizes to reExec() and is shared with the old-node ESM path, which picks up process.execve as a side effect. Measured with the hermetic_launcher_report output group under --collect_code_coverage, 76 targets move from blocked to eligible (42 -> 118) and every remaining verdict matches its non-coverage counterpart. Nothing changes today, though: coverage_enabled is false in the exec configuration, so no js_run_binary tool was ever blocked by this. The targets it frees are js_tests, which do not use the hermetic launcher yet. --- ### Changes are visible to end-users: no ### Test plan - New test case: coverage_test runs the launcher binary the way a coverage-enabled test action would -- COVERAGE_DIR and COVERAGE_MANIFEST and nothing else -- and asserts on the lcov report the merger would publish, for both the entry point and a child spawned through the node wrapper, which must not claim the report a second time. Verified non-vacuous: disabling setUpCoverage fails on the NODE_V8_COVERAGE assertion, and breaking the derived generator path leaves no report at all. - `bazel test //...`, `examples`, `bazel coverage //js/private/test/coverage/...` and `e2e/coverage` (inline and split) all pass. js_binary.sh.tpl is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # this -- JS_BINARY__WORKSPACE and the rest are documented as unreachable from it. | ||
| js_run_binary( | ||
| name = "capture_stderr", | ||
| args = select({"//conditions:default": []}), |
There was a problem hiding this comment.
Should this be its own target specifically describing the edge case it's testing? Is that a test that can merge on its own now?
| ":node_modules/google-protobuf", | ||
| ], | ||
| outs = ["bundle.js"], | ||
| # Only the config path needs `$RUNFILES_DIR` expanded; the rest are ordinary `args`, |
There was a problem hiding this comment.
Prefactor (without mentioning hermetic launcher which is an implementation detail)? Does this comment belong on the fixed_args instead of args?
There was a problem hiding this comment.
I'm actually inclined to think that if we go with the direction in this PR we should probably find a way to increase the fixed-arg limit so that changes like this one aren't necessary. Hermetic_launcher has a limit of 10 built-in args, and we use five of them ourselves, so that only leaves five for users.
There was a problem hiding this comment.
Is that the reason for this? Does something (hermetic-launcher action or js_binary or something) throw an error outlining issue if/when that happens?
There was a problem hiding this comment.
Maybe open an issue to discuss it on the hermetic-launcher repo? If you can't find any explanation for the limit in the code or existing (closed) issues...
There was a problem hiding this comment.
Is that the reason for this? Does something (hermetic-launcher action or js_binary or something) throw an error outlining issue if/when that happens?
Yes, hermetic-launcher will fail to stamp out a launcher if you try to give it more than 10 embedded args. I believe it's due to the fact that hermetic-launcher is implemented like a binary template so that launchers can be stamped out without requiring a compiler invocation. As a result there's a fixed number of slots and there's a size penalty to every launcher if we increase that number.
I filed an issue here last week but haven't gotten a response yet: hermeticbuild/hermetic-launcher#76
Since filing that issue I have mostly changed my mind, though. I think we are going to end up needing some form of generated launcher, because for example we need a way to set the NODE_V8_COVERAGE environment variable before invoking node. As long as we need a launcher anyway, we might as well put the embedded args there, and then we can have an unlimited number of them without needing any changes in hermetic_launcher.
| } | ||
|
|
||
| // The path this file was preloaded from. Not __dirname, which node has already resolved | ||
| // through the runfiles symlink back to the source tree; the files beside this one have to |
There was a problem hiding this comment.
To the source tree? Is that only on macos?
|
|
||
| const runfiles = resolveRunfiles(startCwd) | ||
| process.env.RUNFILES_DIR = runfiles | ||
| process.env.JS_BINARY__RUNFILES = runfiles |
There was a problem hiding this comment.
Should this be deprecated and tell people to use RUNFILES_DIR? Maybe that's a bigger discussion, and would be breaking so we'd have to keep it for now anyway...
| // is a real target that does both. | ||
| const IS_MAIN_THREAD = require('worker_threads').isMainThread | ||
|
|
||
| const LOG_PREFIX = 'aspect_rules_js[js_binary]' |
| // Everything else here still has to happen in a worker, because the fs patches apply per | ||
| // realm. | ||
| // | ||
| // The bash launcher never met any of this: the shell did its cd before node started, and |
There was a problem hiding this comment.
Why are we mentioning "the bash launcher"? This comment will last longer then todays "bash launcher" and shouldn't reference it
| // and does not return. The two things a preload cannot do to the node it is already | ||
| // running in both end up here. | ||
| // | ||
| // process.execve replaces this process rather than adding one, which is what the launcher |
There was a problem hiding this comment.
don't reference the history of the repo in a comment
| function reExec(mainScript, env, reason) { | ||
| debug(`re-executing node on ${mainScript}: ${reason}`) | ||
| const argv = [...process.execArgv, mainScript, ...process.argv.slice(2)] | ||
| if (typeof process.execve === 'function') { |
There was a problem hiding this comment.
When would this not be true? That might be worth a (1 line) comment)
3 participants
Changes are visible to end-users: no
Test plan