feat(builtin): introduce npm_package_bin (#1139)

This is a genrule-like rule that runs any npm bin
We generate one of these in the @npm workspace

illustrate its usage by replacing stylus and less rules

Those rules are now deprecated, will be deprecated on npm, and their
sources deleted soon.

Design doc: https://hackmd.angular.io/f8OBi9KMTNKButSWsh1gCA?view

WORKSPACE

@@ -237,13 +237,13 @@ filegroup(
     "//@gregmagolan:BUILD.bazel",
     "//@gregmagolan/test-a/bin:BUILD.bazel",
     "//@gregmagolan/test-a:BUILD.bazel",
-    "//@gregmagolan/test-b/bin:BUILD.bazel",
+    "//@gregmagolan/test-a:index.bzl",
     "//@gregmagolan/test-b:BUILD.bazel",
     "//ajv:BUILD.bazel",
     "//jasmine/bin:BUILD.bazel",
     "//jasmine:BUILD.bazel",
+    "//jasmine:index.bzl",
     "//rxjs:BUILD.bazel",
-    "//unidiff/bin:BUILD.bazel",
     "//unidiff:BUILD.bazel",
     "//zone.js:BUILD.bazel",
   ],

defs.bzl

@@ -28,6 +28,7 @@ load(
     _nodejs_test = "nodejs_test",
 )
 load("//internal/node:node_repositories.bzl", _node_repositories = "node_repositories")
+load("//internal/node:npm_package_bin.bzl", _npm_bin = "npm_package_bin")
 load("//internal/npm_install:npm_install.bzl", _npm_install = "npm_install", _yarn_install = "yarn_install")
 load("//internal/npm_package:npm_package.bzl", _npm_package = "npm_package")
 load("//internal/rollup:rollup_bundle.bzl", _rollup_bundle = "rollup_bundle")
@@ -41,6 +42,7 @@ rollup_bundle = _rollup_bundle
 npm_package = _npm_package
 history_server = _history_server
 http_server = _http_server
+npm_package_bin = _npm_bin
 # ANY RULES ADDED HERE SHOULD BE DOCUMENTED, see index.for_docs.bzl
 
 # Allows us to avoid a transitive dependency on bazel_skylib from leaking to users

e2e/less/BUILD.bazel

@@ -1,9 +1,20 @@
 load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_test")
-load("@npm_bazel_less//:index.bzl", "less_binary")
+load("@npm//less:index.bzl", "lessc")
 
-less_binary(
+lessc(
     name = "styles",
-    src = "test.less",
+    srcs = ["test.less"],
+    outs = [
+        "test.css",
+        "test.css.map",
+    ],
+    args = [
+        "test.less",
+        # Output paths must use $(location) since Bazel puts them in a platform-dependent output directory
+        "$(location test.css)",
+        "--silent",
+        "--source-map",
+    ],
 )
 
 nodejs_test(

e2e/less/yarn.lock

@@ -2,8 +2,10 @@
 # yarn lockfile v1
 
 
-"@bazel/less@file:../../dist/bin/packages/less/npm_package":
-  version "1.2.3"
+"@bazel/less@latest":
+  version "0.37.1"
+  resolved "https://registry.yarnpkg.com/@bazel/less/-/less-0.37.1.tgz#915bd08f3f164b106639482ce3feebac30f63773"
+  integrity sha512-Ch+wN17E4CF5Gk7eX+mJ/i42LHXwDvDe6WMFHxBD0zwY3Omi/DozGYcaM+byCKSmssiPKK4g//d4ZIljVCsdfQ==
   dependencies:
     less "^3.9.0"
 

e2e/stylus/BUILD.bazel

@@ -1,9 +1,21 @@
 load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_test")
-load("@npm_bazel_stylus//:index.bzl", "stylus_binary")
+load("@npm//stylus:index.bzl", "stylus")
 
-stylus_binary(
+stylus(
     name = "styles",
-    src = "test.styl",
+    srcs = ["test.styl"],
+    outs = [
+        "test.css",
+        "test.css.map",
+    ],
+    args = [
+        "test.styl",
+        "--out",
+        # Output paths must use $(location) since Bazel puts them in a platform-dependent output directory
+        "$(location test.css)",
+        "--compress",
+        "--sourcemap",
+    ],
 )
 
 nodejs_test(

internal/node/npm_package_bin.bzl

@@ -0,0 +1,68 @@
+"A generic rule to run a tool that appears in node_modules/.bin"
+
+load("@build_bazel_rules_nodejs//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "register_node_modules_linker")
+
+_ATTRS = {
+    "srcs": attr.label_list(allow_files = True),
+    "outs": attr.output_list(),
+    "args": attr.string_list(mandatory = True),
+    "tool": attr.label(
+        executable = True,
+        cfg = "host",
+        mandatory = True,
+    ),
+    "deps": attr.label_list(aspects = [module_mappings_aspect]),
+}
+
+def _impl(ctx):
+    args = ctx.actions.args()
+    inputs = ctx.files.srcs + ctx.files.deps
+    outputs = ctx.outputs.outs
+    register_node_modules_linker(ctx, args, inputs)
+    for a in ctx.attr.args:
+        args.add(ctx.expand_location(a))
+    ctx.actions.run(
+        executable = ctx.executable.tool,
+        inputs = inputs,
+        outputs = outputs,
+        arguments = [args],
+    )
+
+_npm_genrule = rule(
+    _impl,
+    attrs = _ATTRS,
+)
+
+def npm_package_bin(tool = None, package = None, package_bin = None, **kwargs):
+    """Run an arbitrary npm package binary (anything under node_modules/.bin/*) under Bazel.
+
+    This is like a genrule() except that it runs our launcher script that first
+    links the node_modules tree before running the program.
+
+    It's probably easy to wrap this with macros, so something with no rule logic
+    like stylus_binary could probably just be a macro wrapping this.
+
+    Args:
+        srcs: identical to [genrule.srcs](https://docs.bazel.build/versions/master/be/general.html#genrule.srcs)
+        deps: Targets that produce or reference npm packages which are needed by the tool
+        outs: identical to [genrule.outs](https://docs.bazel.build/versions/master/be/general.html#genrule.outs)
+        args: Command-line arguments to the tool.
+
+            Subject to 'Make variable' substitution.
+            Can use $(location) expansion. See https://docs.bazel.build/versions/master/be/make-variables.html
+
+        package: an npm package whose binary to run, like "terser". Assumes your node_modules are installed in a workspace called "npm"
+        package_bin: the "bin" entry from `package` that should be run. By default package_bin is the same string as `package`
+        tool: a label for a binary to run, like `@npm//terser/bin:terser`. This is the longer form of package/package_bin
+    """
+    if not tool:
+        if not package:
+            fail("You must supply either the tool or package attribute")
+        if not package_bin:
+            package_bin = package
+        tool = "@npm//%s/bin:%s" % (package, package_bin)
+
+    _npm_genrule(
+        tool = tool,
+        **kwargs
+    )

internal/node/test/BUILD.bazel

@@ -1,4 +1,4 @@
-load("//:defs.bzl", "nodejs_binary", "nodejs_test")
+load("//:defs.bzl", "nodejs_binary", "nodejs_test", "npm_package_bin")
 load("//internal/js_library:js_library.bzl", "js_library")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/copy_file.bzl", "copy_file")
 load("//third_party/github.com/bazelbuild/bazel-skylib:rules/write_file.bzl", "write_file")
@@ -145,3 +145,21 @@ nodejs_test(
     ],
     entry_point = ":data_resolution_built.spec.js",
 )
+
+npm_package_bin(
+    name = "run_terser",
+    srcs = ["terser_input.js"],
+    outs = ["minified.js"],
+    args = [
+        "$(location terser_input.js)",
+        "--output",
+        "$(location minified.js)",
+    ],
+    package = "terser",
+)
+
+nodejs_test(
+    name = "npm_package_bin_test",
+    data = ["minified.js"],
+    entry_point = "npm_package_bin.spec.js",
+)

internal/node/test/npm_package_bin.spec.js

@@ -0,0 +1,8 @@
+const fs = require('fs');
+const path = require('path');
+
+const content = fs.readFileSync(path.join(require.resolve(__dirname + '/minified.js')), 'utf-8');
+if (!content.includes('{console.error("thing")}')) {
+  console.error(content);
+  process.exitCode = 1;
+}

internal/node/test/terser_input.js

@@ -0,0 +1,5 @@
+class A {
+  doThing() {
+    console.error('thing');
+  }
+}

internal/npm_install/generate_build_file.js

@@ -221,14 +221,27 @@ node_module_library(
 }
 
 /**
- * Generates all BUILD files for a package.
+ * Generates all BUILD & bzl files for a package.
  */
 function generatePackageBuildFiles(pkg) {
-  const buildFile = BUILD_FILE_HEADER + printPackage(pkg);
-  writeFileSync(path.posix.join(pkg._dir, 'BUILD.bazel'), buildFile);
+  let buildFile = printPackage(pkg);
 
-  const binBuildFile = BUILD_FILE_HEADER + printPackageBin(pkg);
-  writeFileSync(path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), binBuildFile);
+  const binBuildFile = printPackageBin(pkg);
+  if (binBuildFile.length) {
+    writeFileSync(
+        path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), BUILD_FILE_HEADER + binBuildFile);
+  }
+
+  const indexFile = printIndexBzl(pkg);
+  if (indexFile.length) {
+    writeFileSync(path.posix.join(pkg._dir, 'index.bzl'), indexFile);
+    buildFile = `${buildFile}
+# For integration testing
+exports_files(["index.bzl"])
+`;
+  }
+
+  writeFileSync(path.posix.join(pkg._dir, 'BUILD.bazel'), BUILD_FILE_HEADER + buildFile);
 }
 
 /**
@@ -951,12 +964,7 @@ npm_umd_bundle(
   return result;
 }
 
-/**
- * Given a pkg, return the skylark nodejs_binary targets for the package.
- */
-function printPackageBin(pkg) {
-  let result = '';
-
+function _findExecutables(pkg) {
   const executables = new Map();
 
   // For root packages, transform the pkg.bin entries
@@ -982,6 +990,16 @@ function printPackageBin(pkg) {
     }
   }
 
+  return executables;
+}
+
+
+/**
+ * Given a pkg, return the skylark nodejs_binary targets for the package.
+ */
+function printPackageBin(pkg) {
+  let result = '';
+  const executables = _findExecutables(pkg);
   if (executables.size) {
     result = `load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary")
 
@@ -1024,6 +1042,26 @@ nodejs_binary(
   return result;
 }
 
+function printIndexBzl(pkg) {
+  let result = '';
+  const executables = _findExecutables(pkg);
+  if (executables.size) {
+    result = `load("@build_bazel_rules_nodejs//:defs.bzl", "npm_package_bin")
+
+`;
+  }
+  for (const name of executables.keys()) {
+    result = `${result}
+
+# Generated helper macro to call ${name}
+def ${name}(**kwargs):
+    npm_package_bin(tool = "@${WORKSPACE}//${pkg._dir}/bin:${name}", **kwargs)
+`;
+  }
+
+  return result;
+}
+
 /**
  * Given a scope, return the skylark `node_module_library` target for the scope.
  */

internal/npm_install/test/check.js

@@ -56,13 +56,13 @@ module.exports = {
     '@gregmagolan/BUILD.bazel',
     '@gregmagolan/test-a/bin/BUILD.bazel',
     '@gregmagolan/test-a/BUILD.bazel',
-    '@gregmagolan/test-b/bin/BUILD.bazel',
+    '@gregmagolan/test-a/index.bzl',
     '@gregmagolan/test-b/BUILD.bazel',
     'ajv/BUILD.bazel',
     'jasmine/bin/BUILD.bazel',
     'jasmine/BUILD.bazel',
+    'jasmine/index.bzl',
     'rxjs/BUILD.bazel',
-    'unidiff/bin/BUILD.bazel',
     'unidiff/BUILD.bazel',
     'zone.js/BUILD.bazel',
   ],

internal/npm_install/test/golden/@gregmagolan/test-a/BUILD.bazel.golden

@@ -30,3 +30,4 @@ npm_umd_bundle(
     entry_point = "//:node_modules/@gregmagolan/test-a/main.js",
     package = ":test-a",
 )
+exports_files(["index.bzl"])

internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden

@@ -0,0 +1,3 @@
+load("@build_bazel_rules_nodejs//:defs.bzl", "npm_package_bin")
+def test(**kwargs):
+    npm_package_bin(tool = "@fine_grained_goldens//@gregmagolan/test-a/bin:test", **kwargs)

internal/npm_install/test/golden/BUILD.bazel.golden

@@ -4754,13 +4754,13 @@ filegroup(
     "//@gregmagolan:BUILD.bazel",
     "//@gregmagolan/test-a/bin:BUILD.bazel",
     "//@gregmagolan/test-a:BUILD.bazel",
-    "//@gregmagolan/test-b/bin:BUILD.bazel",
+    "//@gregmagolan/test-a:index.bzl",
     "//@gregmagolan/test-b:BUILD.bazel",
     "//ajv:BUILD.bazel",
     "//jasmine/bin:BUILD.bazel",
     "//jasmine:BUILD.bazel",
+    "//jasmine:index.bzl",
     "//rxjs:BUILD.bazel",
-    "//unidiff/bin:BUILD.bazel",
     "//unidiff:BUILD.bazel",
     "//zone.js:BUILD.bazel",
   ],

internal/npm_install/test/golden/jasmine/BUILD.bazel.golden

@@ -53,3 +53,4 @@ npm_umd_bundle(
     entry_point = "//:node_modules/jasmine/lib/jasmine.js",
     package = ":jasmine",
 )
+exports_files(["index.bzl"])

internal/npm_install/test/golden/jasmine/index.bzl.golden

@@ -0,0 +1,3 @@
+load("@build_bazel_rules_nodejs//:defs.bzl", "npm_package_bin")
+def jasmine(**kwargs):
+    npm_package_bin(tool = "@fine_grained_goldens//jasmine/bin:jasmine", **kwargs)

internal/npm_install/test/golden/manual_build_file_contents.golden

@@ -10,13 +10,13 @@ filegroup(
     "//@gregmagolan:BUILD.bazel",
     "//@gregmagolan/test-a/bin:BUILD.bazel",
     "//@gregmagolan/test-a:BUILD.bazel",
-    "//@gregmagolan/test-b/bin:BUILD.bazel",
+    "//@gregmagolan/test-a:index.bzl",
     "//@gregmagolan/test-b:BUILD.bazel",
     "//ajv:BUILD.bazel",
     "//jasmine/bin:BUILD.bazel",
     "//jasmine:BUILD.bazel",
+    "//jasmine:index.bzl",
     "//rxjs:BUILD.bazel",
-    "//unidiff/bin:BUILD.bazel",
     "//unidiff:BUILD.bazel",
     "//zone.js:BUILD.bazel",
   ],

packages/less/BUILD.bazel

@@ -33,6 +33,8 @@ npm_package(
     srcs = [
         "@npm_bazel_less//:package_contents",
     ],
+    # less package is deprecated
+    tags = ["do-not-publish"],
     vendor_external = [
         "npm_bazel_less",
     ],

packages/stylus/BUILD.bazel

@@ -33,6 +33,8 @@ npm_package(
     srcs = [
         "@npm_bazel_stylus//:package_contents",
     ],
+    # stylus package is deprecated
+    tags = ["do-not-publish"],
     vendor_external = [
         "npm_bazel_stylus",
     ],