Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add npm_package_bin rule #1139

Merged
merged 1 commit into from
Sep 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
2 changes: 2 additions & 0 deletions defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down
17 changes: 14 additions & 3 deletions e2e/less/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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(
Expand Down
6 changes: 4 additions & 2 deletions e2e/less/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
18 changes: 15 additions & 3 deletions e2e/stylus/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user, can I use a relative path here such as ../test.css? Not that it makes sense in this example but in general is that possible?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You linked the docs which answers the question :) https://docs.bazel.build/versions/master/be/general.html#genrule.srcs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this will be tough for users I think, because if you just write "test.css" it will write to the input folder. if you're lucky you get an error about that folder being readonly in the sandbox, more likely it writes it and you get an error that bazel says the declared output wasn't produced.
But this is standard genrule behavior so not strictly our job to improve that?

"--compress",
"--sourcemap",
],
)

nodejs_test(
Expand Down
68 changes: 68 additions & 0 deletions internal/node/npm_package_bin.bzl
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could allow a bit more flexibility by allow user to override the @npm workspace without specifying the whole tool? But maybe having the tool override is sufficient

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we could easily have another attribute for the package workspace. but that does make the API surface of this rule bigger just to preserve some syntax sugar, which seems probably wrong to me.


_npm_genrule(
tool = tool,
**kwargs
)
20 changes: 19 additions & 1 deletion internal/node/test/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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")
Expand Down Expand Up @@ -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",
)
8 changes: 8 additions & 0 deletions internal/node/test/npm_package_bin.spec.js
Original file line number Diff line number Diff line change
@@ -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;
}
5 changes: 5 additions & 0 deletions internal/node/test/terser_input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class A {
doThing() {
console.error('thing');
}
}
60 changes: 49 additions & 11 deletions internal/npm_install/generate_build_file.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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.
*/
Expand Down
4 changes: 2 additions & 2 deletions internal/npm_install/test/check.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@ npm_umd_bundle(
entry_point = "//:node_modules/@gregmagolan/test-a/main.js",
package = ":test-a",
)
exports_files(["index.bzl"])
Original file line number Diff line number Diff line change
@@ -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)

This file was deleted.

4 changes: 2 additions & 2 deletions internal/npm_install/test/golden/BUILD.bazel.golden
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,4 @@ npm_umd_bundle(
entry_point = "//:node_modules/jasmine/lib/jasmine.js",
package = ":jasmine",
)
exports_files(["index.bzl"])
3 changes: 3 additions & 0 deletions internal/npm_install/test/golden/jasmine/index.bzl.golden
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down

This file was deleted.

2 changes: 2 additions & 0 deletions packages/less/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ npm_package(
srcs = [
"@npm_bazel_less//:package_contents",
],
# less package is deprecated
tags = ["do-not-publish"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update release script to key off of this tag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it already does, that's how we avoided publishing @bazel/rollup and @bazel/terser to this point.

vendor_external = [
"npm_bazel_less",
],
Expand Down
2 changes: 2 additions & 0 deletions packages/stylus/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down