fix: correctly handle empty chdir on js_binary (#2815)

The `js_binary` rule has a `chdir` attribute, and it is common to set
`chdir = package_name()` to indicate that the binary should run under
the target bin directory in the subdirectory corresponding to the
current package.

The logic for this checks `if ctx.attr.chdir`, which is usually fine but
does not work correctly when the target is at the top level of an
external repo. In that case `package_name()` is empty, making `chdir`
falsy, but we do still need to change directories since the package is
in an external repo.

This change fixes that bug and adds a test verifying that the binary
runs in the expected working directory.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

The js_binary rule now correctly handles an empty chdir in a target in
an external repo.

### Test plan

- Covered by existing test cases
- New test cases added

Author: acozzette

e2e/js_binary_workspace/.bazelignore

@@ -0,0 +1 @@
+workspace/

e2e/js_binary_workspace/BUILD.bazel

@@ -14,3 +14,13 @@ build_test(
         "@workspace//:run_bin",
     ],
 )
+
+# Verify that js_binary and js_test in an external repo with chdir =
+# package_name() run in the expected working directory.
+test_suite(
+    name = "external_tests",
+    tests = [
+        "@workspace//:js_binary_chdir_test",
+        "@workspace//:js_test_chdir_test",
+    ],
+)

e2e/js_binary_workspace/MODULE.bazel

@@ -2,6 +2,7 @@ module(name = "js_binary_workspace")
 
 bazel_dep(name = "aspect_rules_js", version = "0.0.0", dev_dependency = True)
 bazel_dep(name = "bazel_skylib", version = "1.8.1", dev_dependency = True)
+bazel_dep(name = "rules_shell", version = "0.6.1", dev_dependency = True)
 
 local_path_override(
     module_name = "aspect_rules_js",

e2e/js_binary_workspace/workspace/BUILD.bazel

@@ -1,4 +1,6 @@
-load("@aspect_rules_js//js:defs.bzl", "js_binary", "js_run_binary")
+load("@aspect_rules_js//js:defs.bzl", "js_binary", "js_run_binary", "js_test")
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
+load("@rules_shell//shell:sh_test.bzl", "sh_test")
 
 js_binary(
     name = "bin",
@@ -13,3 +15,48 @@ js_run_binary(
     tool = ":bin",
     visibility = ["//visibility:public"],
 )
+
+# Write the expected relative working directory beginning with bazel-out/
+genrule(
+    name = "working_directory_expected",
+    outs = ["working_directory_expected.txt"],
+    cmd = "echo $(@D) > $@",
+)
+
+# Check that the working directory used by js_test matches the chdir argument.
+js_test(
+    name = "js_test_chdir_test",
+    args = ["$(rlocationpath :working_directory_expected.txt)"],
+    chdir = package_name(),
+    data = [":working_directory_expected.txt"],
+    entry_point = "check_chdir.mjs",
+    tags = ["manual"],
+)
+
+write_file(
+    name = "working_directory_test_sh",
+    out = "working_directory_test.sh",
+    content = [
+        "#!/usr/bin/env bash",
+        "actual=$1",
+        "expected=$2",
+        'expected_len=$(wc -c < "$expected")',
+        # Assert that the end of $actual exactly matches $expected
+        'diff -u <(tail -c "$expected_len" "$actual") "$expected"',
+    ],
+)
+
+# Check that the working directory used by js_run_binary matches the chdir
+# argument on js_binary.
+sh_test(
+    name = "js_binary_chdir_test",
+    srcs = ["working_directory_test.sh"],
+    args = [
+        "$(rootpath :out_workspace.txt)",
+        "$(rootpath :working_directory_expected.txt)",
+    ],
+    data = [
+        ":out_workspace.txt",
+        ":working_directory_expected.txt",
+    ],
+)

e2e/js_binary_workspace/workspace/MODULE.bazel

@@ -1,6 +1,9 @@
 module(name = "workspace")
 
 bazel_dep(name = "aspect_rules_js", version = "0.0.0")
+bazel_dep(name = "bazel_skylib", version = "1.5.0")
+bazel_dep(name = "rules_shell", version = "0.6.1")
+
 local_path_override(
     module_name = "aspect_rules_js",
     path = "../../..",

e2e/js_binary_workspace/workspace/check_chdir.mjs

@@ -0,0 +1,12 @@
+import { readFileSync } from 'fs';
+import { 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`);
+    process.exit(1);
+}

js/defs.bzl

@@ -83,6 +83,15 @@ def js_binary(**kwargs):
     Args:
         **kwargs: All attributes of the [js_binary](#js_binary) rule.
     """
+
+    # Often a js_binary target will set "chdir = package_name()", and if it is
+    # in the top-level directory then this will result in an empty string. That
+    # argument may still be significant, though, particularly if the target is
+    # in an external repo. We make sure to replace an empty string with "." so
+    # that the underlying rule can distinguish this from an unset chdir
+    # parameter.
+    if kwargs.get("chdir") == "":
+        kwargs["chdir"] = "."
     _js_binary(
         enable_runfiles = select({
             Label("@bazel_lib//lib:enable_runfiles"): True,
@@ -117,6 +126,8 @@ def js_test(**kwargs):
     Args:
         **kwargs: All attributes of the [js_test](#js_test) rule.
     """
+    if kwargs.get("chdir") == "":
+        kwargs["chdir"] = "."
     _js_test(
         enable_runfiles = select({
             Label("@bazel_lib//lib:enable_runfiles"): True,