feat: make esbuild_bundle path-mapping-friendly

.github/workflows/ci-vanilla-bazel.yaml

@@ -30,6 +30,7 @@ jobs:
           - 'e2e/smoke'
           - 'e2e/custom_version'
           - 'e2e/npm-links'
+          - 'e2e/path_mapping'
           - 'e2e/sourcemaps'
           - 'e2e/toolchain_from_source'
           - 'e2e/tsconfig'
@@ -41,3 +42,12 @@ jobs:
       - run: bazel test //...
         working-directory: ${{ matrix.folder }}
         shell: bash
+      - name: Optional ./test.sh
+        working-directory: ${{ matrix.folder }}
+        shell: bash
+        run: |
+          if [[ -f ./test.sh ]]; then
+              ./test.sh
+          else
+              echo "No test.sh in ${PWD}; skipping"
+          fi

.github/workflows/ci-workflows.yaml

@@ -36,6 +36,7 @@ jobs:
                 - { path: "e2e/bundle", slug: "e2e-bundle" }
                 - { path: "e2e/custom_version", slug: "e2e-custom_version" }
                 - { path: "e2e/npm-links", slug: "e2e-npm-links" }
+                - { path: "e2e/path_mapping", slug: "e2e-path_mapping" }
                 - { path: "e2e/sourcemaps", slug: "e2e-sourcemaps" }
                 - { path: "e2e/toolchain_from_source", slug: "e2e-toolchain_from_source" }
                 - { path: "e2e/tsconfig", slug: "e2e-tsconfig" }
@@ -53,3 +54,11 @@ jobs:
             - name: Test
               working-directory: ${{ matrix.workspace.path }}
               run: aspect test --task-key=test-${{ matrix.workspace.slug }}-${{ matrix.bazel.id }} ${{ matrix.bazel.flags }} -- //...
+            - name: Optional ./test.sh
+              working-directory: ${{ matrix.workspace.path }}
+              run: |
+                  if [[ -f ./test.sh ]]; then
+                      ./test.sh
+                  else
+                      echo "No test.sh in ${PWD}; skipping"
+                  fi

MODULE.bazel

@@ -8,7 +8,7 @@ module(
 
 # Lower-bounds (minimum) versions for direct runtime dependencies
 bazel_dep(name = "bazel_lib", version = "3.0.0")
-bazel_dep(name = "aspect_rules_js", version = "3.0.3")
+bazel_dep(name = "aspect_rules_js", version = "3.4.0")
 bazel_dep(name = "aspect_tools_telemetry", version = "0.3.3")
 bazel_dep(name = "bazel_skylib", version = "1.8.2")
 bazel_dep(name = "platforms", version = "1.0.0")

e2e/path_mapping/.bazelrc

@@ -0,0 +1,11 @@
+# Path mapping:
+# https://bazel.build/reference/command-line-reference#flag--experimental_output_paths
+common --experimental_output_paths=strip
+
+# Build without the bytes
+common:ci --remote_download_outputs=minimal
+common:ci --nobuild_runfile_links
+
+# Override the preset's `--lockfile_mode=error` on CI since we don't
+# yet commit MODULE.bazel.lock — see .gitignore.
+common:ci --lockfile_mode=off

e2e/path_mapping/.bazelversion

@@ -0,0 +1 @@
+../../.bazelversion

e2e/path_mapping/BUILD

@@ -0,0 +1,27 @@
+load("@aspect_rules_esbuild//esbuild:defs.bzl", "esbuild")
+load("@bazel_skylib//rules:build_test.bzl", "build_test")
+
+esbuild(
+    name = "bundle",
+    entry_point = "index.js",
+)
+
+# A location expansion such as $(execpath ...) can bake a real, config-specific
+# path into the action's args-file content, so this target must not advertise
+# supports-path-mapping. See test.sh.
+esbuild(
+    name = "bundle_with_location_expansion",
+    srcs = ["index.js"],
+    define = {"__PATH__": "'$(execpath index.js)'"},
+    entry_point = "index.js",
+)
+
+# Gives the `bazel test //...` step in CI something to run; the actual test
+# assertions live in test.sh, which is run as a separate CI step.
+build_test(
+    name = "bundle_test",
+    targets = [
+        ":bundle",
+        ":bundle_with_location_expansion",
+    ],
+)

e2e/path_mapping/MODULE.bazel

@@ -0,0 +1,10 @@
+module(name = "e2e_path_mapping")
+
+bazel_dep(name = "aspect_rules_esbuild", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "aspect_rules_esbuild",
+    path = "../..",
+)
+
+bazel_dep(name = "bazel_lib", version = "3.0.0", dev_dependency = True)
+bazel_dep(name = "bazel_skylib", version = "1.5.0", dev_dependency = True)

e2e/path_mapping/index.js

@@ -0,0 +1 @@
+console.log('path mapping e2e test')

e2e/path_mapping/test.sh

@@ -0,0 +1,80 @@
+#!/usr/bin/env bash
+# Proves a few things about esbuild_bundle's `supports-path-mapping` execution
+# requirement (esbuild/private/esbuild.bzl):
+#
+# 1. It lets Bazel's path mapping share a single cached action between two
+#    builds that differ only in compilation mode.
+# 2. It is advertised in the ordinary case.
+# 3. It is not advertised when doing so would be unsafe, i.e. when a location
+#    expansion in `define` produced a real, non-path-mapped path.
+#
+# A shared --disk_cache is required: -c opt and -c fastbuild are different
+# configurations, so each gets its own action instance the first time Bazel
+# visits it in a given build graph -- the incremental "did anything change"
+# check within one build never gets a chance to compare across them. Only an
+# explicit disk (or remote) cache lookup, keyed by the path-mapped action
+# digest, can serve the second build's action from the first build's result.
+set -o errexit -o nounset -o pipefail
+
+cd "$(dirname "${BASH_SOURCE[0]}")"
+
+scratch="$(mktemp -d)"
+trap 'rm -rf "$scratch"' EXIT
+disk_cache="$scratch/disk_cache"
+exec_log="$scratch/exec_log.json"
+
+# Force the esbuild action to be treated as new on every run of this script,
+# so that a prior local build of the same target/config (e.g. from an earlier
+# run of this script, or from another CI step) can't let Bazel skip it as
+# already up-to-date -- we want every build here to genuinely consult (and
+# thus prove something about) the shared --disk_cache. This is forwarded into
+# the action's environment for free since the esbuild action already sets
+# use_default_shell_env = True.
+invalidate="$(date +%s)"
+
+bazel build -c fastbuild //:bundle \
+    --disk_cache="$disk_cache" \
+    --action_env="ESBUILD_PATH_MAPPING_TEST_INVALIDATE=$invalidate"
+
+bazel build -c opt //:bundle \
+    --disk_cache="$disk_cache" \
+    --action_env="ESBUILD_PATH_MAPPING_TEST_INVALIDATE=$invalidate" \
+    --execution_log_json_file="$exec_log"
+
+matches="$(jq -s '[.[] | select(.mnemonic == "esbuild")]' "$exec_log")"
+count="$(echo "$matches" | jq 'length')"
+if [ "$count" -eq 0 ]; then
+    echo "FAIL: no esbuild entry found in the -c opt execution log" >&2
+    exit 1
+fi
+
+cache_hit="$(echo "$matches" | jq -r '.[0].cacheHit')"
+if [ "$cache_hit" != "true" ]; then
+    echo "FAIL: action was re-executed under -c opt (cacheHit=$cache_hit); path mapping did not share the cache entry from -c fastbuild" >&2
+    exit 1
+fi
+
+echo "PASS: action was cache-shared across -c fastbuild and -c opt"
+
+# We should find via bazel aquery that the action advertises path-mapping
+# support.
+aquery_output="$(bazel aquery 'mnemonic("esbuild", //:bundle)')"
+if ! echo "$aquery_output" | grep -q "supports-path-mapping"; then
+    echo "FAIL: supports-path-mapping was not advertised for //:bundle" >&2
+    echo "$aquery_output" >&2
+    exit 1
+fi
+
+echo "PASS: supports-path-mapping is advertised for //:bundle"
+
+# A location expansion in `define` (e.g. $(execpath ...)) can produce a real,
+# config-specific path that isn't safe under path mapping, so esbuild_bundle
+# must not advertise support for it in that case.
+aquery_output="$(bazel aquery 'mnemonic("esbuild", //:bundle_with_location_expansion)')"
+if echo "$aquery_output" | grep -q "supports-path-mapping"; then
+    echo "FAIL: supports-path-mapping was advertised even though define used a location expansion" >&2
+    echo "$aquery_output" >&2
+    exit 1
+fi
+
+echo "PASS: path mapping is not advertised when define uses a location expansion"

esbuild/BUILD.bazel

@@ -57,4 +57,7 @@ bzl_library(
     name = "toolchain",
     srcs = ["toolchain.bzl"],
     visibility = ["//visibility:public"],
+    deps = [
+        "//esbuild/private:helpers",
+    ],
 )

esbuild/private/esbuild.bzl

@@ -1,10 +1,10 @@
 "# esbuild rule"
 
-load("@aspect_rules_js//js:libs.bzl", "js_lib_constants", "js_lib_helpers")
+load("@aspect_rules_js//js:libs.bzl", "js_binary_lib", "js_lib_constants", "js_lib_helpers")
 load("@aspect_rules_js//js:providers.bzl", "JsInfo", "js_info")
 load("@bazel_lib//lib:copy_to_bin.bzl", "COPY_FILE_TO_BIN_TOOLCHAINS", "copy_file_to_bin_action", "copy_files_to_bin_actions")
 load("@bazel_lib//lib:expand_make_vars.bzl", "expand_variables")
-load(":helpers.bzl", "desugar_entry_point_names", "write_args_file")
+load(":helpers.bzl", "LauncherKindInfo", "desugar_entry_point_names", "launcher_kind_aspect", "write_args_file")
 
 _ATTRS = {
     "args_file": attr.label(
@@ -76,6 +76,7 @@ See https://esbuild.github.io/api/#format for more details
         executable = True,
         doc = "Override the default esbuild wrapper, which is supplied by the esbuild toolchain",
         cfg = "exec",
+        aspects = [launcher_kind_aspect],
     ),
     "max_threads": attr.int(
         mandatory = False,
@@ -256,15 +257,17 @@ def _esbuild_impl(ctx):
     entry_points_bin_copy = copy_files_to_bin_actions(ctx, entry_points)
     tsconfig_bin_copy = copy_file_to_bin_action(ctx, ctx.file.tsconfig)
 
+    can_path_map = True
+    define = {}
+    for k, v in ctx.attr.define.items():
+        expanded_v = expand_variables(ctx, ctx.expand_location(v), attribute_name = "define")
+        if expanded_v != v:
+            can_path_map = False
+        define[k] = expanded_v
+
     args = dict({
         "bundle": ctx.attr.bundle,
-        "define": dict([
-            [
-                k,
-                expand_variables(ctx, ctx.expand_location(v), attribute_name = "define"),
-            ]
-            for k, v in ctx.attr.define.items()
-        ]),
+        "define": define,
         "entryPoints": [_bin_relative_path(ctx, entry_point) for entry_point in entry_points_bin_copy],
         "external": ctx.attr.external,
         "logLevel": ctx.attr.esbuild_log_level,
@@ -338,7 +341,6 @@ def _esbuild_impl(ctx):
         output_sources.append(extra_dir)
 
     env = {
-        "BAZEL_BINDIR": ctx.bin_dir.path,
         "ESBUILD_BINARY_PATH": esbuild_toolinfo.target_tool_path,
     }
 
@@ -351,9 +353,18 @@ def _esbuild_impl(ctx):
     for log_level_env in js_lib_helpers.envs_for_log_level(ctx.attr.js_log_level):
         env[log_level_env] = "1"
 
+    if ctx.executable.launcher:
+        launcher_files_to_run = ctx.attr.launcher[DefaultInfo].files_to_run
+        launcher_is_js_binary = ctx.attr.launcher[LauncherKindInfo].is_js_binary
+    else:
+        launcher_files_to_run = esbuild_toolinfo.launcher.files_to_run
+        launcher_is_js_binary = esbuild_toolinfo.launcher[LauncherKindInfo].is_js_binary
+
     execution_requirements = {}
     if "no-remote-exec" in ctx.attr.tags:
-        execution_requirements = {"no-remote-exec": "1"}
+        execution_requirements["no-remote-exec"] = "1"
+    if can_path_map and launcher_is_js_binary:
+        execution_requirements["supports-path-mapping"] = "1"
 
     # setup the args passed to the launcher
     launcher_args = ctx.actions.args()
@@ -406,23 +417,42 @@ def _esbuild_impl(ctx):
         )],
     )
 
-    launcher = ctx.executable.launcher or esbuild_toolinfo.launcher.files_to_run
-    ctx.actions.run(
-        inputs = input_sources,
-        outputs = output_sources,
-        arguments = [launcher_args],
-        progress_message = "%s Javascript %s [esbuild]" % ("Bundling" if not ctx.attr.output_dir else "Splitting", " ".join([_bin_relative_path(ctx, entry_point) for entry_point in entry_points])),
-        execution_requirements = execution_requirements,
-        mnemonic = "esbuild",
-        env = env,
-        use_default_shell_env = True,
-        executable = launcher,
-    )
+    progress_message = "%s Javascript %s [esbuild]" % ("Bundling" if not ctx.attr.output_dir else "Splitting", " ".join([_bin_relative_path(ctx, entry_point) for entry_point in entry_points]))
+
+    if launcher_is_js_binary:
+        # run_binary_action() invokes the js_binary launcher in a path-mapping-friendly way.
+        js_binary_lib.run_binary_action(
+            ctx,
+            inputs = input_sources,
+            outputs = output_sources,
+            arguments = [launcher_args],
+            progress_message = progress_message,
+            execution_requirements = execution_requirements,
+            mnemonic = "esbuild",
+            env = env,
+            use_default_shell_env = True,
+            executable = launcher_files_to_run,
+        )
+    else:
+        # The launcher is not a js_binary, so we must explicitly set the BAZEL_BINDIR environment
+        # variable, which is not path-mapping-friendly.
+        env["BAZEL_BINDIR"] = ctx.bin_dir.path
+        ctx.actions.run(
+            inputs = input_sources,
+            outputs = output_sources,
+            arguments = [launcher_args],
+            progress_message = progress_message,
+            execution_requirements = execution_requirements,
+            mnemonic = "esbuild",
+            env = env,
+            use_default_shell_env = True,
+            executable = ctx.executable.launcher or launcher_files_to_run,
+        )
 
     output_sources_depset = depset(output_sources)
 
     if ctx.attr.bundle:
-        # When bundling don't propogate any transitive sources or declarations since sources
+        # When bundling don't propagate any transitive sources or declarations since sources
         # are typically bundled into the output.
         transitive_sources = output_sources_depset
         transitive_types = depset()

esbuild/private/helpers.bzl

@@ -6,6 +6,21 @@ TS_EXTENSIONS = ["ts", "tsx"]
 JS_EXTENSIONS = ["js", "jsx", "mjs"]
 ALLOWED_EXTENSIONS = JS_EXTENSIONS + TS_EXTENSIONS
 
+LauncherKindInfo = provider(
+    doc = "Internal use only. Reports the rule kind of a launcher target.",
+    fields = {
+        "is_js_binary": "True if the target's underlying rule is js_binary.",
+    },
+)
+
+def _launcher_kind_aspect_impl(target, ctx):
+    return [LauncherKindInfo(is_js_binary = ctx.rule.kind == "js_binary")]
+
+# Detects whether a launcher target is a js_binary.
+launcher_kind_aspect = aspect(
+    implementation = _launcher_kind_aspect_impl,
+)
+
 def desugar_entry_point_names(entry_point, entry_points):
     """Users can specify entry_point (sugar) or entry_points (long form).
 

esbuild/private/plugins/bazel-sandbox.js

@@ -4,6 +4,20 @@ const process = require('process')
 const bindir = process.env.BAZEL_BINDIR
 const execroot = process.env.JS_BINARY__EXECROOT
 
+// Matches the bazel-out/<config>/bin segment of an absolute path. Used to detect and strip a
+// bindir prefix instead of matching BAZEL_BINDIR exactly, because under Bazel's path-mapping
+// feature BAZEL_BINDIR may hold a generic mapped placeholder (e.g. "bazel-out/cfg/bin") for
+// cache-sharing purposes, rather than the real per-config value (e.g.
+// "bazel-out/k8-fastbuild/bin") -- but once esbuild follows a symlink out of the sandbox and
+// node resolves it to a real absolute path, that path always contains the *real* bindir segment,
+// never the mapped one. The path is then reconstructed using `bindir` (see below), since that's
+// the name the mapped sandbox's own directory tree actually uses on disk for this action.
+//
+// Matches both `/` and `\` as separators: BAZEL_BINDIR (from Bazel's Starlark-internal path
+// representation) is always forward-slash, but a real resolved path on native Windows uses
+// backslashes (see the startsWith('\\') check below for the same reason).
+const BAZEL_OUT_BINDIR_RE = /bazel-out[\\/][^\\/]+[\\/]bin[\\/]/
+
 // Under Bazel, esbuild will follow symlinks out of the sandbox when the sandbox is enabled. See https://github.com/aspect-build/rules_esbuild/issues/58.
 // This plugin using a separate resolver to detect if the the resolution has left the execroot (which is the root of the sandbox
 // when sandboxing is enabled) and patches the resolution back into the sandbox.
@@ -78,16 +92,18 @@ function correctImportPath(result, otherOptions, firstEntry) {
     }
 
     // If it tried to leave bazel-bin, error out completely.
-    if (!result.path.includes(bindir)) {
+    const bindirMatch = BAZEL_OUT_BINDIR_RE.exec(result.path)
+    if (!bindirMatch) {
       throw new Error(
-        `Error: esbuild resolved a path outside of BAZEL_BINDIR (${bindir}): ${result.path}`
+        `Error: esbuild resolved a path outside of bazel-out/*/bin: ${result.path}`
       )
     }
-    // Otherwise remap the bindir-relative path
-    const correctedPath = path.join(
-      execroot,
-      result.path.substring(result.path.indexOf(bindir))
+    // Otherwise remap the bindir-relative path, reconstructed under this action's actual
+    // (possibly path-mapped) bindir rather than the real one baked into `result.path`.
+    const relativeToBindir = result.path.substring(
+      bindirMatch.index + bindirMatch[0].length
     )
+    const correctedPath = path.join(execroot, bindir, relativeToBindir)
     if (!!process.env.JS_BINARY__LOG_DEBUG) {
       console.error(
         `DEBUG: [bazel-sandbox] correcting esbuild resolution ${result.path} that left the sandbox to ${correctedPath}.`