From 1d0454f903c4de55ab6f28eb73961b1f617fa752 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 6 Dec 2019 09:10:38 -0800 Subject: [PATCH 1/2] refactor: register_node_modules_linker just writes the manifest This makes it easier to reuse in nodejs_binary --- internal/linker/link_node_modules.bzl | 15 ++++++--------- internal/linker/test/integration/rule.bzl | 17 ++++++++--------- internal/providers/node_runtime_deps_info.bzl | 6 ++++-- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl index 739fe1302e..d3d2dcb597 100644 --- a/internal/linker/link_node_modules.bzl +++ b/internal/linker/link_node_modules.bzl @@ -34,14 +34,12 @@ def add_arg(args, arg): else: args.add(arg) -def register_node_modules_linker(ctx, args, inputs, data = []): - """Helps an action to run node by setting up the node_modules linker as a pre-process +def write_node_modules_manifest(ctx, extra_data = []): + """Writes a manifest file read by the linker, containing info about resolving runtime dependencies Args: - ctx: Bazel's starlark execution context, used to get attributes and actions - args: Arguments being passed to the program; a linker argument will be appended - inputs: inputs being passed to the program; a linker input will be appended - data: labels to search for npm packages that need to be linked (ctx.attr.deps and ctx.attr.data will always be searched) + ctx: starlark rule execution context + extra_data: labels to search for npm packages that need to be linked (ctx.attr.deps and ctx.attr.data will always be searched) """ mappings = { @@ -53,7 +51,7 @@ def register_node_modules_linker(ctx, args, inputs, data = []): node_modules_root = "" # Look through data/deps attributes to find... - for dep in data + getattr(ctx.attr, "data", []) + getattr(ctx.attr, "deps", []): + for dep in extra_data + getattr(ctx.attr, "data", []) + getattr(ctx.attr, "deps", []): # ...the root directory for the third-party node_modules; we'll symlink the local "node_modules" to it if NpmPackageInfo in dep: possible_root = "/".join([dep[NpmPackageInfo].workspace, "node_modules"]) @@ -80,8 +78,7 @@ def register_node_modules_linker(ctx, args, inputs, data = []): "workspace": ctx.workspace_name, } ctx.actions.write(modules_manifest, str(content)) - add_arg(args, "--bazel_node_modules_manifest=%s" % modules_manifest.path) - inputs.append(modules_manifest) + return modules_manifest def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name = None): """Returns the module_mappings from the given attrs. diff --git a/internal/linker/test/integration/rule.bzl b/internal/linker/test/integration/rule.bzl index 5578aa6dce..157cbb041f 100644 --- a/internal/linker/test/integration/rule.bzl +++ b/internal/linker/test/integration/rule.bzl @@ -1,18 +1,17 @@ "Minimal fixture for executing the linker's starlark code" -load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "register_node_modules_linker") +load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "write_node_modules_manifest") def _linked(ctx): - inputs = ctx.files.deps[:] - outputs = [ctx.outputs.out] - args = ctx.actions.args() - register_node_modules_linker(ctx, args, inputs) - args.add(ctx.outputs.out.path) + modules_manifest = write_node_modules_manifest(ctx) ctx.actions.run( - inputs = inputs, - outputs = outputs, + inputs = ctx.files.deps + [modules_manifest], + outputs = [ctx.outputs.out], executable = ctx.executable.program, - arguments = [args], + arguments = [ + "--bazel_node_modules_manifest=%s" % modules_manifest.path, + ctx.outputs.out.path, + ], ) linked = rule(_linked, attrs = { diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl index efa90f7b72..90a6942c22 100644 --- a/internal/providers/node_runtime_deps_info.bzl +++ b/internal/providers/node_runtime_deps_info.bzl @@ -14,7 +14,7 @@ """ Custom provider that mimics the Runfiles, but doesn't incur the expense of creating the runfiles symlink tree""" -load("//internal/linker:link_node_modules.bzl", "add_arg", "register_node_modules_linker") +load("//internal/linker:link_node_modules.bzl", "add_arg", "write_node_modules_manifest") NodeRuntimeDepsInfo = provider( doc = """Stores runtime dependencies of a nodejs_binary or nodejs_test @@ -54,7 +54,9 @@ def run_node(ctx, inputs, arguments, executable, **kwargs): extra_inputs = exec_attr[NodeRuntimeDepsInfo].deps.to_list() link_data = exec_attr[NodeRuntimeDepsInfo].pkgs - register_node_modules_linker(ctx, arguments, inputs, link_data) + modules_manifest = write_node_modules_manifest(ctx, link_data) + add_arg(arguments, "--bazel_node_modules_manifest=%s" % modules_manifest.path) + inputs.append(modules_manifest) # By using the run_node helper, you suggest that your program # doesn't implicitly use runfiles to require() things From 58b66f405cf6839a8c217cb39c25e9c99bf32855 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 6 Dec 2019 11:14:50 -0800 Subject: [PATCH 2/2] feat(builtin): wire linker/node-patches to npm-generated bin and test - nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker - if the program isn't run with a user-specific linker manifest, we use the static one as a default - always pass the flag to opt-out of custom module resolver for generated rules Fixes #1382 --- internal/node/node.bzl | 17 +++++++++-------- internal/node/node_launcher.sh | 6 ++++++ internal/npm_install/generate_build_file.js | 6 ++++-- internal/npm_install/generate_build_file.ts | 6 ++++-- .../golden/@gregmagolan/test-a/index.bzl.golden | 2 ++ .../test/golden/jasmine/index.bzl.golden | 2 ++ packages/node-patches/BUILD.bazel | 6 ++++-- 7 files changed, 31 insertions(+), 14 deletions(-) diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 9e47e648e8..50a47230fa 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -25,6 +25,7 @@ load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfile load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect") load("//internal/common:path_utils.bzl", "strip_external") load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows") +load("//internal/linker:link_node_modules.bzl", "write_node_modules_manifest") load("//internal/node:node_repositories.bzl", "BUILT_IN_NODE_PLATFORMS") def _trim_package_node_modules(package_name): @@ -132,18 +133,15 @@ def _to_manifest_path(ctx, file): def _to_execroot_path(ctx, file): parts = file.path.split("/") - #print("_to_execroot", file.path, file.is_source) if parts[0] == "external": if parts[2] == "node_modules": # external/npm/node_modules -> node_modules/foo # the linker will make sure we can resolve node_modules from npm return "/".join(parts[2:]) - if file.is_source: - return file.path - - return (" _to_execroot_path not yet implemented for " + file.path) + return file.path def _nodejs_binary_impl(ctx): + node_modules_manifest = write_node_modules_manifest(ctx) node_modules = depset(ctx.files.node_modules) # Also include files from npm fine grained deps as inputs. @@ -155,14 +153,15 @@ def _nodejs_binary_impl(ctx): # Using a depset will allow us to avoid flattening files and sources # inside this loop. This should reduce the performances hits, # since we don't need to call .to_list() - sources = depset() + sources_sets = [] for d in ctx.attr.data: # TODO: switch to JSModuleInfo when it is available if JSNamedModuleInfo in d: - sources = depset(transitive = [sources, d[JSNamedModuleInfo].sources]) + sources_sets.append(d[JSNamedModuleInfo].sources) if hasattr(d, "files"): - sources = depset(transitive = [sources, d.files]) + sources_sets.append(d.files) + sources = depset(transitive = sources_sets) _write_loader_script(ctx) @@ -198,6 +197,7 @@ def _nodejs_binary_impl(ctx): node_tool_files.append(ctx.file._link_modules_script) node_tool_files.append(ctx.file._bazel_require_script) + node_tool_files.append(node_modules_manifest) if not ctx.outputs.templated_args_file: templated_args = ctx.attr.templated_args @@ -236,6 +236,7 @@ def _nodejs_binary_impl(ctx): "TEMPLATED_expected_exit_code": str(expected_exit_code), "TEMPLATED_link_modules_script": _to_manifest_path(ctx, ctx.file._link_modules_script), "TEMPLATED_loader_path": script_path, + "TEMPLATED_modules_manifest": _to_manifest_path(ctx, node_modules_manifest), "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args), "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point), "TEMPLATED_vendored_node": "" if is_builtin else strip_external(ctx.file._node.path), diff --git a/internal/node/node_launcher.sh b/internal/node/node_launcher.sh index 766ff47fad..6b7705073c 100644 --- a/internal/node/node_launcher.sh +++ b/internal/node/node_launcher.sh @@ -186,6 +186,12 @@ for ARG in "${ALL_ARGS[@]}"; do --nobazel_patch_module_resolver) MAIN="TEMPLATED_script_path" NODE_OPTIONS+=( "--require" "$bazel_require_script" ) + + # In this case we should always run the linker + # For programs which are called with bazel run or bazel test, there will be no additional runtime + # dependencies to link, so we use the default modules_manifest which has only the static dependencies + # of the binary itself + MODULES_MANIFEST=${MODULES_MANIFEST:-$(rlocation "TEMPLATED_modules_manifest")} ;; --node_options=*) NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;; *) ARGS+=( "$ARG" ) diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js index 97154ab32e..ff8b655571 100644 --- a/internal/npm_install/generate_build_file.js +++ b/internal/npm_install/generate_build_file.js @@ -982,7 +982,8 @@ def ${name.replace(/-/g, '_')}(**kwargs): nodejs_binary( entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),${additionalAttributes(pkg, name)} + data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)} **kwargs ) @@ -991,7 +992,8 @@ def ${name.replace(/-/g, '_')}_test(**kwargs): nodejs_test( entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),${additionalAttributes(pkg, name)} + data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${additionalAttributes(pkg, name)} **kwargs ) `; diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index c2a8dec9bf..2de1035aeb 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -1084,7 +1084,8 @@ def ${name.replace(/-/g, '_')}(**kwargs): nodejs_binary( entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),${ + data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${ additionalAttributes(pkg, name)} **kwargs ) @@ -1094,7 +1095,8 @@ def ${name.replace(/-/g, '_')}_test(**kwargs): nodejs_test( entry_point = "@${WORKSPACE}//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []),${ + data = [${data.map(p => `"${p}"`).join(', ')}] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []),${ additionalAttributes(pkg, name)} **kwargs ) diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden index 34d685e941..8e3a864ddc 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden +++ b/internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden @@ -8,6 +8,7 @@ def test(**kwargs): entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js", install_source_map_support = False, data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []), **kwargs ) def test_test(**kwargs): @@ -15,5 +16,6 @@ def test_test(**kwargs): entry_point = "@fine_grained_goldens//:node_modules/@gregmagolan/test-a/@bin/test.js", install_source_map_support = False, data = ["@fine_grained_goldens//@gregmagolan/test-a:test-a"] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []), **kwargs ) diff --git a/internal/npm_install/test/golden/jasmine/index.bzl.golden b/internal/npm_install/test/golden/jasmine/index.bzl.golden index fb339b6688..d71afcd81f 100644 --- a/internal/npm_install/test/golden/jasmine/index.bzl.golden +++ b/internal/npm_install/test/golden/jasmine/index.bzl.golden @@ -8,6 +8,7 @@ def jasmine(**kwargs): entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js", install_source_map_support = False, data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []), **kwargs ) def jasmine_test(**kwargs): @@ -15,5 +16,6 @@ def jasmine_test(**kwargs): entry_point = "@fine_grained_goldens//:node_modules/jasmine/bin/jasmine.js", install_source_map_support = False, data = ["@fine_grained_goldens//jasmine:jasmine"] + kwargs.pop("data", []), + templated_args = ["--nobazel_patch_module_resolver"] + kwargs.pop("templated_args", []), **kwargs ) diff --git a/packages/node-patches/BUILD.bazel b/packages/node-patches/BUILD.bazel index a927dc8dd9..d2c7becad0 100644 --- a/packages/node-patches/BUILD.bazel +++ b/packages/node-patches/BUILD.bazel @@ -12,8 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test") load("@npm_bazel_rollup//:index.bzl", "rollup_bundle") -load("@npm_node_patches//mocha:index.bzl", "mocha_test") load("@npm_node_patches//typescript:index.bzl", "tsc") package(default_visibility = ["//:__subpackages__"]) @@ -52,12 +52,14 @@ tsc( ], ) -mocha_test( +# Like the generated mocha_test but we don't want to run the patches before testing the patches +nodejs_test( name = "unit_test", args = ["$(location %s)" % s for s in test_js], data = js + test_js + [ "@npm_node_patches//:node_modules", ], + entry_point = "@npm_node_patches//:node_modules/mocha/bin/mocha", tags = [ "fix-windows", ],