Conversation
…runfiles are off
On Windows without --enable_runfiles the launcher sets JS_BINARY__NO_RUNFILES
and resolves its entry point in the output tree rather than the runfiles tree.
It resolved it under BAZEL_BINDIR, which is the bin directory of the action
running the js_binary and so is in the target configuration. js_run_binary
copies the tool's entry point and data there, but a custom rule that runs a
js_binary tool via js_binary_lib.run_binary_action does not, and the tool is
built in the exec configuration, so the entry point was not found:
FATAL: aspect_rules_js[js_test]: the entry_point
'.../bazel-out/x64_windows-fastbuild/bin/js/private/test/launcher/stdout_stderr.mjs'
not found
Fall back to this js_binary's own $(BINDIR) when the entry point is not under
BAZEL_BINDIR, which keeps using the path-mapping-friendly BAZEL_BINDIR wherever
the copies are in place.
Case 8 in js/private/test/launcher covers the fallback on every platform by
setting JS_BINARY__NO_RUNFILES on the action itself. Its tool is tagged manual
and has an entry point of its own so that no target configuration copy of the
entry point exists to be found first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Windows smoke build failed in //js/private/test/chdir:cwd_run:
FAIL: child exited null: undefined
TypeError: Cannot read properties of undefined (reading 'trim')
at .../js/private/test/chdir/cwd.mjs:40:33
cwd.mjs spawns a child `node` to check that a relative chdir is not applied a
second time when the child re-enters bootstrap.cjs through the node wrapper's
--require. On Windows that wrapper is node.bat, which node will not spawn
without a shell, and bootstrap.cjs does not reach child processes on win32
anyway, so there is no second chdir there to guard against. Skip that check on
Windows; the cwd, PWD, JS_BINARY__CHDIR and worker thread checks still run.
Report child.error and stop dereferencing child.stdout when the child never
ran, so a spawn failure says what went wrong instead of raising a TypeError.
Also pass --keep_going in the smoke job so a build failure reports every
failing target rather than only the first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Windows does not build runfiles trees by default, and nothing under the runfiles
directory a js_test or `bazel run` is launched in exists then: the program's
files are only in the output tree, where copy_data_to_bin put them. The launcher
stayed in that empty directory, so a relative chdir and any data path relative
to the workspace root failed:
FATAL: aspect_rules_js[js_test]: could not change directory to
'js/private/test/data': ENOENT: no such file or directory,
chdir '...\from-genrule.bat.runfiles\_main' -> 'js/private/test/data'
Change directory to the bin directory in that case, which is where those paths
resolve, the same way they resolve in the runfiles tree when there is one. This
is what a js_binary in a build action already does via BAZEL_BINDIR.
Also in this repo's own build:
- The rollup and babel actions passed their config file as
$RUNFILES_DIR/$(rlocationpath ...), which names nothing without a tree. Pass
it under the bin directory instead, where the config is an input either way.
- The lifecycle hooks' generated .cmd bin shims passed "%*" rather than %*,
which collapses the arguments into one and, with no arguments, passes an empty
one. That is what made @kubernetes/client-node's build hook run `tsc ""`:
`error TS6231: Could not resolve the path ''`.
- npm_package_publish's shell tests read their arguments as cwd-relative
runfiles paths; they now resolve them with rlocation.
- cwd.mjs prints its cwd with forward slashes so the assertions on that line
read the same on both platforms.
Tests that cover behavior Windows does not have are marked incompatible with it,
each with the reason: unix domain sockets (watch), POSIX paths and symlinks in
the fs patch's own tests, child process patching that bootstrap.cjs does not do
on win32, arguments with shell metacharacters (the Windows native launcher
interpolates them into a `bash -c` line unquoted), assertions that read the bash
launcher a .bat wraps on Windows, the POSIX launcher snapshot, an sh_test that
puts node on the PATH itself, and the cases that need a runfiles tree.
Validated on Linux both normally and with runfiles turned off
(--noenable_runfiles --spawn_strategy=local plus a local edit making js_binary
set JS_BINARY__NO_RUNFILES off Windows too): 335 tests pass normally, and the
only failures left in the no-runfiles run are the targets marked incompatible
with Windows, which that simulation does not skip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-ups to the no-runfiles work, from what the Windows and
bazel-9-no-execroot-entry-point legs reported:
- The rollup and babel config arguments were pointed at the bin directory, but
with use_execroot_entry_point=False the tool's data is not copied there, so the
config was only in the tool's runfiles tree and the Linux leg broke. Pass the
config in srcs, which makes it an input of the action in the target
configuration in every mode, and name it the way these targets already name
their other inputs: relative to the bin directory they run out of.
- js_run_devserver read its config, its entries file, its tool and everything it
syncs into the sandbox under the runfiles directory. Read them from the bin
directory when there is no runfiles tree, as the launcher now does. The
constant is renamed FILES_ROOT since it is no longer always runfiles.
- npm_package_publish's shell tests convert the paths rlocation gives them:
those come back as Windows paths, which GNU tar reads as a host:path remote
spec ("tar (child): Cannot connect to D:") and bash cannot exec through.
335 tests pass on Linux, with --@aspect_rules_js//js:use_execroot_entry_point=False,
and in the no-runfiles simulation apart from the targets marked incompatible with
Windows, which the simulation does not skip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more from the Windows smoke run: - npm_publish.mjs spawns `npm`, which on the PATH is the launcher's npm.bat wrapper, and node refuses to spawn a .bat without a shell. The spawn error was never looked at and `process.exit(spawn.status)` exited 0 on a null status, so a publish that never happened reported success and printed nothing at all. Report the error and exit non-zero. `npm publish` still cannot run on Windows, so its test is marked incompatible with the reason. - node_modules_symlink_to_execroot.test.mjs compared a readlink result, which carries the platform separator, against the launcher's directory variables, which use forward slashes everywhere. Compare them in one form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es tree Where runfiles trees are not built, which is the default on Windows, RUNFILES_DIR names a directory holding nothing but the manifest, and a runfiles library has to read that manifest instead. Bazel does not put RUNFILES_MANIFEST_FILE in a build action's environment (bazelbuild/bazel#7994), so the launcher exported only RUNFILES_DIR and @bazel/runfiles fell back to walking a tree that was not there: RUNFILES_MANIFEST_FILE should have been set but wasn't. falling back to using runfiles symlinks. cfg_probe.txt not found in runfiles at _main/js/private/test/js_run_binary/cfg_probe.txt Export RUNFILES_MANIFEST_FILE from the launcher when the tree was not laid out and the manifest is really there. //js/private/test/js_run_binary:target_cfg_test now runs on Windows rather than failing to build, so it is no longer marked incompatible. The two RUNFILES_ONLY cases stay marked, with the reason corrected: they assert that cfg_probe.txt is *absent* from the target bin directory, which only holds for an action that cannot see files it did not declare. Other targets in that package do build cfg_probe.txt in the target configuration, so without a sandbox the probe finds that copy in the execroot. no_copy_to_bin_test's reason is likewise corrected: it requires a file by a path relative to the entry point, and require() does not consult a manifest. 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
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.
Changes are visible to end-users: yes/no
Test plan