refactor: move most js_binary launcher logic from bash into launcher.cjs - #3015
Merged
Merged
Conversation
acozzette
force-pushed
the
simplify-launcher
branch
from
September 15, 2026 03:20
01baf44 to
75867b6
Compare
The bash launcher stamped from js_binary.sh.tpl now only does what has to happen before node starts or after it exits: set the target's environment, consume --bazel-bindir, set up stdout/stderr/exit-code capture, initialize runfiles, cd into the bindir, resolve node and the entry point, gather node options, and exec/wait. Everything else moves to a new static js/private/node-bootstrap/launcher.cjs that bash passes as node's first --require, ahead of any user node_options: the copy_data_to_bin validations, the entry point and wrapper checks, JS_BINARY__NODE_WRAPPER, JS_BINARY__NODE_PATCHES, JS_BINARY__FS_PATCH_ROOTS, PATH, the execroot, the chdir, and the debug and info dumps. It then loads bootstrap.cjs, so the ordering a user preload observes is unchanged. The cd into the bindir stays in bash on purpose: node resolves a bare --require/--import specifier against the cwd at startup, so a node_options preload in a build action depends on it happening first. Because the preload therefore runs one directory below where the execroot derivation is written, bash exports JS_BINARY__CHANGED_TO_BINDIR when it actually changes directory; the preload walks back out of the bindir, replays the same case analysis, and deletes the variable so nothing below the launch sees it. It works from process.cwd() rather than the shell's $PWD, so its paths are spelled the way node spells them, while bash keeps its own spelling for the entry point, which it hands to node as an argument. launcher.cjs rewrites its own process.execArgv entry to bootstrap.cjs so that fork()ed children inherit exactly the preload they did before, and in a worker thread, which gets node's original exec arguments rather than that array, it only loads bootstrap.cjs. That split is the rule for where a given piece of work belongs: launcher.cjs runs once per launch, bootstrap.cjs once per node process and per worker thread. The js_binary chdir moves with it, out of bootstrap.cjs, where it had been costing a third of that file's bytes. It is per-launch, not per-process: it ends by deleting JS_BINARY__CHDIR precisely so that a child process and a worker thread, which both inherit the directory it left them in, do not compound a relative path by applying it again. It runs after the bootstrap rather than before it, so that the ordering coverage.cjs depends on is unchanged -- coverage resolves COVERAGE_DIR and picks the reporter's directory from the directory the launcher started in, and nothing else in the bootstrap reads the working directory. launcher.cjs finds the files beside it through process.execArgv[1] rather than __filename, which node realpaths back into the source tree, and it treats not being node's first --require as fatal rather than falling back. The fallback was unreachable -- bash appends node_options after this --require, and NODE_OPTIONS entries never appear in process.execArgv at all -- and wrong if it ever fired, since the source tree holds a node_bin, an npm_bin and a bootstrap.cjs of its own, so the checks would pass there and the launch would be handed paths leading out of the runfiles tree. JS_BINARY__FS_PATCH_ROOTS is now joined and split on path.delimiter instead of a literal ':' spelled once on each side. No behaviour change on Linux; on Windows ':' was shredding the "C:\..." execroot the preload now derives. That does not make the fs patch work on Windows -- JS_BINARY__RUNFILES is deliberately MSYS-spelled, so patcher() has been dropping both roots and silently returning a no-op for as long as that normalization has been there -- it just stops the delimiter being a second, independent reason it cannot. A failed cd used to abort the script under errexit. Bash now only changes directory when the bindir is there to change into, which leaves a runfiles tree alone, and the preload turns the remaining case into a named FATAL rather than a shell error. With the long BAZEL_BINDIR message gone from bash, the logging helpers no longer need printf formatting and take a finished message, which drops two shellcheck suppressions and stops collapsing runs of whitespace inside a logged path. Two behaviour changes worth knowing about: - $JS_BINARY__EXECROOT no longer expands inside fixed_args and node_options values. Bash expands those before node starts and the execroot is worked out after. It is still set for the program, its children and the preload, and $JS_BINARY__RUNFILES, $JS_BINARY__WORKSPACE and $BAZEL_BINDIR are unaffected. - On Windows JS_BINARY__EXECROOT is now spelled the way node spells it rather than the way MSYS bash does. Unexercised here. New tests in js/private/test/launcher run a node_options preload that asserts the cwd, environment, patch depth and execArgv it sees, and the same for a child it spawns. New tests in js/private/test/execroot cover the execroot in a runfiles tree, in a build action and with an execroot entry point, and cover the BAZEL_BINDIR fatal, none of which had coverage before. js/private/test/chdir gains a "." and a ".." case and now checks a spawned child and a worker thread as well, and the coverage tests gain a chdir, a combination nothing covered before. Windows and e2e/js_image_oci were not exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
force-pushed
the
simplify-launcher
branch
from
September 15, 2026 17:35
75867b6 to
872ad19
Compare
acozzette
marked this pull request as ready for review
September 15, 2026 17:48
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 872ad194df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
launcher.cjs pointed process.execArgv[1] at bootstrap.cjs only after returning early in a worker thread. A worker is handed node's original exec arguments rather than the array the main thread rewrote, so its copy still named launcher.cjs, and a child_process.fork() from a worker -- or a spawn of process.execPath that passes process.execArgv on -- started the child as a fresh launch. The child then redid the whole launch against itself: derived an execroot, overwrote JS_BINARY__EXECROOT, prepended the node wrapper directory to PATH a second time, and checked process.argv[1] as an entry point, which for a -e child is undefined: FATAL: aspect_rules_js[js_test]: the entry_point 'undefined' not found Doing the swap before the early return fixes it. It is bookkeeping about what a child should preload and is as true in a worker as on the main thread. The guard it carried went with it, having been tautological: launcherPath is process.execArgv[1], and the fatal above it has already established that execArgv[0] is --require. New tests in js/private/test/launcher spawn a child from a worker and assert that the worker's execArgv names the bootstrap and that the node wrapper directory appears on the child's PATH once rather than twice. Both fail without the fix, the first with the FATAL above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbedard
reviewed
Sep 15, 2026
jbedard
reviewed
Sep 15, 2026
jbedard
reviewed
Sep 15, 2026
Member
|
How does this effect |
jbedard
approved these changes
Sep 15, 2026
jbedard
left a comment
jbedard
left a comment
Member
There was a problem hiding this comment.
LGTM if you've verified this doesn't have any performance hit when launching a js_binary (directory or via js_run_binary).
Contributor
Author
Claude benchmarked it and told me that for runs that use the runfiles directory as the working directory (so |
logTo() names a destination it does not take, since every level goes to stderr and nothing chooses otherwise. It is now log(), and the level wrappers lose the "f" they inherited from the bash logf_* helpers, which had it because they took a printf format string. These take a finished message, as the bash ones have since that formatting went away. One incidental reflow: a logDebug() call fits on a line now that the name is two characters shorter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
added a commit
that referenced
this pull request
Sep 15, 2026
…er.cjs js_binary.cjs.tpl was a JavaScript translation of the bash launcher as it stood before #3015, so it re-implemented everything that change moved into js/private/node-bootstrap/launcher.cjs. The generated launcher now does only what js_binary.sh.tpl still does -- the environment, --bazel-bindir, runfiles, the bindir chdir, the entry point and node binary, and the argument split -- and hands the rest to the same preload the bash launcher loads. The preload takes its own path from process.execArgv[1] and the entry point from process.argv[1], which the in-process path already fabricates, so it needs no changes: the exec path passes `--require <launcher.cjs>` the way bash does, and the in-process path assigns the execArgv the exec path would have had and require()s it directly. Gone from the template, all of it now the preload's: the execroot scan and its BAZEL_BINDIR fatal, the copy_data_to_bin validations, the node wrapper, npm wrapper and bootstrap resolution, JS_BINARY__FS_PATCH_ROOTS, the PATH prepend, and the debug and info log blocks. Three baked constants go with them, since the preload derives all three from its own location, and _launcher_paths stops returning the two wrapper short_paths that had no other consumer. Two fixes come for free, both from no longer having a second implementation: JS_BINARY__CHDIR is honored again under the hermetic launcher -- #3015 moved it out of bootstrap.cjs, which is all this launcher used to load -- and JS_BINARY__FS_PATCH_ROOTS is joined with the platform delimiter rather than ':'. The template loses 283 lines and gains 109. //js/private/test/launcher:worker_fork_probe_worker_execArgv_is_bootstrap_test still fails under --use_hermetic_launcher=True; the next commit fixes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
added a commit
that referenced
this pull request
Sep 15, 2026
The generated JavaScript launcher grew as a translation of the pre-#3015 bash launcher, so it carried its own copy of the logging functions, the fs predicates and the Windows path normalizer. launcher.cjs then arrived on main with the same four things, and the two files have been re-implementing each other since the merge. Move them into js/private/node-bootstrap/util.cjs and have both require it. The preload keeps the same set minus what it never used; the generated launcher keeps exitWith() and cwd(), which are about its own exit and its own chdir rather than shared vocabulary. The ordering is what makes this possible. The generated launcher cannot load anything out of the runfiles tree until it has found that tree, which would have left the logging it needs to report a failed lookup on the wrong side of the require. So the stub resolves the preload as a second rlocation and passes it as the launcher's first argument, the way it already resolves node and the launcher itself. The helpers sit beside the preload, so the same argument finds them, and the launcher can load them before it does anything else. {{launcher}}, the resolveToolchainPath() call behind it and its existence check all go away with it: node could not have reached the first line if the stub had not found the file. That also fixes the preload lookup for a target with no runfiles tree, where the old string concatenation of JS_BINARY__RUNFILES had nothing to point at and the launcher fell back to the execroot sources. The stub reads the manifest, which is what that configuration has. Two notes for review: - The shared log() collapses whitespace, which is what the generated launcher did and the preload did not. None of the preload's messages contain a run of whitespace to collapse -- the long BAZEL_BINDIR one is concatenated, not wrapped -- so this is a no-op for it today, but it is a behaviour change. - The preload's path arrives from the stub relative to the launch directory when the stub was given a relative runfiles directory, and a relative specifier with no leading "./" is a package name to require(). It is made absolute before the chdir. //js_binary:test10 in examples/ is the case that catches this -- a genrule that runs a js_binary by a relative path -- and nothing in the root repository does. The NO_RUNFILES path is Windows-only (is_windows and not enable_runfiles) and is not exercised by any test on Linux, so that part is argued rather than verified. Root //... 379 pass / 10 skipped hermetic, 377 / 12 bash; examples 86/86 both ways; e2e/js_image_oci //src:image_test passes under the flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
This change moves most logic out of the bash launcher, keeping only the bare minimum functionality there. It sets things up so that launcher.cjs is responsible for everything that needs to run just once per launch, and bootstrap.cjs is responsible for things that also need to happen for worker threads and child processes created with
child_process.fork. I moved thechdirlogic intolauncher.cjsaccordingly, since this is something that should happen just once per launch.The motivation for this is to make it easier to move away from the bash launcher altogether. We are likely going to have an experimental bash-free launcher based on hermetic_launcher, and once we have two launchers we will want to keep the duplication between them to a minimum until we can drop the bash launcher.
Changes are visible to end-users: no
Test plan