Support modern location expansions for run_binary (#490)
A common point of confusion I see around `run_binary` is that it's hard coded to only expand `$(location` values which in codebases I work in are otherwise completely eliminated due to it being described as "legacy"
> location: A synonym for either execpath or rootpath, depending on the attribute being expanded. This is legacy pre-Starlark behavior and not recommended unless you really know what it does for a particular rule. See [#2475](https://github.com/bazelbuild/bazel/issues/2475#issuecomment-339318016) for details.
If `execpath` is used instead as the appropriate alternative, the rule does no do any expansion and fails the action. This change adds support for expanding all available patterns whenever they're provided.
diff --git a/docs/run_binary_doc.md b/docs/run_binary_doc.md
index cec27c1..468c7f5 100755
--- a/docs/run_binary_doc.md
+++ b/docs/run_binary_doc.md
@@ -22,10 +22,10 @@
| Name | Description | Type | Mandatory | Default |
| :------------- | :------------- | :------------- | :------------- | :------------- |
| <a id="run_binary-name"></a>name | A unique name for this target. | <a href="https://bazel.build/concepts/labels#target-names">Name</a> | required | |
-| <a id="run_binary-srcs"></a>srcs | Additional inputs of the action.<br><br>These labels are available for `$(location)` expansion in `args` and `env`. | <a href="https://bazel.build/concepts/labels">List of labels</a> | optional | `[]` |
-| <a id="run_binary-outs"></a>outs | Output files generated by the action.<br><br>These labels are available for `$(location)` expansion in `args` and `env`. | List of labels | required | |
-| <a id="run_binary-args"></a>args | Command line arguments of the binary.<br><br>Subject to [`$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables) expansion. | List of strings | optional | `[]` |
-| <a id="run_binary-env"></a>env | Environment variables of the action.<br><br>Subject to [`$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables) expansion. | <a href="https://bazel.build/rules/lib/dict">Dictionary: String -> String</a> | optional | `{}` |
-| <a id="run_binary-tool"></a>tool | The tool to run in the action.<br><br>Must be the label of a *_binary rule, of a rule that generates an executable file, or of a file that can be executed as a subprocess (e.g. an .exe or .bat file on Windows or a binary with executable permission on Linux). This label is available for `$(location)` expansion in `args` and `env`. | <a href="https://bazel.build/concepts/labels">Label</a> | required | |
+| <a id="run_binary-srcs"></a>srcs | Additional inputs of the action.<br><br>These labels are available for `$(execpath)` and `$(location)` expansion in `args` and `env`. | <a href="https://bazel.build/concepts/labels">List of labels</a> | optional | `[]` |
+| <a id="run_binary-outs"></a>outs | Output files generated by the action.<br><br>These labels are available for `$(execpath)` and `$(location)` expansion in `args` and `env`. | List of labels | required | |
+| <a id="run_binary-args"></a>args | Command line arguments of the binary.<br><br>Subject to [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables) expansion. | List of strings | optional | `[]` |
+| <a id="run_binary-env"></a>env | Environment variables of the action.<br><br>Subject to [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables) expansion. | <a href="https://bazel.build/rules/lib/dict">Dictionary: String -> String</a> | optional | `{}` |
+| <a id="run_binary-tool"></a>tool | The tool to run in the action.<br><br>Must be the label of a *_binary rule, of a rule that generates an executable file, or of a file that can be executed as a subprocess (e.g. an .exe or .bat file on Windows or a binary with executable permission on Linux). This label is available for `$(execpath)` and `$(location)` expansion in `args` and `env`. | <a href="https://bazel.build/concepts/labels">Label</a> | required | |
diff --git a/rules/run_binary.bzl b/rules/run_binary.bzl
index 7701fa0..82d6330 100644
--- a/rules/run_binary.bzl
+++ b/rules/run_binary.bzl
@@ -20,10 +20,10 @@
load("//lib:dicts.bzl", "dicts")
-def _impl(ctx):
+def _run_binary_impl(ctx):
tool_as_list = [ctx.attr.tool]
args = [
- # Expand $(location) / $(locations) in args.
+ # Expand $(execpath ...) / $(execpaths ...) / $(location ...) / $(locations ...) in args.
#
# To keep the rule simple, do not expand Make Variables (like *_binary.args usually would).
# (We can add this feature later if users ask for it.)
@@ -33,12 +33,12 @@
# tokenization they would have to write args=["'a b'"] or args=["a\\ b"]. There's no
# documented tokenization function anyway (as of 2019-05-21 ctx.tokenize exists but is
# undocumented, see https://github.com/bazelbuild/bazel/issues/8389).
- ctx.expand_location(a, tool_as_list) if "$(location" in a else a
+ ctx.expand_location(a, tool_as_list)
for a in ctx.attr.args
]
envs = {
- # Expand $(location) / $(locations) in the values.
- k: ctx.expand_location(v, tool_as_list) if "$(location" in v else v
+ # Expand $(execpath ...) / $(execpaths ...) / $(location ...) / $(locations ...) in the values.
+ k: ctx.expand_location(v, tool_as_list)
for k, v in ctx.attr.env.items()
}
ctx.actions.run(
@@ -57,7 +57,7 @@
)
run_binary = rule(
- implementation = _impl,
+ implementation = _run_binary_impl,
doc = "Runs a binary as a build action.\n\nThis rule does not require Bash (unlike" +
" `native.genrule`).",
attrs = {
@@ -66,7 +66,7 @@
" of a rule that generates an executable file, or of a file that can be" +
" executed as a subprocess (e.g. an .exe or .bat file on Windows or a binary" +
" with executable permission on Linux). This label is available for" +
- " `$(location)` expansion in `args` and `env`.",
+ " `$(execpath)` and `$(location)` expansion in `args` and `env`.",
executable = True,
allow_files = True,
mandatory = True,
@@ -74,22 +74,22 @@
),
"env": attr.string_dict(
doc = "Environment variables of the action.\n\nSubject to " +
- " [`$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" +
+ " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" +
" expansion.",
),
"srcs": attr.label_list(
allow_files = True,
doc = "Additional inputs of the action.\n\nThese labels are available for" +
- " `$(location)` expansion in `args` and `env`.",
+ " `$(execpath)` and `$(location)` expansion in `args` and `env`.",
),
"outs": attr.output_list(
mandatory = True,
doc = "Output files generated by the action.\n\nThese labels are available for" +
- " `$(location)` expansion in `args` and `env`.",
+ " `$(execpath)` and `$(location)` expansion in `args` and `env`.",
),
"args": attr.string_list(
doc = "Command line arguments of the binary.\n\nSubject to" +
- " [`$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" +
+ " [`$(execpath)` and `$(location)`](https://bazel.build/reference/be/make-variables#predefined_label_variables)" +
" expansion.",
),
},
diff --git a/tests/run_binary/BUILD b/tests/run_binary/BUILD
index 0b02087..18682f6 100644
--- a/tests/run_binary/BUILD
+++ b/tests/run_binary/BUILD
@@ -24,6 +24,8 @@
"arg2=(bar)",
"ENV_LOCATION=(a tests/run_binary/BUILD)",
"ENV_LOCATIONS=(b\\ tests/run_binary/BUILD tests/run_binary/printargs.cc)",
+ "ENV_EXECPATH=(a tests/run_binary/BUILD)",
+ "ENV_EXECPATHS=(b\\ tests/run_binary/BUILD tests/run_binary/printargs.cc)",
"ENV_COMPLEX=(xx/yy \\\"zz)",
"ENV_PATH_BASH=($PATH)",
"ENV_PATH_CMD=(%PATH%)",
@@ -51,12 +53,14 @@
# Bash-tokenized, and should appear the same for Windows .bat files and Bash
# .sh scripts.
env = {
+ "ENV_COMPLEX": "xx/yy \\\"zz",
+ "ENV_EXECPATH": "a $(execpath BUILD)",
+ "ENV_EXECPATHS": "b\\ $(execpaths :dummy_srcs)",
# Testing $(location) expansion only on source files so the result is
# predictable. The path of generated files depends on the target
# platform.
"ENV_LOCATION": "a $(location BUILD)",
"ENV_LOCATIONS": "b\\ $(locations :dummy_srcs)",
- "ENV_COMPLEX": "xx/yy \\\"zz",
"ENV_PATH_BASH": "$PATH",
"ENV_PATH_CMD": "%PATH%",
"OUT": "$(location run_script.out)",
@@ -76,6 +80,8 @@
"@echo>>%OUT% arg2=(%2)",
"@echo>>%OUT% ENV_LOCATION=(%ENV_LOCATION%)",
"@echo>>%OUT% ENV_LOCATIONS=(%ENV_LOCATIONS%)",
+ "@echo>>%OUT% ENV_EXECPATH=(%ENV_EXECPATH%)",
+ "@echo>>%OUT% ENV_EXECPATHS=(%ENV_EXECPATHS%)",
"@echo>>%OUT% ENV_COMPLEX=(%ENV_COMPLEX%)",
"@echo>>%OUT% ENV_PATH_BASH=(%ENV_PATH_BASH%)",
"@echo>>%OUT% ENV_PATH_CMD=(%ENV_PATH_CMD%)",
@@ -86,6 +92,8 @@
"echo >> \"$OUT\" \"arg2=($2)\"",
"echo >> \"$OUT\" \"ENV_LOCATION=($ENV_LOCATION)\"",
"echo >> \"$OUT\" \"ENV_LOCATIONS=($ENV_LOCATIONS)\"",
+ "echo >> \"$OUT\" \"ENV_EXECPATH=($ENV_EXECPATH)\"",
+ "echo >> \"$OUT\" \"ENV_EXECPATHS=($ENV_EXECPATHS)\"",
"echo >> \"$OUT\" \"ENV_COMPLEX=($ENV_COMPLEX)\"",
"echo >> \"$OUT\" \"ENV_PATH_BASH=($ENV_PATH_BASH)\"",
"echo >> \"$OUT\" \"ENV_PATH_CMD=($ENV_PATH_CMD)\"",
@@ -114,9 +122,13 @@
"arg5=(tests/run_binary/BUILD)",
"arg6=(tests/run_binary/BUILD tests/run_binary/printargs.cc)",
"arg7=('tests/run_binary/BUILD $tests/run_binary/BUILD')",
- "arg8=($PATH)",
- "arg9=($$PATH)",
- "arg10=(${PATH})",
+ "arg8=(tests/run_binary/BUILD)",
+ "arg9=(tests/run_binary/BUILD tests/run_binary/printargs.cc)",
+ "arg10=('tests/run_binary/BUILD $tests/run_binary/BUILD')",
+ "arg11=($PATH)",
+ "arg12=($$PATH)",
+ "arg13=(${PATH})",
+ "arg14=($(echo hello))",
# Add trailing newline, as printed by printargs.
"",
],
@@ -136,15 +148,20 @@
"\"c d\"",
"e\\ f",
"xx/yy\\ \\\"zz",
- # Testing $(location) expansion only on source files so the result is
+ # Testing $(execpath) expansion only on source files so the result is
# predictable. The path of generated files depends on the target
# platform.
+ "$(execpath BUILD)",
+ "$(execpaths :dummy_srcs)",
+ "'$(execpath BUILD) $$(execpath BUILD)'",
+ # Test the legacy 'location' expansions
"$(location BUILD)",
"$(locations :dummy_srcs)",
"'$(location BUILD) $$(location BUILD)'",
"$PATH",
"$$PATH",
"${PATH}",
+ "$(echo hello)",
],
# Not testing any complex envvars here, because we already did in
# "run_script".
