From 6d8c625d5160af2af2666f96efcc848db0528e5d Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 20 Sep 2019 13:19:50 -0700 Subject: [PATCH] feat(builtin): npm_package_bin can produce directory output (#1164) --- internal/node/npm_package_bin.bzl | 33 ++++++++++++++++--- internal/node/test/BUILD.bazel | 22 ++++++++++++- internal/node/test/copy_to_directory.js | 14 ++++++++ internal/node/test/npm_package_bin.spec.js | 9 +++-- internal/npm_install/generate_build_file.js | 2 +- .../@gregmagolan/test-a/index.bzl.golden | 2 +- .../test/golden/jasmine/index.bzl.golden | 2 +- 7 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 internal/node/test/copy_to_directory.js diff --git a/internal/node/npm_package_bin.bzl b/internal/node/npm_package_bin.bzl index 263be68603..9fc8dd92aa 100644 --- a/internal/node/npm_package_bin.bzl +++ b/internal/node/npm_package_bin.bzl @@ -8,6 +8,7 @@ _ATTRS = { "outs": attr.output_list(), "args": attr.string_list(mandatory = True), "data": attr.label_list(allow_files = True, aspects = [module_mappings_aspect]), + "out_dir": attr.string(), "tool": attr.label( executable = True, cfg = "host", @@ -15,19 +16,36 @@ _ATTRS = { ), } +# Need a custom expand_location function +# because the out_dir is a tree artifact +# so we weren't able to give it a label +def _expand_location(ctx, s): + s = s.replace("$@", "/".join([ctx.bin_dir.path, ctx.label.package, ctx.attr.out_dir])) + return ctx.expand_location(s, targets = ctx.attr.data) + def _impl(ctx): + if ctx.attr.out_dir and ctx.attr.outs: + fail("Only one of out_dir and outs may be specified") + if not ctx.attr.out_dir and not ctx.attr.outs: + fail("One of out_dir and outs must be specified") + args = ctx.actions.args() inputs = ctx.files.data[:] - outputs = ctx.outputs.outs + outputs = [] + if ctx.attr.out_dir: + outputs = [ctx.actions.declare_directory(ctx.attr.out_dir)] + else: + outputs = ctx.outputs.outs register_node_modules_linker(ctx, args, inputs) for a in ctx.attr.args: - args.add(ctx.expand_location(a, targets = ctx.attr.data)) + args.add(_expand_location(ctx, a)) ctx.actions.run( executable = ctx.executable.tool, inputs = inputs, outputs = outputs, arguments = [args], ) + return [DefaultInfo(files = depset(outputs))] _npm_package_bin = rule( _impl, @@ -45,17 +63,22 @@ def npm_package_bin(tool = None, package = None, package_bin = None, **kwargs): https://docs.bazel.build/versions/master/skylark/macros.html#full-example Args: - data: identical to [genrule.srcs](https://docs.bazel.build/versions/master/be/general.html#genrule.srcs) + data: similar to [genrule.srcs](https://docs.bazel.build/versions/master/be/general.html#genrule.srcs) may also include 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) + outs: similar to [genrule.outs](https://docs.bazel.build/versions/master/be/general.html#genrule.outs) + out_dir: use this instead of `outs` if you want the output to be a directory + Exactly one of `outs`, `out_dir` may be used. + If you output a directory, there can only be one output. 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 + You may also refer to the location of the out_dir with the special `$@` replacement, like genrule. 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 + tool: a label for a binary to run, like `@npm//terser/bin:terser`. This is the longer form of package/package_bin. + Note that you can also refer to a binary in your local workspace. """ if not tool: if not package: diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel index 5cffc7b065..6092128fda 100644 --- a/internal/node/test/BUILD.bazel +++ b/internal/node/test/BUILD.bazel @@ -158,8 +158,28 @@ npm_package_bin( package = "terser", ) +nodejs_binary( + name = "copy_to_directory", + entry_point = "copy_to_directory.js", +) + +npm_package_bin( + name = "produces_directory", + args = [ + "--output-dir", + "$@", + "$(location terser_input.js)", + ], + data = ["terser_input.js"], + out_dir = "dir_output", + tool = ":copy_to_directory", +) + nodejs_test( name = "npm_package_bin_test", - data = ["minified.js"], + data = [ + "minified.js", + "produces_directory", + ], entry_point = "npm_package_bin.spec.js", ) diff --git a/internal/node/test/copy_to_directory.js b/internal/node/test/copy_to_directory.js new file mode 100644 index 0000000000..5221bedf3f --- /dev/null +++ b/internal/node/test/copy_to_directory.js @@ -0,0 +1,14 @@ +const fs = require('fs'); +const path = require('path'); + +// argv[2] is always --output-dir +const out_dir = process.argv[3]; + +try { + fs.mkdirSync(out_dir); +} catch { +} + +for (const input of process.argv.slice(4)) { + fs.copyFileSync(input, path.join(out_dir, path.basename(input))); +} diff --git a/internal/node/test/npm_package_bin.spec.js b/internal/node/test/npm_package_bin.spec.js index 1aa20b6068..161360eddf 100644 --- a/internal/node/test/npm_package_bin.spec.js +++ b/internal/node/test/npm_package_bin.spec.js @@ -1,8 +1,13 @@ const fs = require('fs'); const path = require('path'); - -const content = fs.readFileSync(path.join(require.resolve(__dirname + '/minified.js')), 'utf-8'); +const min_js = path.join(require.resolve(__dirname + '/minified.js')); +const content = fs.readFileSync(min_js, 'utf-8'); if (!content.includes('{console.error("thing")}')) { console.error(content); process.exitCode = 1; } + +const dir = fs.readdirSync(path.join(path.dirname(min_js), 'dir_output')); +if (!dir.includes('terser_input.js')) { + console.error(dir), process.exitCode = 1; +} \ No newline at end of file diff --git a/internal/npm_install/generate_build_file.js b/internal/npm_install/generate_build_file.js index 4217b13274..dce5865dc6 100644 --- a/internal/npm_install/generate_build_file.js +++ b/internal/npm_install/generate_build_file.js @@ -1063,7 +1063,7 @@ function printIndexBzl(pkg) { # Generated helper macro to call ${name} def ${name.replace(/-/g, '_')}(**kwargs): - if "outs" in kwargs: + if "outs" in kwargs or "out_dir" in kwargs: npm_package_bin(tool = "@${WORKSPACE}//${pkg._dir}/bin:${name}", **kwargs) else: nodejs_binary( 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 2b1df5dc3c..f623e0dd93 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 @@ -1,6 +1,6 @@ load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary", "npm_package_bin") def test(**kwargs): - if "outs" in kwargs: + if "outs" in kwargs or "out_dir" in kwargs: npm_package_bin(tool = "@fine_grained_goldens//@gregmagolan/test-a/bin:test", **kwargs) else: nodejs_binary( diff --git a/internal/npm_install/test/golden/jasmine/index.bzl.golden b/internal/npm_install/test/golden/jasmine/index.bzl.golden index 2480a7eafb..55e5649fb7 100644 --- a/internal/npm_install/test/golden/jasmine/index.bzl.golden +++ b/internal/npm_install/test/golden/jasmine/index.bzl.golden @@ -1,6 +1,6 @@ load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary", "npm_package_bin") def jasmine(**kwargs): - if "outs" in kwargs: + if "outs" in kwargs or "out_dir" in kwargs: npm_package_bin(tool = "@fine_grained_goldens//jasmine/bin:jasmine", **kwargs) else: nodejs_binary(