fix: make js_run_binary hoist runfiles in the exec configuration

The `js_run_binary` macro has a `use_execroot_entry_point` option which is
enabled by default and which hoists out the runfiles into the execroot.
Currently this hoisting effectively rebuilds the runfiles in the target
platform config, but they should really be in the exec config since they
consist of code that is going to run during the build action.

This change fixes that problem by making sure we hoist the runfiles in the exec
config. I also updated the launcher script, since it needs to be prepared to
look for the entry point in a different directory. This is all guarded by the
`hoist_runfiles_to_exec_cfg` option, which defaults to False for now. Ideally
we should make it True by default, but that will be a breaking change.

This addresses #2754, #2121, and #2073, but does not automatically fix them yet
since the fix is opt-in for now.

Author: acozzette

MODULE.bazel

@@ -20,6 +20,11 @@ bazel_dep(name = "rules_nodejs", version = "6.7.3")
 # Changes ensured by rules_js:
 # 3.2.2: https://github.com/bazel-contrib/bazel-lib/commit/cac2d7855949d1b222fa26888892fbbe1d31015d
 bazel_dep(name = "bazel_lib", version = "3.2.2")
+single_version_override(
+    module_name = "bazel_lib",
+    patch_strip = 1,
+    patches = ["//:bazel-lib.patch"],
+)
 
 # NB: LOWER BOUND on earliest BCR release of protobuf module, to avoid upgrading the root module by accident
 bazel_dep(name = "protobuf", version = "3.19.6")

bazel-lib.patch

@@ -0,0 +1,66 @@
+diff --git a/lib/private/run_binary.bzl b/lib/private/run_binary.bzl
+index 7ce4038..65c2f15 100644
+--- a/lib/private/run_binary.bzl
++++ b/lib/private/run_binary.bzl
+@@ -51,12 +51,12 @@ Possible fixes:
+     # Location and Make variable expansion can reference paths that aren't path mapped.
+     can_path_map = True
+     for a in ctx.attr.args:
+-        expanded = expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs)
++        expanded = expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.tools + ctx.attr.srcs), inputs = ctx.files.tools + ctx.files.srcs, outs = outputs)
+         can_path_map = can_path_map and expanded == a
+         args.add_all(split_args(expanded))
+     envs = {}
+     for k, v in ctx.attr.env.items():
+-        envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.srcs), inputs = ctx.files.srcs, outs = outputs, attribute_name = "env")
++        envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.tools + ctx.attr.srcs), inputs = ctx.files.tools + ctx.files.srcs, outs = outputs, attribute_name = "env")
+         can_path_map = can_path_map and envs[k] == v
+ 
+     stamp = maybe_stamp(ctx)
+@@ -72,6 +72,7 @@ Possible fixes:
+         outputs = outputs,
+         inputs = inputs,
+         executable = ctx.executable.tool,
++        tools = ctx.files.tools,
+         arguments = [args],
+         resource_set = resource_set(ctx.attr),
+         mnemonic = ctx.attr.mnemonic if ctx.attr.mnemonic else None,
+@@ -95,6 +96,10 @@ _run_binary = rule(
+             mandatory = True,
+             cfg = "exec",
+         ),
++        "tools": attr.label_list(
++            allow_files = True,
++            cfg = "exec",
++        ),
+         "env": attr.string_dict(),
+         "srcs": attr.label_list(
+             allow_files = True,
+@@ -122,6 +127,7 @@ def run_binary(
+         execution_requirements = None,
+         use_default_shell_env = False,
+         stamp = 0,
++        tools = [],
+         **kwargs):
+     """Runs a binary as a build action.
+ 
+@@ -215,12 +221,19 @@ def run_binary(
+ 
+             These files can be read and parsed by the action, for example to pass some values to a linker.
+ 
++        tools: A list of tool dependencies for this action.
++
++            These are treated similarly to srcs, except that they are built in
++            the exec configuration. Any code that will be executed as part of
++            the action should generally be listed here instead of in srcs.
++
+         **kwargs: Additional arguments
+     """
+     _run_binary(
+         name = name,
+         tool = tool,
+         srcs = srcs,
++        tools = tools,
+         args = args,
+         env = env,
+         outs = outs,

contrib/nextjs/defs.bzl

@@ -202,6 +202,7 @@ def nextjs_build(name, config, srcs, next_js_binary, data = [], **kwargs):
         chdir = native.package_name(),
         mnemonic = "NextJs",
         progress_message = "Compile Next.js app %{label}",
+        hoist_runfiles_to_exec_cfg = False,
         **kwargs
     )
 
@@ -327,6 +328,7 @@ def nextjs_standalone_build(name, config, srcs, next_js_binary, data = [], **kwa
         chdir = native.package_name(),
         mnemonic = "NextJs",
         progress_message = "Compile Next.js standalone app %{label}",
+        hoist_runfiles_to_exec_cfg = False,
         **kwargs
     )
 

examples/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU=

@@ -20,7 +20,7 @@ npm_package/packages/pkg_d/package.json=1146506398
 npm_package/packages/pkg_e/package.json=2046864123
 package.json=-2075121688
 package_json_module/package.json=-1167380556
-pnpm-lock.yaml=-657827575
+pnpm-lock.yaml=559561144
 pnpm-workspace.yaml=-1653994035
 runfiles/package.json=-1545884645
 stack_traces/package.json=2011229626

examples/MODULE.bazel

@@ -6,7 +6,13 @@ local_path_override(
     path = "..",
 )
 
-bazel_dep(name = "bazel_lib", version = "3.0.0", dev_dependency = True)
+bazel_dep(name = "bazel_lib", version = "3.2.2", dev_dependency = True)
+single_version_override(
+    module_name = "bazel_lib",
+    patch_strip = 1,
+    patches = ["//:bazel-lib.patch"],
+)
+
 bazel_dep(name = "bazel_skylib", version = "1.5.0", dev_dependency = True)
 bazel_dep(name = "jq.bzl", version = "0.4.0", dev_dependency = True)
 bazel_dep(name = "platforms", version = "1.0.0", dev_dependency = True)

examples/bazel-lib.patch

@@ -0,0 +1 @@
+../bazel-lib.patch

js/private/js_binary.sh.tpl

@@ -96,11 +96,16 @@ function logf_debug {
 }
 
 function resolve_execroot_bin_path {
+    if [ "${JS_BINARY__HOIST_RUNFILES_TO_EXEC_CFG:-}" ]; then
+        local bindir="$JS_BINARY__EXECROOT/$JS_BINARY__BINDIR"
+    else
+        local bindir="$JS_BINARY__EXECROOT/$BAZEL_BINDIR"
+    fi
     local short_path="$1"
     if [[ "$short_path" == ../* ]]; then
-        echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/external/${short_path:3}"
+        echo "$bindir/external/${short_path:3}"
     else
-        echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$short_path"
+        echo "$bindir/$short_path"
     fi
 }
 

js/private/js_run_binary.bzl

@@ -31,6 +31,7 @@ def js_run_binary(
         exit_code_out = None,
         silent_on_success = True,
         use_execroot_entry_point = True,
+        hoist_runfiles_to_exec_cfg = True,
         copy_srcs_to_bin = True,
         include_sources = True,
         include_types = False,
@@ -149,6 +150,13 @@ def js_run_binary(
             needed by the binary are available in the execroot output tree. This requirement can be turned off with by
             setting `allow_execroot_entry_point_with_no_copy_data_to_bin` to True.
 
+        hoist_runfiles_to_exec_cfg: Hoist runfiles to the execution platform configuration instead of the target platform
+            configuration.
+
+            This only has an effect when `use_execroot_entry_point` is True. Setting `hoist_runfiles_to_exec_cfg` to True
+            results in a more correct build by ensuring that code that is executed during a build action is built for the
+            execution platform, which may not be the same as the target platform.
+
         copy_srcs_to_bin: When True, all srcs files are copied to the output tree that are not already there.
 
         include_sources: see `js_info_files` documentation
@@ -246,6 +254,7 @@ def js_run_binary(
         fail("Use srcs instead of deps in js_run_binary: https://docs.aspect.build/rules/aspect_rules_js/docs/js_run_binary#srcs")
 
     extra_srcs = []
+    tools = []
 
     # Hoist js provider files to DefaultInfo
     make_js_info_files_target = (include_transitive_sources or
@@ -380,14 +389,19 @@ See https://github.com/aspect-build/rules_js/tree/main/docs#using-binaries-publi
             # Always propagate the testonly attribute
             testonly = kwargs.get("testonly", False),
         )
-        extra_srcs.append(":{}".format(js_runfiles_name))
+        if hoist_runfiles_to_exec_cfg:
+            fixed_env["JS_BINARY__HOIST_RUNFILES_TO_EXEC_CFG"] = "1"
+            tools.append(":{}".format(js_runfiles_name))
+        else:
+            extra_srcs.append(":{}".format(js_runfiles_name))
 
     if allow_execroot_entry_point_with_no_copy_data_to_bin:
         fixed_env["JS_BINARY__ALLOW_EXECROOT_ENTRY_POINT_WITH_NO_COPY_DATA_TO_BIN"] = "1"
 
     _run_binary(
         name = name,
         tool = tool,
+        tools = tools,
         env = fixed_env | env,
         srcs = srcs + extra_srcs,
         outs = outs + extra_outs,

js/private/test/data/BUILD.bazel

@@ -117,12 +117,12 @@ js_test(
 # TEST: js_test(data = js_run_binary(srcs)) ---------------
 js_run_binary(
     name = "run-write-srcs",
+    srcs = ["data.json"],
     outs = ["rb0.json"],
     args = [
-        "rb0.json",
-        "data.json",
+        "$(rootpath rb0.json)",
+        "$(rootpath data.json)",
     ],
-    chdir = package_name(),
     tool = ":write-js_library-srcs",
 )
 
@@ -142,12 +142,12 @@ js_test(
 # TEST: js_test(data = js_run_binary(srcs = js_library(srcs))) ----------------
 js_run_binary(
     name = "run-write-js_library-srcs",
+    srcs = ["data.json"],
     outs = ["rb1.json"],
     args = [
-        "rb1.json",
-        "data.json",
+        "$(rootpath rb1.json)",
+        "$(rootpath data.json)",
     ],
-    chdir = package_name(),
     tool = ":write-js_library-srcs",
 )
 
@@ -174,12 +174,12 @@ js_binary(
 
 js_run_binary(
     name = "run-write-js_library-data",
+    srcs = ["data.json"],
     outs = ["rb2.json"],
     args = [
-        "rb2.json",
-        "data.json",
+        "$(rootpath rb2.json)",
+        "$(rootpath data.json)",
     ],
-    chdir = package_name(),
     tool = ":write-js_library-data",
 )
 
@@ -199,12 +199,12 @@ js_test(
 # TEST: js_test(data = js_run_binary(srcs = genrule())) -----------------------
 js_run_binary(
     name = "run-write-generated",
+    srcs = ["data-generated.json"],
     outs = ["rb3.json"],
     args = [
-        "rb3.json",
-        "data-generated.json",
+        "$(rootpath rb3.json)",
+        "$(rootpath data-generated.json)",
     ],
-    chdir = package_name(),
     tool = ":write-genrule",
 )
 
@@ -224,12 +224,12 @@ js_test(
 # TEST: js_test(data = js_run_binary(srcs = genrule(srcs = filegroup))) -------
 js_run_binary(
     name = "run-write-generated-copied",
+    srcs = ["data-copied.json"],
     outs = ["rb4.json"],
     args = [
-        "rb4.json",
-        "data-copied.json",
+        "$(rootpath rb4.json)",
+        "$(rootpath data-copied.json)",
     ],
-    chdir = package_name(),
     tool = ":write-genrule-filegroup-srcs",
 )