fix: resolve chdir inside the tree the launcher leaves us in

e2e/js_binary_workspace/workspace/check_chdir.mjs

@@ -1,12 +1,13 @@
-import { readFileSync } from 'fs';
-import { join } from 'path';
+import { realpathSync } from 'fs';
+import { dirname, join } from 'path';
 
 const runfiles = process.env.JS_BINARY__RUNFILES;
-const expectedFile = join(runfiles, process.argv[2]);
-const expected = readFileSync(expectedFile, 'utf8').trim();
-const cwd = process.cwd();
 
-if (!cwd.endsWith(expected)) {
-    process.stderr.write(`Expected cwd to end with:\n  ${expected}\nActual cwd:\n  ${cwd}\n`);
+// Compare resolved physical paths since JS_BINARY__RUNFILES may contain symlinks.
+const expected = realpathSync(join(runfiles, dirname(process.argv[2])));
+const cwd = realpathSync(process.cwd());
+
+if (cwd !== expected) {
+    process.stderr.write(`Expected cwd:\n  ${expected}\nActual cwd:\n  ${cwd}\n`);
     process.exit(1);
 }

js/private/devserver/src/js_run_devserver.mjs

@@ -125,6 +125,18 @@ async function generateChecksum(p) {
     )
 }
 
+// Converts a chdir value, which names a path in the output tree, to one relative to the root of
+// the custom sandbox. The sandbox mirrors the runfiles tree, where an external repository is a
+// top-level directory beside the main one rather than under "external/".
+export function sandboxRelativeChdir(chdir) {
+    if (!chdir) {
+        return ''
+    }
+    return chdir.startsWith('external/')
+        ? '../' + chdir.slice('external/'.length)
+        : chdir
+}
+
 // Converts a size in bytes to a human readable friendly number such as "24 KiB"
 export function friendlyFileSize(bytes) {
     if (!bytes) {
@@ -444,7 +456,7 @@ async function main(args, sandbox, config) {
 
     const entriesPath = path.join(RUNFILES_ROOT, args[1])
 
-    const cwd = config.chdir ? path.join(sandbox, config.chdir) : sandbox
+    const cwd = path.join(sandbox, sandboxRelativeChdir(config.chdir))
 
     const tool = config.tool
         ? path.join(RUNFILES_ROOT, config.tool)
@@ -756,7 +768,7 @@ async function cycleSyncRecurse(cycle, file, isDirectory, sandbox, writePerm) {
         )
 
         // Intentionally synchronous; see comment on mkdirpSync
-        mkdirpSync(path.join(sandboxMain, config.chdir || ''))
+        mkdirpSync(path.join(sandboxMain, sandboxRelativeChdir(config.chdir)))
         await main(args, sandboxMain, config)
     } catch (e) {
         console.error(e)

js/private/js_binary.bzl

@@ -5,7 +5,7 @@ load("@bazel_lib//lib:directory_path.bzl", "DirectoryPathInfo")
 load("@bazel_lib//lib:expand_make_vars.bzl", "expand_locations", "expand_variables")
 load("@bazel_lib//lib:windows_utils.bzl", "create_windows_native_launcher_script")
 load(":bash.bzl", "BASH_INITIALIZE_RUNFILES")
-load(":js_helpers.bzl", "LOG_LEVELS", "envs_for_log_level", "gather_files_from_js_infos", "gather_runfiles")
+load(":js_helpers.bzl", "LOG_LEVELS", "envs_for_log_level", "gather_files_from_js_infos", "gather_runfiles", "normalize_chdir")
 
 _ATTRS = {
     "chdir": attr.string(
@@ -366,22 +366,7 @@ def _bash_launcher(ctx, nodeinfo, entry_point_path, log_prefix_rule_set, log_pre
 
     if ctx.attr.chdir:
         # Set chdir env if not already set to allow js_run_binary to override
-        chdir_value = _expand_env_if_needed(ctx, ctx.attr.chdir)
-
-        # Normalize workspace-relative chdir for external repositories to avoid requiring
-        # callers to manually prefix with "external/<repo>/".
-        if (
-            ctx.label.repo_name and
-            not (chdir_value.startswith("external/") or chdir_value.startswith("/")) and
-            not chdir_value.startswith("@")
-        ):
-            if chdir_value == ".":
-                normalized_chdir = "external/{}".format(ctx.label.repo_name)
-            else:
-                normalized_chdir = "external/{}/{}".format(ctx.label.repo_name, chdir_value)
-        else:
-            normalized_chdir = chdir_value
-
+        normalized_chdir = normalize_chdir(_expand_env_if_needed(ctx, ctx.attr.chdir), ctx.label.repo_name)
         envs.append(_ENV_SET_IFF_NOT_SET.format(var = "JS_BINARY__CHDIR", quoted_value = _bash_quote(normalized_chdir)))
 
     # Set log envs iff not already set to allow js_run_binary to override

js/private/js_binary.sh.tpl

@@ -43,7 +43,7 @@ fi
 function resolve_capture_path {
     case "$1" in
     bazel-out/*) echo "$PWD/$1" ;;
-    *) echo "$PWD/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$1" ;;
+    *) echo "$PWD/$BAZEL_BINDIR/$1" ;;
     esac
 }
 
@@ -131,9 +131,9 @@ function logf_debug {
 function resolve_execroot_bin_path {
     local short_path="$1"
     if [[ "$short_path" == ../* ]]; then
-        echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/external/${short_path:3}"
+        echo "$JS_BINARY__EXECROOT/$BAZEL_BINDIR/external/${short_path:3}"
     else
-        echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$short_path"
+        echo "$JS_BINARY__EXECROOT/$BAZEL_BINDIR/$short_path"
     fi
 }
 

js/private/js_helpers.bzl

@@ -265,3 +265,25 @@ def gather_files_from_js_infos(
                 files_depsets.append(js_info.npm_sources)
 
     return depset(transitive = files_depsets)
+
+def normalize_chdir(chdir, repo):
+    """Normalizes a chdir value to a path relative to the root of the output tree.
+
+    Args:
+        chdir: the `chdir` attribute value, which is workspace-relative; an empty string means
+            the root package
+        repo: canonical name of the repository the target is in, empty for the main repository
+
+    Returns:
+        the normalized chdir value
+    """
+    chdir = "." if chdir == "" else chdir
+
+    # Callers pass a workspace-relative path such as package_name(), so an external repository
+    # needs the "external/<repo>/" prefix its packages have in the output tree. Absolute paths and
+    # paths that already name a repository are left alone.
+    if not repo or chdir.startswith("external/") or chdir.startswith("/") or chdir.startswith("@"):
+        return chdir
+    if chdir == ".":
+        return "external/{}".format(repo)
+    return "external/{}/{}".format(repo, chdir)

js/private/js_run_binary.bzl

@@ -13,7 +13,7 @@ load("@aspect_rules_js//js:defs.bzl", "js_run_binary")
 load("@bazel_lib//lib:copy_to_bin.bzl", _copy_to_bin = "copy_to_bin")
 load("@bazel_lib//lib:run_binary.bzl", _run_binary = "run_binary")
 load("@bazel_lib//lib:utils.bzl", bazel_lib_utils = "utils")
-load(":js_helpers.bzl", _envs_for_log_level = "envs_for_log_level")
+load(":js_helpers.bzl", _envs_for_log_level = "envs_for_log_level", _normalize_chdir = "normalize_chdir")
 load(":js_info_files.bzl", _js_info_files = "js_info_files")
 
 def js_run_binary(
@@ -336,23 +336,7 @@ def js_run_binary(
 
     # Configure working directory to `chdir` is set
     if chdir != None:
-        normalized_chdir = "." if chdir == "" else chdir
-        repo = native.repo_name()
-
-        # Normalize workspace-relative chdir for external repositories so callers can pass
-        # native.package_name() without worrying about external/ prefixing.
-        # - Leave absolute paths and already-external paths untouched.
-        # - For external repos, prefix with "external/<repo>/".
-        if (
-            repo and
-            not (chdir.startswith("external/") or chdir.startswith("/")) and
-            not chdir.startswith("@")
-        ):
-            if chdir == ".":
-                normalized_chdir = "external/{}".format(repo)
-            else:
-                normalized_chdir = "external/{}/{}".format(repo, chdir)
-        fixed_env["JS_BINARY__CHDIR"] = normalized_chdir
+        fixed_env["JS_BINARY__CHDIR"] = _normalize_chdir(chdir, native.repo_name())
 
     # Disable node patches if requested
     if patch_node_fs:

js/private/node-bootstrap/bootstrap.cjs

@@ -7,28 +7,27 @@ if (process.env.NODE_COMPILE_CACHE) {
 
 const patchfs = require('./fs.cjs').patcher
 const {
-    BAZEL_BINDIR,
-    JS_BINARY__BINDIR,
+    BUILD_WORKSPACE_DIRECTORY,
     JS_BINARY__CHDIR,
-    JS_BINARY__EXECROOT,
     JS_BINARY__FS_PATCH_ROOTS,
     JS_BINARY__LOG_DEBUG,
     JS_BINARY__LOG_PREFIX,
     JS_BINARY__NODE_WRAPPER,
     JS_BINARY__PATCH_NODE_FS,
+    TEST_SRCDIR,
 } = process.env
 
 // Change directory as indicated by the chdir option on js_binary or js_run_binary.
 if (JS_BINARY__CHDIR) {
-    // An "external/<repo>" chdir names a path in the bin directory rather than one relative to
-    // the cwd we were given, which for a js_test is the runfiles tree.
-    const dir = JS_BINARY__CHDIR.startsWith('external/')
-        ? require('node:path').join(
-              JS_BINARY__EXECROOT,
-              BAZEL_BINDIR || JS_BINARY__BINDIR,
-              JS_BINARY__CHDIR
-          )
-        : JS_BINARY__CHDIR
+    let dir = JS_BINARY__CHDIR
+    // chdir is relative to the root of the output tree, where an external repository's package sits
+    // under "external/<repo>". The runfiles tree instead gives every repository a top-level
+    // directory beside our own, so re-point the path there rather than leaving the tree. Bazel sets
+    // TEST_SRCDIR for a test and BUILD_WORKSPACE_DIRECTORY for `bazel run`, and these are the two
+    // situations where we will be in the runfiles tree.
+    if ((TEST_SRCDIR || BUILD_WORKSPACE_DIRECTORY) && dir.startsWith('external/')) {
+        dir = '../' + dir.slice('external/'.length)
+    }
     try {
         process.chdir(dir)
     } catch (e) {

js/private/test/BUILD.bazel

@@ -4,8 +4,11 @@ load("@bazel_skylib//rules:write_file.bzl", "write_file")
 load("@rules_shell//shell:sh_test.bzl", "sh_test")
 load("//js:defs.bzl", "js_binary", "js_library", "js_test")
 load(":js_library_test.bzl", "js_library_test_suite")
+load(":normalize_chdir_test.bzl", "normalize_chdir_tests")
 load(":run_environment_info_test.bzl", "run_environment_info_test_suite")
 
+normalize_chdir_tests(name = "test_normalize_chdir")
+
 ####################################################################################################
 # Write a js_binary launcher to the source tree so it is shell checked on commit
 

js/private/test/image/checksum_test.expected

@@ -1,4 +1,4 @@
-97511535490d677b58431c2c3074b9ec2eeb34b54254443c0a9e9c669e3705d5  js/private/test/image/cksum_node.tar
+296041ddc116956ef3ad57ec178b0e14e12c06972c331c0921cda1df292092a0  js/private/test/image/cksum_node.tar
 70b10220a2c05d87da4271c17c38ded8febc725e20e06f1c3809d2bb02ba4ae7  js/private/test/image/cksum_package_store_3p.tar
 2cb6f678d6eb0b2e9d5e2637f41ae3f192233752f1ea2a55cede2531deec2a64  js/private/test/image/cksum_package_store_1p.tar
 79afa99006aff19460354e72cf2634db693670d09ccc7531fbf27f1f20fe0a5f  js/private/test/image/cksum_node_modules.tar

js/private/test/image/custom_layers_nomatch_test_node.listing

@@ -8,7 +8,7 @@ drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 0      0        2899 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 0      0        3090 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 -r-xr-xr-x  0 0      0       37120 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/

js/private/test/image/custom_owner_test_node.listing

@@ -7,7 +7,7 @@ drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runf
 drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/
 drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/
 drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 100    0        2899 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 100    0        3090 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 -r-xr-xr-x  0 100    0       37120 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
 drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
 drwxr-xr-x  0 100    0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/

js/private/test/image/default_test_node.listing

@@ -7,7 +7,7 @@ drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runf
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 0      0        2899 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 0      0        3090 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 -r-xr-xr-x  0 0      0       37120 Jan  1  1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/

js/private/test/image/non_ascii/custom_layer_groups_test_node.listing

@@ -9,7 +9,7 @@ drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 0      0        2899 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 0      0        3090 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/nodejs/

js/private/test/image/platform_deps/rspack_linux_arm64_test_node.listing

@@ -9,7 +9,7 @@ drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/plat
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 0      0        2899 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 0      0        3090 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 -r-xr-xr-x  0 0      0       37120 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/rules_nodejs++node+nodejs_linux_arm64/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/platform_deps/bin.runfiles/rules_nodejs++node+nodejs_linux_arm64/bin/

js/private/test/image/regex_edge_cases_test_node.listing

@@ -8,7 +8,7 @@ drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x  0 0      0        2899 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x  0 0      0        3090 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
 -r-xr-xr-x  0 0      0       37120 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
 drwxr-xr-x  0 0      0           0 Jan  1  1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/

js/private/test/js_run_devserver/js_run_devserver.spec.mjs

@@ -2,6 +2,7 @@ import {
     isNodeModulePath,
     is1pPackageStoreDep,
     friendlyFileSize,
+    sandboxRelativeChdir,
 } from '../../devserver/js_run_devserver.mjs'
 
 // isNodeModulePath
@@ -78,3 +79,22 @@ for (const [k, v] of friendlyFileSize_cases) {
         process.exit(1)
     }
 }
+
+// sandboxRelativeChdir
+const sandboxRelativeChdir_cases = new Map()
+sandboxRelativeChdir_cases.set(undefined, '')
+sandboxRelativeChdir_cases.set('', '')
+sandboxRelativeChdir_cases.set('.', '.')
+sandboxRelativeChdir_cases.set('foo/bar', 'foo/bar')
+// The sandbox has no external/ directory; a repository sits beside the main one
+sandboxRelativeChdir_cases.set('external/myrepo', '../myrepo')
+sandboxRelativeChdir_cases.set('external/myrepo/foo/bar', '../myrepo/foo/bar')
+for (const [k, v] of sandboxRelativeChdir_cases) {
+    const a = sandboxRelativeChdir(k)
+    if (a !== v) {
+        console.error(
+            `Expected sandboxRelativeChdir(${k}) to be '${v}' but got '${a}'`
+        )
+        process.exit(1)
+    }
+}