Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .aspect/workflows/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,15 @@ workspaces:
tasks: *e2e_tasks
e2e/repo_mapping:
icon: js
tasks: *e2e_tasks
tasks:
- test:
queue: aspect-medium
- format:
without: true
- buildifier:
without: true
- bazel-7:
Comment thread
acozzette marked this conversation as resolved.
Outdated
without: true
e2e/output_paths:
icon: js
tasks: *e2e_tasks
Expand Down Expand Up @@ -154,7 +162,7 @@ tasks:
name: Bazel 7
id: bazel-7
bazel:
flags: ['--test_tag_filters=-skip-on-bazel7']
flags: ['--test_tag_filters=-skip-on-bazel7', '--@aspect_rules_js//js:_hoist_runfiles_to_exec_cfg=true']
Comment thread
acozzette marked this conversation as resolved.
Outdated
env:
USE_BAZEL_VERSION: '7.x'
- test:
Expand Down
2 changes: 1 addition & 1 deletion MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment just like this for the reason we need 3.3.0?

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")
Expand Down
2 changes: 2 additions & 0 deletions contrib/nextjs/defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions js/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
"Public API"

load("@bazel_skylib//:bzl_library.bzl", "bzl_library")
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")

# Underscore prefix signals this is internal — do not rely on it.
# This controls the default value of the hoist_runfiles_to_exec_cfg flag on
# js_run_binary, and is for CI testing only.
bool_flag(
name = "_hoist_runfiles_to_exec_cfg",
build_setting_default = False,
visibility = ["//visibility:public"],
)

config_setting(
name = "_hoist_runfiles_to_exec_cfg_true",
flag_values = {"_hoist_runfiles_to_exec_cfg": "True"},
)

bzl_library(
name = "defs",
Expand Down
12 changes: 10 additions & 2 deletions js/private/js_binary.sh.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just modify short_parth instead of introducing the extra suffix var? Any disadvantage to that?

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
}

Expand Down
30 changes: 29 additions & 1 deletion js/private/js_run_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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 = None,
Comment thread
jbedard marked this conversation as resolved.
Outdated
copy_srcs_to_bin = True,
include_sources = True,
include_types = False,
Expand Down Expand Up @@ -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.

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.

When set to `None` (the default), the value of the internal `//js:_hoist_runfiles_to_exec_cfg` build flag
Comment thread
jbedard marked this conversation as resolved.
Outdated
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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -380,14 +393,29 @@ 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 hoist_runfiles_to_exec_cfg:
tools.append(runfiles_label)
elif hoist_runfiles_to_exec_cfg == False:
extra_srcs.append(runfiles_label)
else:
# hoist_runfiles_to_exec_cfg is None, so we will defer to the flag.
tools = tools + select({
"@aspect_rules_js//js:_hoist_runfiles_to_exec_cfg_true": [runfiles_label],
"//conditions:default": [],
})
extra_srcs = extra_srcs + select({
"@aspect_rules_js//js:_hoist_runfiles_to_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"

_run_binary(
name = name,
tool = tool,
tools = tools,
Comment thread
acozzette marked this conversation as resolved.
env = fixed_env | env,
srcs = srcs + extra_srcs,
outs = outs + extra_outs,
Expand Down
30 changes: 15 additions & 15 deletions js/private/test/data/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,12 @@ js_test(
# TEST: js_test(data = js_run_binary(srcs)) ---------------
js_run_binary(
name = "run-write-srcs",
srcs = ["data.json"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was essentially an undeclared input before, but magically worked because the tool also declared the input? Now it correctly fails with the new option enabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. With the new option enabled and without this line adding the dep via srcs, this would fail since data.json would no longer be in the same place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds correct to me 👍

outs = ["rb0.json"],
args = [
"rb0.json",
"data.json",
"$(rootpath rb0.json)",
"$(rootpath data.json)",
],
chdir = package_name(),
Comment thread
acozzette marked this conversation as resolved.
tool = ":write-js_library-srcs",
)

Expand All @@ -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",
)

Expand All @@ -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",
)

Expand All @@ -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",
)

Expand All @@ -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",
)

Expand Down
2 changes: 1 addition & 1 deletion js/private/test/image/asserts.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading