refactor: use deps declared on rollup_bundle (#1287)

* refactor: use deps declared on rollup_bundle

This replaces the dynamic_deps mechanism for pulling user-specified plugins into the node_modules tree

* refactor: instead of special case for rollup, generally patch realpath/lstat

* refactor: use new no-loader mode for npm_package_bin

This should unblock usage from Angular architect, Jest, webpack and others

examples/angular/src/BUILD.bazel

@@ -127,7 +127,11 @@ rollup_bundle(
         ":main.prod.ts": "index",
     },
     output_dir = True,
-    deps = ["//src"],
+    deps = [
+        "//src",
+        "@npm//rollup-plugin-commonjs",
+        "@npm//rollup-plugin-node-resolve",
+    ],
 )
 
 babel(

examples/angular_view_engine/src/BUILD.bazel

@@ -125,7 +125,13 @@ rollup_bundle(
         ":main.prod.ts": "index",
     },
     output_dir = True,
-    deps = ["//src"],
+    deps = [
+        "//src",
+        "@npm//rollup-plugin-amd",
+        "@npm//rollup-plugin-commonjs",
+        "@npm//rollup-plugin-node-resolve",
+        "@npm//rollup-plugin-re",
+    ],
 )
 
 babel(

examples/kotlin/BUILD.bazel

@@ -42,6 +42,8 @@ rollup_bundle(
     deps = [
         "@npm//kotlin",
         "@npm//kotlinx-html-js",
+        "@npm//rollup-plugin-commonjs",
+        "@npm//rollup-plugin-node-resolve",
     ],
 )
 

internal/linker/link_node_modules.bzl

@@ -10,7 +10,7 @@ linker, which uses the mappings to link a node_modules directory for
 runtimes to locate all the first-party packages.
 """
 
-load("@build_bazel_rules_nodejs//:providers.bzl", "NpmPackageInfo")
+load("@build_bazel_rules_nodejs//internal/providers:npm_package_info.bzl", "NpmPackageInfo")
 
 def _debug(vars, *args):
     if "VERBOSE_LOGS" in vars.keys():
@@ -22,13 +22,26 @@ _ASPECT_RESULT_NAME = "link_node_modules__aspect_result"
 # Traverse 'srcs' in addition so that we can go across a genrule
 _MODULE_MAPPINGS_DEPS_NAMES = ["data", "deps", "srcs"]
 
-def register_node_modules_linker(ctx, args, inputs):
+def add_arg(args, arg):
+    """Add an argument
+
+    Args:
+        args: either a list or a ctx.actions.Args object
+        arg: string arg to append on the end
+    """
+    if (type(args) == type([])):
+        args.append(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
 
     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)
     """
 
     mappings = {
@@ -40,7 +53,7 @@ def register_node_modules_linker(ctx, args, inputs):
     node_modules_root = ""
 
     # Look through data/deps attributes to find...
-    for dep in getattr(ctx.attr, "data", []) + getattr(ctx.attr, "deps", []):
+    for dep in 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"])
@@ -67,7 +80,7 @@ def register_node_modules_linker(ctx, args, inputs):
         "workspace": ctx.workspace_name,
     }
     ctx.actions.write(modules_manifest, str(content))
-    args.add("--bazel_node_modules_manifest=%s" % modules_manifest.path)
+    add_arg(args, "--bazel_node_modules_manifest=%s" % modules_manifest.path)
     inputs.append(modules_manifest)
 
 def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name = None):

internal/node/bazel_require_script.js

@@ -0,0 +1,38 @@
+// Adapt node programs to run under Bazel
+// Meant to be run in a --require hook
+
+const fs = require('fs');
+const path = require('path');
+const orig = {};
+// TODO: more functions need patched like
+// the async and native versions
+orig['realPathSync'] = fs.realpathSync;
+orig['lstatSync'] = fs.lstatSync;
+
+// To fully resolve a symlink requires recursively
+// following symlinks until the target is a file
+// rather than a symlink, so we must make this look
+// like a file.
+function lstatSync(p) {
+  const result = orig.lstatSync(p);
+  result.isSymbolicLink = () => false;
+  result.isFile = () => true;
+  return result;
+}
+
+function realpathSync(...s) {
+  // Realpath returns an absolute path, so we should too
+  return path.resolve(s[0]);
+}
+
+function monkeypatch() {
+  fs.realpathSync = realpathSync;
+  fs.lstatSync = lstatSync;
+}
+
+function unmonkeypatch() {
+  fs.realpathSync = orig.realPathSync;
+  fs.lstatSync = orig.lstatSync;
+}
+
+monkeypatch();

internal/node/node.bzl

@@ -20,7 +20,7 @@ They support module mapping: any targets in the transitive dependencies with
 a `module_name` attribute can be `require`d by that name.
 """
 
-load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo", "NpmPackageInfo", "node_modules_aspect")
+load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo", "NodeRuntimeDepsInfo", "NpmPackageInfo", "node_modules_aspect")
 load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
 load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows")
@@ -125,6 +125,20 @@ def _to_manifest_path(ctx, file):
     else:
         return ctx.workspace_name + "/" + file.short_path
 
+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 ("<ERROR> _to_execroot_path not yet implemented for " + file.path)
+
 def _nodejs_binary_impl(ctx):
     node_modules = depset(ctx.files.node_modules)
 
@@ -170,6 +184,7 @@ def _nodejs_binary_impl(ctx):
         fail("The node toolchain was not properly configured so %s cannot be executed. Make sure that target_tool_path or target_tool is set." % ctx.attr.name)
 
     node_tool_files.append(ctx.file._link_modules_script)
+    node_tool_files.append(ctx.file._bazel_require_script)
 
     if not ctx.outputs.templated_args_file:
         templated_args = ctx.attr.templated_args
@@ -201,12 +216,14 @@ def _nodejs_binary_impl(ctx):
             expand_location_into_runfiles(ctx, a)
             for a in templated_args
         ]),
+        "TEMPLATED_bazel_require_script": _to_manifest_path(ctx, ctx.file._bazel_require_script),
         "TEMPLATED_env_vars": env_vars,
         "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_node": node_tool_info.target_tool_path,
         "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
-        "TEMPLATED_script_path": script_path,
+        "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point),
     }
     ctx.actions.expand_template(
         template = ctx.file._launcher_template,
@@ -227,22 +244,30 @@ def _nodejs_binary_impl(ctx):
     if ctx.file.entry_point.extension == "js":
         runfiles = depset([ctx.file.entry_point], transitive = [runfiles])
 
-    return [DefaultInfo(
-        executable = executable,
-        runfiles = ctx.runfiles(
-            transitive_files = runfiles,
-            files = node_tool_files + [
-                        ctx.outputs.loader,
-                    ] + ctx.files._source_map_support_files +
-
-                    # We need this call to the list of Files.
-                    # Calling the .to_list() method may have some perfs hits,
-                    # so we should be running this method only once per rule.
-                    # see: https://docs.bazel.build/versions/master/skylark/depsets.html#performance
-                    node_modules.to_list() + sources.to_list(),
-            collect_data = True,
+    return [
+        DefaultInfo(
+            executable = executable,
+            runfiles = ctx.runfiles(
+                transitive_files = runfiles,
+                files = node_tool_files + [
+                            ctx.outputs.loader,
+                        ] + ctx.files._source_map_support_files +
+
+                        # We need this call to the list of Files.
+                        # Calling the .to_list() method may have some perfs hits,
+                        # so we should be running this method only once per rule.
+                        # see: https://docs.bazel.build/versions/master/skylark/depsets.html#performance
+                        node_modules.to_list() + sources.to_list(),
+                collect_data = True,
+            ),
         ),
-    )]
+        # TODO(alexeagle): remove sources and node_modules from the runfiles
+        # when downstream usage is ready to rely on linker
+        NodeRuntimeDepsInfo(
+            deps = depset([ctx.file.entry_point], transitive = [node_modules, sources]),
+            pkgs = ctx.attr.data,
+        ),
+    ]
 
 _NODEJS_EXECUTABLE_ATTRS = {
     "bootstrap": attr.string_list(
@@ -274,8 +299,9 @@ The set of default  environment variables is:
 
 - `DEBUG`: rules use this environment variable to turn on debug information in their output artifacts
 - `VERBOSE_LOGS`: rules use this environment variable to turn on debug output in their logs
+- `NODE_DEBUG`: used by node.js itself to print more logs
 """,
-        default = ["DEBUG", "VERBOSE_LOGS"],
+        default = ["DEBUG", "VERBOSE_LOGS", "NODE_DEBUG"],
     ),
     "entry_point": attr.label(
         doc = """The script which should be executed first, usually containing a main function.
@@ -424,6 +450,10 @@ jasmine_node_test(
         """,
     ),
     "_bash_runfile_helpers": attr.label(default = Label("@bazel_tools//tools/bash/runfiles")),
+    "_bazel_require_script": attr.label(
+        default = Label("//internal/node:bazel_require_script.js"),
+        allow_single_file = True,
+    ),
     "_launcher_template": attr.label(
         default = Label("//internal/node:node_launcher.sh"),
         allow_single_file = True,

internal/node/node_launcher.sh

@@ -116,8 +116,21 @@ TEMPLATED_env_vars
 
 readonly node=$(rlocation "TEMPLATED_node")
 readonly repository_args=$(rlocation "TEMPLATED_repository_args")
-readonly script=$(rlocation "TEMPLATED_script_path")
+MAIN=$(rlocation "TEMPLATED_loader_path")
 readonly link_modules_script=$(rlocation "TEMPLATED_link_modules_script")
+bazel_require_script=$(rlocation "TEMPLATED_bazel_require_script")
+
+# Node's --require option assumes that a non-absolute path not starting with `.` is
+# a module, so that you can do --require=source-map-support/register
+# So if the require script is not absolute, we must make it so
+case "${bazel_require_script}" in
+  # Absolute path on unix
+  /*          ) ;;
+  # Absolute path on Windows, e.g. C:/path/to/thing
+  [a-zA-Z]:/* ) ;;
+  # Otherwise it needs to be made relative
+  *           ) bazel_require_script="./${bazel_require_script}" ;;
+esac
 
 source $repository_args
 
@@ -127,6 +140,10 @@ ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
 for ARG in "${ALL_ARGS[@]}"; do
   case "$ARG" in
     --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
+    --nobazel_patch_module_resolver)
+      MAIN="TEMPLATED_script_path"
+      NODE_OPTIONS+=( "--require" "$bazel_require_script" )
+      ;;
     --node_options=*) NODE_OPTIONS+=( "${ARG#--node_options=}" ) ;;
     *) ARGS+=( "$ARG" )
   esac
@@ -147,12 +164,12 @@ if [ "${EXPECTED_EXIT_CODE}" -eq "0" ]; then
   # handled by the node process.
   # If we had merely forked a child process here, we'd be responsible
   # for forwarding those OS interactions.
-  exec "${node}" "${NODE_OPTIONS[@]}" "${script}" "${ARGS[@]}"
+  exec "${node}" "${NODE_OPTIONS[@]}" "${MAIN}" "${ARGS[@]}"
   # exec terminates execution of this shell script, nothing later will run.
 fi
 
 set +e
-"${node}" "${NODE_OPTIONS[@]}" "${script}" "${ARGS[@]}"
+"${node}" "${NODE_OPTIONS[@]}" "${MAIN}" "${ARGS[@]}"
 RESULT="$?"
 set -e
 

internal/node/npm_package_bin.bzl

@@ -1,7 +1,7 @@
 "A generic rule to run a tool that appears in node_modules/.bin"
 
-load("@build_bazel_rules_nodejs//:providers.bzl", "NpmPackageInfo", "node_modules_aspect")
-load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "register_node_modules_linker")
+load("@build_bazel_rules_nodejs//:providers.bzl", "NpmPackageInfo", "node_modules_aspect", "run_node")
+load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect")
 
 # Note: this API is chosen to match nodejs_binary
 # so that we can generate macros that act as either an output-producing tool or an executable
@@ -52,11 +52,12 @@ def _impl(ctx):
         outputs = [ctx.actions.declare_directory(ctx.attr.name)]
     else:
         outputs = ctx.outputs.outs
-    register_node_modules_linker(ctx, args, inputs)
+
     for a in ctx.attr.args:
         args.add(_expand_location(ctx, a))
-    ctx.actions.run(
-        executable = ctx.executable.tool,
+    run_node(
+        ctx,
+        executable = "tool",
         inputs = inputs,
         outputs = outputs,
         arguments = [args],

internal/npm_install/generate_build_file.js

@@ -419,9 +419,6 @@ def _maybe(repo_rule, name, **kwargs):
     }
     function addDynamicDependencies(pkgs, dynamic_deps = DYNAMIC_DEPS) {
         function match(name, p) {
-            // Automatically include dynamic dependency on plugins of the form pkg-plugin-foo
-            if (name.startsWith(`${p._moduleName}-plugin-`))
-                return true;
             const value = dynamic_deps[p._moduleName];
             if (name === value)
                 return true;

internal/npm_install/generate_build_file.ts

@@ -459,9 +459,6 @@ function hasRootBuildFile(pkg: Dep, rootPath: string) {
 
 function addDynamicDependencies(pkgs: Dep[], dynamic_deps = DYNAMIC_DEPS) {
   function match(name: string, p: Dep) {
-    // Automatically include dynamic dependency on plugins of the form pkg-plugin-foo
-    if (name.startsWith(`${p._moduleName}-plugin-`)) return true;
-
     const value = dynamic_deps[p._moduleName];
     if (name === value) return true;
 

internal/npm_install/test/generate_build_file.spec.js

@@ -101,27 +101,6 @@ describe('build file generator', () => {
       expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:bar']);
       expect(printPackageBin(pkgs[0])).toContain('data = ["//some_dir:foo", "//bar:bar"]');
     });
-    it('should automatically include plugins in nodejs_binary data', () => {
-      const pkgs = [
-        {_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'},
-        {_name: 'foo-plugin-bar', _dir: 'bar', _moduleName: 'foo-plugin-bar'}
-      ];
-      addDynamicDependencies(pkgs, {});
-      expect(pkgs[0]._dynamicDependencies).toEqual(['//bar:foo-plugin-bar']);
-      expect(printPackageBin(pkgs[0]))
-          .toContain('data = ["//some_dir:foo", "//bar:foo-plugin-bar"]');
-    });
-    it('should not include nested packages', () => {
-      const pkgs = [
-        {_name: 'foo', bin: 'foobin', _dir: 'some_dir', _moduleName: 'foo'}, {
-          _name: 'foo-plugin-bar',
-          _dir: 'top-level/node_modules/bar',
-          _moduleName: 'foo-plugin-bar'
-        }
-      ];
-      addDynamicDependencies(pkgs, {});
-      expect(pkgs[0]._dynamicDependencies).toEqual([]);
-    });
   });
 
   describe('index.bzl files', () => {