Fix js_binary and js_image_layer on windows - #2884
Open
peakschris wants to merge 16 commits into
Open
Conversation
|
|
![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: 6033e2ef9f
ℹ️ 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".
Author
|
@alexeagle could you ask someone to help me get this one merged? Also one in rules_jest. Would really help us if we didn't need to keep our forks up to date. |
The Windows js_binary .bat launcher failed to resolve the entry point for lifecycle-hook js_binary actions (e.g. npm postinstall on unrs-resolver), producing `the entry_point '' not found` or a malformed `...\bin\..\<repo>\...index.min.js` path. Three distinct bugs: 1. entry_point / node / npm / node_wrapper / node_patches were assigned `%RESULT%` inside `if (...)` blocks under `setlocal enabledelayedexpansion`, where cmd.exe expands `%RESULT%` at block parse-time (before the `call` that sets it) -> empty value. Use `!RESULT!` (delayed expansion). 2. `:resolve_execroot_bin_path` / `:resolve_execroot_src_path` compared the 3-char prefix `!short_path:~0,3!` against the 4-char literal `"..\\"`, which never matches, so the `..\`-prefix branch (which strips `..\` and inserts `external\`) never fired. Compare against `"..\"`. 3. execroot inheritance used a nested `if defined USE_EXECROOT_ENTRY_POINT ( if defined EXECROOT (...) )` with no else on the inner guard, so when USE_EXECROOT_ENTRY_POINT was set but EXECROOT was not (the lifecycle-hook case, launched directly in the execroot) it fell into the do-nothing branch and left JS_BINARY__EXECROOT empty -> leading `\` in resolved paths. Use a combined `_inherit_execroot` flag mirroring the `&&` condition in js_binary.sh.tpl, deriving the execroot otherwise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…_PATH `tar --directory <dir>` chdir's into <dir> via SetCurrentDirectory, which is hard-capped at MAX_PATH (260 chars) even with LongPathsEnabled=1 -- the current directory is the one Win32 path operation that never honors the `\?\` long-path prefix. Deep npm package store paths (e.g. babel's long peer-resolved placeholder names under react-scripts) exceed this and fail with "could not chdir to ...". The minimal Windows bsdtar in this toolchain also lacks the `-s` path-rewrite that would avoid the chdir. On Windows, extract into a SHORT-named staging TreeArtifact (chdir target stays well under MAX_PATH), then copy_directory -- which is long-path aware -- into the final deep store TreeArtifact. Once cwd is the short staging dir, tar creates the package's own (possibly deep) entries via CreateFileW, which does honor long paths. Non-Windows keeps the original single-step extract into the store dir. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The generated js_binary .bat wrapper (and BAT_INITIALIZE_RUNFILES) called bare `findstr` during runfiles/execroot detection, before any PATH is set. Bazel build/aspect actions that run with use_default_shell_env=False (e.g. aspect_rules_lint's eslint aspect) get a hermetic env with no System32 on PATH, so `findstr` fails with "'findstr' is not recognized". Fully-qualify to %SYSTEMROOT%\system32\findstr.exe (matching aspect_bazel_lib's windows_utils.bzl, which already relies on %SYSTEMROOT% being injected into Windows action envs). Zero-regression: works everywhere bare findstr did, plus where PATH lacks System32. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The js_binary .bat launcher scanned runtime args with `for %%a in (%*)`, but cmd's FOR set parser splits each token on '=' (and ',' ';'). A single runtime arg such as `--node_options=--require=<path>` (passed e.g. by aspect_rules_lint's eslint aspect via eslint.workaround_17660.js) was split into `--node_options`, `--require`, `<path>`, so the `--node_options=` interceptor never matched and the fragments leaked through to the program -- eslint aborted with "Invalid option '--node'". Replace the FOR loop with a shift-based loop using `%1`, which yields each whole argument as cmd parsed it from the command line (split on spaces only, respecting quotes), preserving '=' inside values. Complements the build-time fixed-arg hoisting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The runtime-arg scan used `for %%a in (%*)` (and a later shift/%1 attempt),
but cmd's FOR set parser and the %1..%9 parameters both split tokens on '='.
A single runtime arg like `--node_options=--require=<path>` (passed by
aspect_rules_lint's eslint aspect via eslint.workaround_17660.js) was split
into fragments, so the `--node_options=` interceptor never matched and the
fragments leaked to the program ("Invalid option '--node'").
The raw command line %* preserves '=', so tokenize it on spaces only via
`for /f "delims= "` (which also keeps quoted, space-containing values intact),
with an empty-ARG guard so the loop terminates rather than spins if for/f
skips a token (e.g. one beginning with the default eol ';').
…NTFS Windows/NTFS stat() reports st_ino==0 for every file, which breaks bsdtar (libarchive) in two silent ways when building js_image_layer tarballs: 1. Hardlink dedup (keyed on dev,ino) false-links unrelated package-store files into shared content, corrupting the layer. 2. libarchive's self-reference guard decides an input file "is" the output archive, prints "Can't add archive to itself", SKIPS the file, and still exits 0 -- so Bazel ships a layer missing files. This surfaced downstream as MODULE_NOT_FOUND at container start (filerepo losing stack-trace/lib). Neither is fixable from the prebuilt bsdtar CLI. Fixes: - mtree now declares nlink=1 for every regular file, so bsdtar skips its hardlink resolver entirely (defeats #1). - tar-create is routed through a small Node wrapper (js_image_layer_tar.mjs) that fails the build if tar prints the self-reference signature (#2) or if the produced archive contains any hardlink member (canary for #1 / nlink=1 regression). Node is already a toolchain of this rule, so this stays hermetic and cross-platform. Detection predicates live in js_image_layer_tar_lib.mjs (imported by the CLI and by the unit test); the CLI calls main() unconditionally. An ESM entry-point self-check is unreliable under Bazel on Windows -- Node realpath's the main entry through the external/<repo> junction, so import.meta.url and process.argv[1] disagree and main() would silently never run. Adds a regression test (tar_guard_test) covering both detection predicates. Co-Authored-By: Claude Opus 4.8 <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.
Changes:
Fix js_binary to run on windows without bash; works both in js_image_layer and via bazel
Fix js_image_layer to run on windows without bash
This means windows oci images do not need to package bash
Disable failing tests due to windows or --noenable_runfiles
Fixes: