Skip to content

Commit

Permalink
refactor: remove templated_args_file from nodejs_binary & nodejs_test
Browse files Browse the repository at this point in the history
BREAKING CHANGE:
`templated_args_file` removed from nodejs_binary, nodejs_test & jasmine_node_test. This was a separation of concerns and complicated node.bzl more than necessary while also being rigid in how the params file is formatted. It is more flexible to expose this functionality as another simple rule named params_file.

To match standard $(location) and $(locations) expansion, params_file args location expansions are also in the standard short_path form (this differs from the old templated_args behavior which was not Bazel idiomatic)
Usage example:

```
load("@build_bazel_rules_nodejs//:index.bzl", "params_file", "nodejs_binary")

params_file(
    name = "params_file",
    args = [
        "--some_param",
        "$(location //path/to/some:file)",
        "--some_other_param",
        "$(location //path/to/some/other:file)",
    ],
    data = [
        "//path/to/some:file",
        "//path/to/some/other:file",
    ],
)

nodejs_binary(
    name = "my_binary",
    data = [":params_file"],
    entry_point = ":my_binary.js",
    templated_args = ["$(location :params_file)"],
)
```
  • Loading branch information
gregmagolan authored and alexeagle committed Dec 20, 2019
1 parent 1860a6a commit 799acb4
Show file tree
Hide file tree
Showing 8 changed files with 172 additions and 42 deletions.
2 changes: 2 additions & 0 deletions index.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Users should not load files under "/internal"
load("//internal/common:check_bazel_version.bzl", _check_bazel_version = "check_bazel_version")
load("//internal/common:check_version.bzl", "check_version")
load("//internal/common:copy_to_bin.bzl", _copy_to_bin = "copy_to_bin")
load("//internal/common:params_file.bzl", _params_file = "params_file")
load(
"//internal/node:node.bzl",
_nodejs_binary = "nodejs_binary",
Expand All @@ -39,6 +40,7 @@ pkg_npm = _pkg_npm
npm_package_bin = _npm_bin
pkg_web = _pkg_web
copy_to_bin = _copy_to_bin
params_file = _params_file
# 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
2 changes: 2 additions & 0 deletions index.for_docs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ This differs from :index.bzl because we don't have wrapping macros that hide the

load("//internal/common:check_bazel_version.bzl", _check_bazel_version = "check_bazel_version")
load("//internal/common:copy_to_bin.bzl", _copy_to_bin = "copy_to_bin")
load("//internal/common:params_file.bzl", _params_file = "params_file")
load("//internal/node:node.bzl", _nodejs_binary = "nodejs_binary", _nodejs_test = "nodejs_test")
load("//internal/node:node_repositories.bzl", _node_repositories = "node_repositories_rule")
load("//internal/node:npm_package_bin.bzl", _npm_bin = "npm_package_bin")
Expand All @@ -27,6 +28,7 @@ load("//internal/pkg_web:pkg_web.bzl", _pkg_web = "pkg_web")

check_bazel_version = _check_bazel_version
copy_to_bin = _copy_to_bin
params_file = _params_file
nodejs_binary = _nodejs_binary
nodejs_test = _nodejs_test
node_repositories = _node_repositories
Expand Down
7 changes: 1 addition & 6 deletions internal/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,9 @@ filegroup(

nodejs_test(
name = "package_json_test",
data = [
"check_package_json.js",
"//:package.json",
],
data = ["//:package.json"],
entry_point = ":check_package_json.js",
templated_args = [BAZEL_VERSION],
# Not necessary here, just a regression test for #1043
templated_args_file = "package_json_test.params",
)

# Detect if the build is running under --stamp
Expand Down
5 changes: 2 additions & 3 deletions internal/check_package_json.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@
'use strict';

const args = process.argv.slice(2);
// The arguments are passed via a params file
const paramsFile = require.resolve('build_bazel_rules_nodejs/' + args[0]);
const [BAZEL_VERSION] = require('fs').readFileSync(paramsFile, 'utf-8').split(/\r?\n/);

const BAZEL_VERSION = args[0];

const packageJson = require('build_bazel_rules_nodejs/package.json');

Expand Down
98 changes: 98 additions & 0 deletions internal/common/params_file.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# Copyright 2019 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Generates a params file from a list of arguments.
"""

load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")

_DOC = """Generates a params file from a list of arguments."""

_ATTRS = {
"out": attr.output(
doc = """Path of the output file, relative to this package.""",
mandatory = True,
),
"args": attr.string_list(
doc = """Arguments to concatenate into a params file.
Subject to $(location) substitutions""",
),
"data": attr.label_list(
doc = """Data for $(location) expansions in args.""",
allow_files = True,
),
"is_windows": attr.bool(mandatory = True),
"newline": attr.string(
doc = """one of ["auto", "unix", "windows"]: line endings to use. "auto"
for platform-determined, "unix" for LF, and "windows" for CRLF.""",
values = ["unix", "windows", "auto"],
default = "auto",
),
}

def _impl(ctx):
if ctx.attr.newline == "auto":
newline = "\r\n" if ctx.attr.is_windows else "\n"
elif ctx.attr.newline == "windows":
newline = "\r\n"
else:
newline = "\n"

# ctx.actions.write creates a FileWriteAction which uses UTF-8 encoding.
ctx.actions.write(
output = ctx.outputs.out,
content = newline.join([expand_location_into_runfiles(ctx, a, ctx.attr.data) for a in ctx.attr.args]),
is_executable = False,
)
files = depset(direct = [ctx.outputs.out])
runfiles = ctx.runfiles(files = [ctx.outputs.out])
return [DefaultInfo(files = files, runfiles = runfiles)]

_params_file = rule(
implementation = _impl,
provides = [DefaultInfo],
attrs = _ATTRS,
doc = _DOC,
)

def params_file(
name,
out,
args = [],
newline = "auto",
**kwargs):
"""Generates a UTF-8 encoded params file from a list of arguments.
Handles $(location) expansions for arguments.
Args:
name: Name of the rule.
out: Path of the output file, relative to this package.
args: Arguments to concatenate into a params file.
Subject to $(location) substitutions
newline: one of ["auto", "unix", "windows"]: line endings to use. "auto"
for platform-determined, "unix" for LF, and "windows" for CRLF.
**kwargs: further keyword arguments, e.g. <code>visibility</code>
"""
_params_file(
name = name,
out = out,
args = args,
newline = newline or "auto",
is_windows = select({
"@bazel_tools//src/conditions:host_windows": True,
"//conditions:default": False,
}),
**kwargs
)
30 changes: 30 additions & 0 deletions internal/common/test/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test")
load("//internal/common:copy_to_bin.bzl", "copy_to_bin")
load("//internal/common:params_file.bzl", "params_file")

licenses(["notice"])

Expand All @@ -18,3 +20,31 @@ copy_to_bin(
name = "a",
srcs = ["foo/bar/a.txt"],
)

filegroup(
name = "locations_in",
srcs = [
"check_params_file.js",
"//:package.json",
],
)

params_file(
name = "params_file",
out = ":params_file.out",
args = [
"$(location //:package.json)",
"$(locations :locations_in)",
],
data = [
":locations_in",
"//:package.json",
],
)

nodejs_test(
name = "params_file_test",
data = [":params_file.out"],
entry_point = ":check_params_file.js",
templated_args = ["$(location :params_file.out)"],
)
36 changes: 36 additions & 0 deletions internal/common/test/check_params_file.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* @license
* Copyright 2017 The Bazel Authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
*
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const args = process.argv.slice(2);

// The arguments are passed via a params file
const [a1, a2] = require('fs').readFileSync(require.resolve(args[0]), 'utf-8').split(/\r?\n/);

const a1_exp = 'build_bazel_rules_nodejs/package.json';
const a2_exp =
'build_bazel_rules_nodejs/package.json build_bazel_rules_nodejs/internal/common/test/check_params_file.js';

if (a1 !== a1_exp) {
console.error(`expected first argument in params file to be '${a1_exp}'`)
process.exit(1);
}

if (a2 !== a2_exp) {
console.error(`expected second argument in params file to be '${a2_exp}'`)
process.exit(1);
}
34 changes: 1 addition & 33 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -201,37 +201,12 @@ def _nodejs_binary_impl(ctx):
node_tool_files.append(ctx.file._bazel_require_script)
node_tool_files.append(node_modules_manifest)

if not ctx.outputs.templated_args_file:
templated_args = ctx.attr.templated_args
else:
# Distribute the templated_args between the params file and the node options
params = []
templated_args = []
for a in ctx.attr.templated_args:
if a.startswith("--node_options="):
templated_args.append(a)
else:
params.append(a)

# Put the params into the params file
ctx.actions.write(
output = ctx.outputs.templated_args_file,
content = "\n".join([expand_location_into_runfiles(ctx, p, ctx.attr.data) for p in params]),
is_executable = False,
)

# after the node_options args, pass the params file arg
templated_args.append(ctx.outputs.templated_args_file.short_path)

# also be sure to include the params file in the program inputs
node_tool_files.append(ctx.outputs.templated_args_file)

is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]

substitutions = {
"TEMPLATED_args": " ".join([
expand_location_into_runfiles(ctx, a, ctx.attr.data)
for a in templated_args
for a in ctx.attr.templated_args
]),
"TEMPLATED_bazel_require_script": _to_manifest_path(ctx, ctx.file._bazel_require_script),
"TEMPLATED_env_vars": env_vars,
Expand Down Expand Up @@ -458,13 +433,6 @@ jasmine_node_test(
`--node_options=--preserve-symlinks`
""",
),
"templated_args_file": attr.output(
mandatory = False,
doc = """If specified, arguments specified in `templated_args` are instead written to this file,
which is then passed as an argument to the program. Arguments prefixed with `--node_options=` are
passed directly to node and not included in the params file.
""",
),
"_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"),
Expand Down

0 comments on commit 799acb4

Please sign in to comment.