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
bazel-contrib · Dec 6, 2019 · 98bb420 · 98bb420
1 parent 68a8467
commit 98bb420
Show file tree

Hide file tree

Showing 12 changed files with 48 additions and 14 deletions.
diff --git 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):
@@ -144,6 +145,7 @@ def _to_execroot_path(ctx, file):
     return ("<ERROR> _to_execroot_path not yet implemented for " + 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.
@@ -198,6 +200,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 +239,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
@@ -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
@@ -951,7 +951,8 @@ nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
     install_source_map_support = False,
-    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],
+    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
 )
 `;
             }
@@ -982,7 +983,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 +993,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
@@ -1049,7 +1049,8 @@ nodejs_binary(
     name = "${name}",
     entry_point = "//:node_modules/${pkg._dir}/${path}",
     install_source_map_support = False,
-    data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)}
+    data = [${data.map(p => `"${p}"`).join(', ')}],
+    templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)}
 )
 `;
     }
@@ -1084,7 +1085,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 +1096,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/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden
@@ -6,4 +6,5 @@ nodejs_binary(
     entry_point = "//:node_modules/@gregmagolan/test-a/@bin/test.js",
     install_source_map_support = False,
     data = ["//@gregmagolan/test-a:test-a"],
+    templated_args = ["--nobazel_patch_module_resolver"],
 )
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
@@ -8,12 +8,14 @@ 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):
     nodejs_test(
       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/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden
@@ -6,4 +6,5 @@ nodejs_binary(
     entry_point = "//:node_modules/jasmine/bin/jasmine.js",
     install_source_map_support = False,
     data = ["//jasmine:jasmine"],
+    templated_args = ["--nobazel_patch_module_resolver"],
 )
diff --git a/internal/npm_install/test/golden/jasmine/index.bzl.golden b/internal/npm_install/test/golden/jasmine/index.bzl.golden
@@ -8,12 +8,14 @@ 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):
     nodejs_test(
       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/labs/src/webpack/webpack_bundle.bzl b/packages/labs/src/webpack/webpack_bundle.bzl
@@ -17,6 +17,8 @@
 This rule is experimental, as part of Angular Labs! There may be breaking changes.
 """
 
+load("@build_bazel_rules_nodejs//:providers.bzl", "run_node")
+
 WEBPACK_BUNDLE_ATTRS = {
     "srcs": attr.label_list(allow_files = True),
     "entry_point": attr.label(allow_single_file = True, mandatory = True),
@@ -33,9 +35,10 @@ def _webpack_bundle(ctx):
     args.add(ctx.outputs.bundle.path)
     args.add(ctx.outputs.sourcemap.path)
     args.add(ctx.file.entry_point.path)
-    ctx.actions.run(
-        inputs = ctx.files.srcs,
-        executable = ctx.executable.webpack,
+    run_node(
+        ctx,
+        inputs = ctx.files.srcs[:],
+        executable = "webpack",
         outputs = [ctx.outputs.bundle, ctx.outputs.sourcemap],
         arguments = [args],
         progress_message = "Bundling JavaScript %s [webpack]" % ctx.outputs.bundle.path,

diff --git 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,15 @@ 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//mocha",
         "@npm_node_patches//:node_modules",
     ],
+    entry_point = "@npm_node_patches//:node_modules/mocha/bin/mocha",
     tags = [
         "fix-windows",
     ],

diff --git a/packages/terser/docs/BUILD.bazel b/packages/terser/docs/BUILD.bazel
@@ -9,5 +9,8 @@ stardoc(
     testonly = True,
     out = "index.md",
     input = "@npm_bazel_terser//:index.bzl",
-    deps = ["@npm_bazel_terser//:bzl"],
+    deps = [
+        "//:bzl",
+        "@npm_bazel_terser//:bzl",
+    ],
 )
diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl
@@ -14,6 +14,8 @@
 
 "Rule to run the terser binary under bazel"
 
+load("@build_bazel_rules_nodejs//:providers.bzl", "run_node")
+
 _DOC = """Run the terser minifier.
 
 Typical example:
@@ -165,10 +167,11 @@ def _terser(ctx):
     args.add_all(["--config-file", opts.path])
     args.add_all(ctx.attr.args)
 
-    ctx.actions.run(
+    run_node(
+        ctx,
         inputs = inputs,
         outputs = outputs,
-        executable = ctx.executable.terser_bin,
+        executable = "terser_bin",
         arguments = [args],
         progress_message = "Minifying JavaScript %s [terser]" % (outputs[0].short_path),
     )