diff --git a/.aspect/workflows/config.yaml b/.aspect/workflows/config.yaml index ffb5c70b24..2643a10caa 100644 --- a/.aspect/workflows/config.yaml +++ b/.aspect/workflows/config.yaml @@ -36,6 +36,8 @@ workspaces: # Still depends on some WORKSPACE setup - bazel-9: without: true + - bazel-9-exec: + without: true e2e/nextjs: icon: nextjs tasks: *e2e_tasks @@ -123,7 +125,17 @@ workspaces: tasks: *e2e_tasks e2e/repo_mapping: icon: js - tasks: *e2e_tasks + tasks: + - test: + queue: aspect-medium + - format: + without: true + - buildifier: + without: true + # We need to pass a flag that begins with @aspect_rules_js//, but + # that repo is given a different name in e2e/repo_mapping. + - bazel-9-exec: + without: true e2e/output_paths: icon: js tasks: *e2e_tasks @@ -173,6 +185,15 @@ tasks: # TODO: change this back to 9.x once this bug is fixed: # https://github.com/bazelbuild/bazel/issues/29393 USE_BAZEL_VERSION: '9.0.2' + - test: + name: Bazel 9 with exec cfg + id: bazel-9-exec + bazel: + flags: ['--test_tag_filters=-skip-on-bazel9', '--@aspect_rules_js//js:use_execroot_exec_cfg=true'] + env: + # TODO: change this back to 9.x once this bug is fixed: + # https://github.com/bazelbuild/bazel/issues/29393 + USE_BAZEL_VERSION: '9.0.2' - format: queue: aspect-medium - buildifier: diff --git a/MODULE.bazel b/MODULE.bazel index 493e46ff0b..3aaf1f18c1 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -19,7 +19,7 @@ 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") +bazel_dep(name = "bazel_lib", version = "3.3.1") # 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") diff --git a/contrib/nextjs/defs.bzl b/contrib/nextjs/defs.bzl index e6e1e216ae..732b61d38c 100644 --- a/contrib/nextjs/defs.bzl +++ b/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}", + use_execroot_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}", + use_execroot_exec_cfg = False, **kwargs ) diff --git a/examples/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= b/examples/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= index cf3c2845e0..432032fa13 100755 --- a/examples/.aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU= +++ b/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 diff --git a/js/BUILD.bazel b/js/BUILD.bazel index 7fab92fbbe..043bed5014 100644 --- a/js/BUILD.bazel +++ b/js/BUILD.bazel @@ -1,6 +1,20 @@ "Public API" load("@bazel_skylib//:bzl_library.bzl", "bzl_library") +load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") + +# This flag controls the default value of the use_execroot_exec_cfg flag on +# js_run_binary. +bool_flag( + name = "use_execroot_exec_cfg", + build_setting_default = False, + visibility = ["//visibility:public"], +) + +config_setting( + name = "_use_execroot_exec_cfg_true", + flag_values = {"use_execroot_exec_cfg": "True"}, +) bzl_library( name = "defs", diff --git a/js/private/js_binary.sh.tpl b/js/private/js_binary.sh.tpl index ad99a00047..4a8f3d6da1 100644 --- a/js/private/js_binary.sh.tpl +++ b/js/private/js_binary.sh.tpl @@ -97,10 +97,18 @@ function logf_debug { function resolve_execroot_bin_path { local short_path="$1" + local suffix if [[ "$short_path" == ../* ]]; then - echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/external/${short_path:3}" + suffix="external/${short_path:3}" else - echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$short_path" + suffix="$short_path" + fi + local exec_path="$JS_BINARY__EXECROOT/$JS_BINARY__BINDIR/$suffix" + local target_path="$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$suffix" + if [ -e "$exec_path" ]; then + echo "$exec_path" + else + echo "$target_path" fi } diff --git a/js/private/js_run_binary.bzl b/js/private/js_run_binary.bzl index 844da7f4c1..b356e97524 100644 --- a/js/private/js_run_binary.bzl +++ b/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, + use_execroot_exec_cfg = None, copy_srcs_to_bin = True, include_sources = True, include_types = False, @@ -149,6 +150,17 @@ 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. + use_execroot_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 `use_execroot_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. + + When set to `None` (the default), the value of the internal `//js:use_execroot_exec_cfg` build flag is used. That + flag is for internal testing only and should not be used. It is temporary and will be removed once the default is + changed to `True`. + 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 +258,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,7 +393,21 @@ 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)) + runfiles_label = ":{}".format(js_runfiles_name) + if use_execroot_exec_cfg: + tools.append(runfiles_label) + elif use_execroot_exec_cfg == False: + extra_srcs.append(runfiles_label) + else: + # use_execroot_exec_cfg is None, so we will defer to the flag. + tools = tools + select({ + "@aspect_rules_js//js:_use_execroot_exec_cfg_true": [runfiles_label], + "//conditions:default": [], + }) + extra_srcs = extra_srcs + select({ + "@aspect_rules_js//js:_use_execroot_exec_cfg_true": [], + "//conditions:default": [runfiles_label], + }) 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" @@ -388,6 +415,7 @@ See https://github.com/aspect-build/rules_js/tree/main/docs#using-binaries-publi _run_binary( name = name, tool = tool, + tools = tools, env = fixed_env | env, srcs = srcs + extra_srcs, outs = outs + extra_outs, diff --git a/js/private/test/data/BUILD.bazel b/js/private/test/data/BUILD.bazel index 814afb8ed9..845e04f714 100644 --- a/js/private/test/data/BUILD.bazel +++ b/js/private/test/data/BUILD.bazel @@ -117,6 +117,7 @@ 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", @@ -142,6 +143,7 @@ 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", @@ -174,6 +176,7 @@ js_binary( js_run_binary( name = "run-write-js_library-data", + srcs = ["data.json"], outs = ["rb2.json"], args = [ "rb2.json", @@ -199,6 +202,7 @@ 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", @@ -224,6 +228,7 @@ 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", diff --git a/js/private/test/image/asserts.bzl b/js/private/test/image/asserts.bzl index 87e42207cc..7b988f29ae 100644 --- a/js/private/test/image/asserts.bzl +++ b/js/private/test/image/asserts.bzl @@ -6,7 +6,7 @@ load("//js:defs.bzl", "js_image_layer") # buildifier: disable=function-docstring def assert_tar_listing(name, actual, expected): # Either of these two file sizes may be observed on a file like /js/private/test/image/bin - sanitize_cmd = "sed -E 's/239[0-9]{2}|24[0-9]{3}/xxxxx/g'" + sanitize_cmd = "sed -E 's/250[0-9]{2}|24[0-9]{3}/xxxxx/g'" actual_listing = "_{}_listing".format(name) native.genrule( name = actual_listing, diff --git a/js/private/test/js_run_binary/BUILD.bazel b/js/private/test/js_run_binary/BUILD.bazel new file mode 100644 index 0000000000..d163d3977d --- /dev/null +++ b/js/private/test/js_run_binary/BUILD.bazel @@ -0,0 +1,145 @@ +load("@bazel_lib//lib:diff_test.bzl", "diff_test") +load("@bazel_lib//lib:testing.bzl", "assert_contains") +load("@bazel_lib//lib:transitions.bzl", "platform_transition_filegroup") +load("@bazel_skylib//rules:write_file.bzl", "write_file") +load("//js:defs.bzl", "js_binary", "js_run_binary") + +# Verify that the js_binary tool's data dependencies are built in the exec configuration +# when use_execroot_exec_cfg is True. +# To do this we check that js_run_binary agrees with genrule on the location of a +# tool dependency. +write_file( + name = "gen_cfg_probe", + out = "cfg_probe.txt", + content = ["probe"], +) + +js_binary( + name = "find_cfg_probe", + data = [":cfg_probe.txt"], + entry_point = "find_cfg_probe.mjs", +) + +js_run_binary( + name = "tools_cfg_location", + stdout = "tools_cfg_location.txt", + tool = ":find_cfg_probe", + use_execroot_exec_cfg = True, +) + +genrule( + name = "expected_tools_cfg_location", + outs = ["expected_tools_cfg_location.txt"], + cmd = "echo $(execpath :cfg_probe.txt) > $@", + tools = [":cfg_probe.txt"], +) + +diff_test( + name = "tools_exec_cfg_test", + file1 = ":tools_cfg_location.txt", + file2 = ":expected_tools_cfg_location.txt", +) + +# Verify the opposite: with use_execroot_exec_cfg = False, the tool's data +# dependencies are built in the target configuration, matching a genrule srcs path. +js_run_binary( + name = "target_cfg_location", + stdout = "target_cfg_location.txt", + tool = ":find_cfg_probe", + use_execroot_exec_cfg = False, +) + +genrule( + name = "expected_target_cfg_location", + srcs = [":cfg_probe.txt"], + outs = ["expected_target_cfg_location.txt"], + cmd = "echo $(execpath :cfg_probe.txt) > $@", +) + +diff_test( + name = "target_cfg_test", + file1 = ":target_cfg_location.txt", + file2 = ":expected_target_cfg_location.txt", +) + +# Verify that dependencies passed via the srcs parameter are always built for +# the target platform, unlike the tool parameter and its dependencies. +# We use a select() on the OS to produce a platform-specific file, then +# transition to three different target platforms and assert the contents match. +write_file( + name = "gen_os_name", + out = "os_name.txt", + content = select({ + "@platforms//os:linux": ["linux"], + "@platforms//os:macos": ["macos"], + "@platforms//os:windows": ["windows"], + "//conditions:default": ["other"], + }), + tags = ["manual"], +) + +platform( + name = "linux", + constraint_values = ["@platforms//os:linux"], +) + +platform( + name = "macos", + constraint_values = ["@platforms//os:macos"], +) + +platform( + name = "windows", + constraint_values = ["@platforms//os:windows"], +) + +js_binary( + name = "read_file", + entry_point = "read_file.mjs", +) + +js_run_binary( + name = "gen_target_os", + srcs = [":os_name.txt"], + args = ["$(rootpath :os_name.txt)"], + stdout = "target_os.txt", + tags = ["manual"], + tool = ":read_file", + use_execroot_exec_cfg = True, +) + +platform_transition_filegroup( + name = "target_os_linux", + srcs = [":gen_target_os"], + target_platform = ":linux", +) + +platform_transition_filegroup( + name = "target_os_macos", + srcs = [":gen_target_os"], + target_platform = ":macos", +) + +platform_transition_filegroup( + name = "target_os_windows", + srcs = [":gen_target_os"], + target_platform = ":windows", +) + +assert_contains( + name = "linux_target_platform_test", + actual = ":target_os_linux", + expected = "linux", +) + +assert_contains( + name = "macos_target_platform_test", + actual = ":target_os_macos", + expected = "macos", +) + +assert_contains( + name = "windows_target_platform_test", + actual = ":target_os_windows", + expected = "windows", +) diff --git a/js/private/test/js_run_binary/find_cfg_probe.mjs b/js/private/test/js_run_binary/find_cfg_probe.mjs new file mode 100644 index 0000000000..5acea9a483 --- /dev/null +++ b/js/private/test/js_run_binary/find_cfg_probe.mjs @@ -0,0 +1,32 @@ +// The purpose of this binary is to determine where the generated file +// cfg_probe.txt ended up. We look for it in both the exec and target bin +// directories and output the path where it appears. +import fs from 'fs' +import path from 'path' + +const suffix = 'js/private/test/js_run_binary/cfg_probe.txt' +const execCfgRelPath = path.join(process.env.JS_BINARY__BINDIR, suffix) +const targetCfgRelPath = path.join(process.env.BAZEL_BINDIR, suffix) + +function fileExists(absPath) { + try { + fs.lstatSync(absPath) + return true + } catch (_) { + return false + } +} + +const foundInExecCfg = fileExists(path.join(process.env.JS_BINARY__EXECROOT, execCfgRelPath)) +const foundInTargetCfg = fileExists(path.join(process.env.JS_BINARY__EXECROOT, targetCfgRelPath)) + +if (foundInExecCfg && foundInTargetCfg) { + process.stderr.write('cfg_probe.txt unexpectedly found in both exec cfg and target cfg paths\n') + process.exit(1) +} +if (!foundInExecCfg && !foundInTargetCfg) { + process.stderr.write('cfg_probe.txt not found in either exec cfg or target cfg path\n') + process.exit(1) +} + +process.stdout.write((foundInExecCfg ? execCfgRelPath : targetCfgRelPath) + '\n') diff --git a/js/private/test/js_run_binary/read_file.mjs b/js/private/test/js_run_binary/read_file.mjs new file mode 100644 index 0000000000..46b7ecae70 --- /dev/null +++ b/js/private/test/js_run_binary/read_file.mjs @@ -0,0 +1,4 @@ +import fs from 'fs' + +const content = fs.readFileSync(process.argv[2], 'utf8').trim() +process.stdout.write(content + '\n') diff --git a/js/private/test/snapshots/launcher.sh b/js/private/test/snapshots/launcher.sh index 7fdf331a76..5386f31b2f 100644 --- a/js/private/test/snapshots/launcher.sh +++ b/js/private/test/snapshots/launcher.sh @@ -108,10 +108,18 @@ function logf_debug { function resolve_execroot_bin_path { local short_path="$1" + local suffix if [[ "$short_path" == ../* ]]; then - echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/external/${short_path:3}" + suffix="external/${short_path:3}" else - echo "$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$short_path" + suffix="$short_path" + fi + local exec_path="$JS_BINARY__EXECROOT/$JS_BINARY__BINDIR/$suffix" + local target_path="$JS_BINARY__EXECROOT/${BAZEL_BINDIR:-$JS_BINARY__BINDIR}/$suffix" + if [ -e "$exec_path" ]; then + echo "$exec_path" + else + echo "$target_path" fi }