Skip to content

Commit 1860a6a

Browse files
committed
refactor: remove bootstrap attribute & fix $(location) expansions in nodejs_binary templated_args
BREAKING CHANGE: bootstrap attribute in nodejs_binary, nodejs_test & jasmine_node_test removed This can be replaced with the `--node_options=--require=$(location label)` argument such as, ``` nodejs_test( name = "bootstrap_test", templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"], entry_point = ":bootstrap.spec.js", data = ["bootstrap.js"], ) ``` or ``` jasmine_node_test( name = "bootstrap_test", srcs = ["bootstrap.spec.js"], templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"], data = ["bootstrap.js"], ) ``` `templated_args` `$(location)` and `$(locations)` are now correctly expanded when there is no space before ` $(location` such as `templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"]`. Path is returned in runfiles manifest path format such as `repo/path/to/file`. This differs from how $(location) and $(locations) expansion behaves in expansion the `args` attribute of a *_binary or *_test which returns the runfiles short path of the format `./path/to/file` for user repo and `../external_repo/path/to/file` for external repositories. We may change this behavior in the future with $(mlocation) and $(mlocations) used to expand to the runfiles manifest path. See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.
1 parent f938ab7 commit 1860a6a

12 files changed

Lines changed: 161 additions & 81 deletions

File tree

BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ exports_files([
3333
"common.bazelrc",
3434
"tsconfig.json",
3535
"package.json",
36+
"bootstrap.js",
3637
])
3738

3839
bzl_library(

bootstrap.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
console.log('here')
2+
global.bootstrapped = true;

e2e/jasmine/BUILD.bazel

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,19 @@ jasmine_node_test(
55
srcs = ["test.spec.js"],
66
)
77

8+
# This requires the linker as jasmine_shared_env_bootstrap.js requires @bazel/jasmine
9+
# which only works with the linker as the --require script doesn't get the require.resolve
10+
# patches.
811
jasmine_node_test(
912
name = "shared_env_test",
1013
srcs = ["jasmine_shared_env_test.spec.js"],
11-
bootstrap = ["e2e_jasmine/jasmine_shared_env_bootstrap.js"],
1214
data = ["jasmine_shared_env_bootstrap.js"],
15+
# TODO(gregmagolan): fix Windows error: Error: Cannot find module 'e2e_jasmine/shared_env_test_devmode_srcs.MF'
16+
tags = ["fix-windows"],
17+
templated_args = [
18+
"--nobazel_patch_module_resolver",
19+
"--node_options=--require=$(rlocation $(location :jasmine_shared_env_bootstrap.js))",
20+
],
1321
deps = [
1422
"@npm//jasmine",
1523
"@npm//jasmine-core",

internal/common/expand_into_runfiles.bzl

Lines changed: 72 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -15,63 +15,82 @@
1515
"""Helper functions to expand paths into runfiles
1616
"""
1717

18-
def expand_location_into_runfiles(ctx, path):
19-
"""Expand a path into runfiles if it contains a $(location).
20-
21-
If the path has a location expansion, expand it. Otherwise return as-is.
18+
# Expand $(location) and $(locations) to runfiles manifest path
19+
def _expand_mlocations(ctx, input, targets):
20+
paths = ctx.expand_location(input, targets)
21+
return " ".join([_short_path_to_runfiles_manifest_path(ctx, p, targets) for p in paths.split(" ")])
22+
23+
# Convert a short_path in the execroot to the runfiles manifest path
24+
def _short_path_to_runfiles_manifest_path(ctx, path, targets):
25+
if path.startswith("../"):
26+
return path[len("../"):]
27+
if path.startswith("./"):
28+
path = path[len("./"):]
29+
elif path.startswith(ctx.bin_dir.path):
30+
path = path[len(ctx.bin_dir.path + "/"):]
31+
elif path.startswith(ctx.genfiles_dir.path):
32+
path = path[len(ctx.genfiles_dir.path + "/"):]
33+
return ctx.workspace_name + "/" + path
34+
35+
# Expand $(location) and $(locations) to runfiles short path
36+
def _expand_locations(ctx, input, targets):
37+
paths = ctx.expand_location(input, targets)
38+
return " ".join([_short_path_to_runfiles_short_path(ctx, p, targets) for p in paths.split(" ")])
39+
40+
# Convert a short_path in the execroot to the runfiles short path
41+
def _short_path_to_runfiles_short_path(ctx, path, targets):
42+
path = path.replace(ctx.bin_dir.path + "/external/", "../", 1)
43+
path = path.replace(ctx.bin_dir.path + "/", "", 1)
44+
path = path.replace(ctx.genfiles_dir.path + "/external/", "../", 1)
45+
path = path.replace(ctx.genfiles_dir.path + "/", "", 1)
46+
return path
47+
48+
def expand_location_into_runfiles(ctx, input, targets = []):
49+
"""Expands all $(location ...) templates in the given string by replacing $(location //x) with the path
50+
in runfiles of the output file of target //x. Expansion only works for labels that point to direct dependencies
51+
of this rule or that are explicitly listed in the optional argument targets.
52+
53+
Path is returned in runfiles manifest path format such as `repo/path/to/file`. This differs from how $(location)
54+
and $(locations) expansion behaves in expansion the `args` attribute of a *_binary or *_test which returns
55+
the runfiles short path of the format `./path/to/file` for user repo and `../external_repo/path/to/file` for external
56+
repositories. We may change this behavior in the future with $(mlocation) and $(mlocations) used to expand
57+
to the runfiles manifest path.
58+
See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.
2259
2360
Args:
2461
ctx: context
25-
path: the path to expand
62+
input: String to be expanded
63+
targets: List of targets for additional lookup information.
2664
2765
Returns:
2866
The expanded path or the original path
2967
"""
30-
if path.find("$(location") < 0:
31-
return path
32-
return expand_path_into_runfiles(ctx, path)
33-
34-
# TODO(gregmagolan): rename to _expand_path_into_runfiles after angular/angular protractor rule
35-
# is removed and no longer references this function
36-
def expand_path_into_runfiles(ctx, path):
37-
"""Expand paths into runfiles.
38-
39-
Given a file path that might contain a $(location) or $(locations) label expansion,
40-
provide the paths to the file in runfiles.
41-
42-
See https://docs.bazel.build/versions/master/skylark/lib/ctx.html#expand_location
43-
44-
Args:
45-
ctx: context
46-
path: the paths to expand
47-
48-
Returns:
49-
The expanded paths
50-
"""
51-
targets = ctx.attr.data if hasattr(ctx.attr, "data") else []
52-
expanded = ctx.expand_location(path, targets)
53-
54-
expansion = [_resolve_expanded_path(ctx, exp) for exp in expanded.strip().split(" ")]
55-
56-
return " ".join(expansion)
57-
58-
def _resolve_expanded_path(ctx, expanded):
59-
"""Resolves an expanded path
60-
61-
Given a file path that has been expaned with $(location), resolve the path to include the workspace name,
62-
handling when that path is within bin_dir or gen_fir
63-
64-
Args:
65-
ctx: context
66-
expanded: the expanded path to resolve
67-
68-
Returns:
69-
The resolved path
70-
"""
71-
if expanded.startswith("../"):
72-
return expanded[len("../"):]
73-
if expanded.startswith(ctx.bin_dir.path):
74-
expanded = expanded[len(ctx.bin_dir.path + "/"):]
75-
if expanded.startswith(ctx.genfiles_dir.path):
76-
expanded = expanded[len(ctx.genfiles_dir.path + "/"):]
77-
return ctx.workspace_name + "/" + expanded
68+
target = "@%s//%s:%s" % (ctx.workspace_name, "/".join(ctx.build_file_path.split("/")[:-1]), ctx.attr.name)
69+
70+
# Loop through input an expand all $(location) and $(locations) using _expand_to_mlocation()
71+
path = ""
72+
length = len(input)
73+
last = 0
74+
for i in range(length):
75+
if (input[i:i + 12] == "$(mlocation ") or (input[i:i + 13] == "$(mlocations "):
76+
j = input.find(")", i) + 1
77+
if (j == 0):
78+
fail("invalid $(mlocation) expansion in string \"%s\" part of target %s" % (input, target))
79+
path += input[last:i]
80+
path += _expand_mlocations(ctx, "$(" + input[i + 3:j], targets)
81+
last = j
82+
i = j
83+
if (input[i:i + 11] == "$(location ") or (input[i:i + 12] == "$(locations "):
84+
j = input.find(")", i) + 1
85+
if (j == 0):
86+
fail("invalid $(location) expansion in string \"%s\" part of target %s" % (input, target))
87+
path += input[last:i]
88+
89+
# TODO(gmagolan): flip to _expand_locations in the future so $(location) expands to runfiles short
90+
# path which is more Bazel idiomatic and $(mlocation) can be used for runfiles manifest path
91+
path += _expand_mlocations(ctx, input[i:j], targets)
92+
last = j
93+
i = j
94+
path += input[last:]
95+
96+
return path

internal/node/node.bzl

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,6 @@ def _write_loader_script(ctx):
109109
output = ctx.outputs.loader,
110110
substitutions = {
111111
"TEMPLATED_bin_dir": ctx.bin_dir.path,
112-
"TEMPLATED_bootstrap": "\n " + ",\n ".join(
113-
["\"" + d + "\"" for d in ctx.attr.bootstrap],
114-
),
115112
"TEMPLATED_entry_point": entry_point_path,
116113
"TEMPLATED_gen_dir": ctx.genfiles_dir.path,
117114
"TEMPLATED_install_source_map_support": str(ctx.attr.install_source_map_support).lower(),
@@ -219,7 +216,7 @@ def _nodejs_binary_impl(ctx):
219216
# Put the params into the params file
220217
ctx.actions.write(
221218
output = ctx.outputs.templated_args_file,
222-
content = "\n".join([expand_location_into_runfiles(ctx, p) for p in params]),
219+
content = "\n".join([expand_location_into_runfiles(ctx, p, ctx.attr.data) for p in params]),
223220
is_executable = False,
224221
)
225222

@@ -233,7 +230,7 @@ def _nodejs_binary_impl(ctx):
233230

234231
substitutions = {
235232
"TEMPLATED_args": " ".join([
236-
expand_location_into_runfiles(ctx, a)
233+
expand_location_into_runfiles(ctx, a, ctx.attr.data)
237234
for a in templated_args
238235
]),
239236
"TEMPLATED_bazel_require_script": _to_manifest_path(ctx, ctx.file._bazel_require_script),
@@ -295,13 +292,6 @@ def _nodejs_binary_impl(ctx):
295292
]
296293

297294
_NODEJS_EXECUTABLE_ATTRS = {
298-
"bootstrap": attr.string_list(
299-
doc = """JavaScript modules to be loaded before the entry point.
300-
For example, Angular uses this to patch the Jasmine async primitives for
301-
zone.js before the first `describe`.
302-
""",
303-
default = [],
304-
),
305295
"configuration_env_vars": attr.string_list(
306296
doc = """Pass these configuration environment variables to the resulting binary.
307297
Chooses a subset of the configuration environment variables (taken from `ctx.var`), which also

internal/node/node_loader.js

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,6 @@ var MODULE_ROOTS = [
4848
TEMPLATED_module_roots
4949
].sort((a, b) => b.module_name.toString().length - a.module_name.toString().length);
5050

51-
/**
52-
* Array of bootstrap modules that need to be loaded before the entry point.
53-
*/
54-
var BOOTSTRAP = [TEMPLATED_bootstrap];
55-
5651
const USER_WORKSPACE_NAME = 'TEMPLATED_user_workspace_name';
5752
const NODE_MODULES_ROOT = 'TEMPLATED_node_modules_root';
5853
const BIN_DIR = 'TEMPLATED_bin_dir';
@@ -66,7 +61,6 @@ log_verbose(`running ${TARGET} with
6661
runfiles: ${process.env.RUNFILES}
6762
6863
BIN_DIR: ${BIN_DIR}
69-
BOOTSTRAP: ${JSON.stringify(BOOTSTRAP, undefined, 2)}
7064
ENTRY_POINT: ${ENTRY_POINT}
7165
GEN_DIR: ${GEN_DIR}
7266
INSTALL_SOURCE_MAP_SUPPORT: ${INSTALL_SOURCE_MAP_SUPPORT}
@@ -504,15 +498,6 @@ if (INSTALL_SOURCE_MAP_SUPPORT) {
504498
Set install_source_map_support = False in ${TARGET} to turn off this warning.`);
505499
}
506500
}
507-
// Load all bootstrap modules before loading the entrypoint.
508-
for (var i = 0; i < BOOTSTRAP.length; i++) {
509-
try {
510-
module.constructor._load(BOOTSTRAP[i], this);
511-
} catch (e) {
512-
console.error('bootstrap failure ' + e.stack || e);
513-
process.exit(1);
514-
}
515-
}
516501

517502
if (require.main === module) {
518503
// Set the actual entry point in the arguments list.

internal/node/test/BUILD.bazel

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,62 @@ nodejs_test(
9999
entry_point = ":data_resolution.spec.js",
100100
)
101101

102+
nodejs_test(
103+
name = "bootstrap_test",
104+
data = ["bootstrap.js"],
105+
entry_point = ":bootstrap.spec.js",
106+
templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"],
107+
)
108+
109+
# Special case when $(location) is for a root file
110+
nodejs_test(
111+
name = "bootstrap_root_test",
112+
data = ["//:bootstrap.js"],
113+
entry_point = ":bootstrap.spec.js",
114+
templated_args = ["--node_options=--require=$(rlocation $(location //:bootstrap.js))"],
115+
)
116+
117+
nodejs_test(
118+
name = "bootstraps_test",
119+
data = [
120+
"bootstrap1.js",
121+
"bootstrap2.js",
122+
],
123+
entry_point = ":bootstraps.spec.js",
124+
templated_args = [
125+
"--node_options=--require=$(rlocation $(location :bootstrap1.js))",
126+
"--node_options=--require=$(rlocation $(location :bootstrap2.js))",
127+
],
128+
)
129+
130+
nodejs_test(
131+
name = "bootstrap_mlocation_test",
132+
data = ["bootstrap.js"],
133+
entry_point = ":bootstrap.spec.js",
134+
templated_args = ["--node_options=--require=$(rlocation $(mlocation :bootstrap.js))"],
135+
)
136+
137+
# Special case when $(location) is for a root file
138+
nodejs_test(
139+
name = "bootstrap_mlocation_root_test",
140+
data = ["//:bootstrap.js"],
141+
entry_point = ":bootstrap.spec.js",
142+
templated_args = ["--node_options=--require=$(rlocation $(mlocation //:bootstrap.js))"],
143+
)
144+
145+
nodejs_test(
146+
name = "bootstraps_mlocation_test",
147+
data = [
148+
"bootstrap1.js",
149+
"bootstrap2.js",
150+
],
151+
entry_point = ":bootstraps.spec.js",
152+
templated_args = [
153+
"--node_options=--require=$(rlocation $(mlocation :bootstrap1.js))",
154+
"--node_options=--require=$(rlocation $(mlocation :bootstrap2.js))",
155+
],
156+
)
157+
102158
# Also test resolution from built files.
103159
copy_file(
104160
name = "module_resolution_lib",

internal/node/test/bootstrap.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
console.log('here')
2+
global.bootstrapped = true;
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
if (!global.bootstrapped) {
2+
console.error('should run bootstrap script first');
3+
process.exitCode = 1;
4+
}

internal/node/test/bootstrap1.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
global.bootstrapped = global.bootstrapped ? global.bootstrapped + 1 : 1;
2+
global.last_bootstrap = 'bootstrap1';

0 commit comments

Comments
 (0)