-
Notifications
You must be signed in to change notification settings - Fork 520
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
feat(builtin): npm_package_bin can produce directory output #1164
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,26 +8,44 @@ _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", | ||
mandatory = True, | ||
), | ||
} | ||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: mention this is a string and relative to the current package? |
||
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: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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))); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
internal/npm_install/test/golden/@gregmagolan/test-a/index.bzl.golden
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be added to doc string for rule and comment on
$@
usage in argsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could out_dir be a label to give the user more freedom as to where it will be? as a string it is limited to subdirs of the package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's possible since https://docs.bazel.build/versions/master/skylark/lib/actions.html#declare_directory is the only API to produce TreeArtifact and it takes string - and you can't use a label to reference something that doesn't exist yet
anyway it's probably good that the output directory has to be inside the package that owns it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added missing doc
Also handle the case of
args=["--out-dir=$@"]
(it's a substring of the arg)