feat: add run_binary with output directory support & improved makevar expansion (#57)

README.md

@@ -1,9 +1,9 @@
 # Aspect's Bazel helpers library
 
-This is code we would contribute to bazel-skylib,
+This is code we would contribute to
-but the declared scope of that project is narrow
+[bazel-skylib](https://github.com/bazelbuild/bazel-skylib), but the declared
-and it's very difficult to get anyone's attention
+scope of that project is narrow and it's very difficult to get anyone's
-to review PRs there.
+attention to review PRs there.
 
 ## Installation
 

docs/BUILD.bazel

@@ -2,6 +2,11 @@
 # so that the dependency on stardoc doesn't leak to them.
 load("//lib:docs.bzl", "stardoc_with_diff_test", "update_docs")
 
+stardoc_with_diff_test(
+    name = "copy_file",
+    bzl_library_target = "//lib:copy_file",
+)
+
 stardoc_with_diff_test(
     name = "copy_to_directory",
     bzl_library_target = "//lib:copy_to_directory",
@@ -53,6 +58,11 @@ stardoc_with_diff_test(
     bzl_library_target = "//lib:output_files",
 )
 
+stardoc_with_diff_test(
+    name = "run_binary",
+    bzl_library_target = "//lib:run_binary",
+)
+
 stardoc_with_diff_test(
     name = "transitions",
     bzl_library_target = "//lib:transitions",

docs/copy_file.md

@@ -0,0 +1,48 @@
+<!-- Generated with Stardoc: http://skydoc.bazel.build -->
+
+A rule that copies a file to another place.
+
+native.genrule() is sometimes used to copy files (often wishing to rename them).
+The 'copy_file' rule does this with a simpler interface than genrule.
+
+The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command
+on Windows (no Bash is required).
+
+This fork of bazel-skylib's copy_file adds directory support.
+
+
+<a id="#copy_file"></a>
+
+## copy_file
+
+<pre>
+copy_file(<a href="#copy_file-name">name</a>, <a href="#copy_file-src">src</a>, <a href="#copy_file-out">out</a>, <a href="#copy_file-is_directory">is_directory</a>, <a href="#copy_file-is_executable">is_executable</a>, <a href="#copy_file-allow_symlink">allow_symlink</a>, <a href="#copy_file-kwargs">kwargs</a>)
+</pre>
+
+Copies a file or directory to another location.
+
+`native.genrule()` is sometimes used to copy files (often wishing to rename them). The 'copy_file' rule does this with a simpler interface than genrule.
+
+This rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command on Windows (no Bash is required).
+
+If using this rule with source directories, it is recommended that you use the
+`--host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1` startup option so that changes
+to files within source directories are detected. See
+https://github.com/bazelbuild/bazel/commit/c64421bc35214f0414e4f4226cc953e8c55fa0d2
+for more context.
+
+
+**PARAMETERS**
+
+
+| Name  | Description | Default Value |
+| :------------- | :------------- | :------------- |
+| <a id="copy_file-name"></a>name |  Name of the rule.   |  none |
+| <a id="copy_file-src"></a>src |  A Label. The file or directory to make a copy of. (Can also be the label of a rule that generates a file or directory.)   |  none |
+| <a id="copy_file-out"></a>out |  Path of the output file, relative to this package.   |  none |
+| <a id="copy_file-is_directory"></a>is_directory |  treat the source file as a directory Workaround for https://github.com/bazelbuild/bazel/issues/12954   |  <code>False</code> |
+| <a id="copy_file-is_executable"></a>is_executable |  A boolean. Whether to make the output file executable. When True, the rule's output can be executed using <code>bazel run</code> and can be in the srcs of binary and test rules that require executable sources. WARNING: If <code>allow_symlink</code> is True, <code>src</code> must also be executable.   |  <code>False</code> |
+| <a id="copy_file-allow_symlink"></a>allow_symlink |  A boolean. Whether to allow symlinking instead of copying. When False, the output is always a hard copy. When True, the output *can* be a symlink, but there is no guarantee that a symlink is created (i.e., at the time of writing, we don't create symlinks on Windows). Set this to True if you need fast copying and your tools can handle symlinks (which most UNIX tools can).   |  <code>False</code> |
+| <a id="copy_file-kwargs"></a>kwargs |  further keyword arguments, e.g. <code>visibility</code>   |  none |
+
+

docs/expand_make_vars.md

@@ -44,7 +44,7 @@ expand_locations(<a href="#expand_locations-ctx">ctx</a>, <a href="#expand_locat
 
 Expand location templates.
 
-Expands all `$(execpath ...)`, `$(rootpath ...)` and legacy `$(location ...)` templates in the
+Expands all `$(execpath ...)`, `$(rootpath ...)` and deprecated `$(location ...)` templates in the
 given string by replacing with the expanded path. Expansion only works for labels that point to direct dependencies
 of this rule or that are explicitly listed in the optional argument targets.
 
@@ -64,20 +64,7 @@ This is of the format:
 - `<bin_dir>/path/to/file`
 - `<bin_dir>/external/external_repo/path/to/file`
 
-The legacy `$(location)` and `$(locations)` expansions are deprecated as they return the runfiles manifest path of the
+The deprecated `$(location)` and `$(locations)` expansions returns either the execpath or rootpath depending on the context.
-format `repo/path/to/file` which behave differently than the built-in `$(location)` expansion in args of *_binary
-and *_test rules which returns the rootpath.
-See https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-binaries.
-
-The legacy `$(location)` and `$(locations)` expansion also differs from how the builtin `ctx.expand_location()` expansions
-of `$(location)` and `$(locations)` behave as that function returns either the execpath or rootpath depending on the context.
-See https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables.
-
-The behavior of `$(location)` and `$(locations)` expansion will be fixed in a future major release to match the
-to default Bazel behavior and return the same path as `ctx.expand_location()` returns for these.
-
-The recommended approach is to now use `$(rootpath)` where you previously used $(location). See the docstrings
-of `nodejs_binary` or `params_file` for examples of how to use `$(rootpath)` in `templated_args` and `args` respectively.
 
 
 **PARAMETERS**

docs/run_binary.md

@@ -0,0 +1,31 @@
+<!-- Generated with Stardoc: http://skydoc.bazel.build -->
+
+Runs a binary as a build action. This rule does not require Bash (unlike native.genrule()).
+
+This fork of bazel-skylib's run_binary adds directory output support and better makevar expansions.
+
+
+<a id="#run_binary"></a>
+
+## run_binary
+
+<pre>
+run_binary(<a href="#run_binary-name">name</a>, <a href="#run_binary-args">args</a>, <a href="#run_binary-env">env</a>, <a href="#run_binary-output_dir">output_dir</a>, <a href="#run_binary-outs">outs</a>, <a href="#run_binary-srcs">srcs</a>, <a href="#run_binary-tool">tool</a>)
+</pre>
+
+Runs a binary as a build action.<br/><br/>This rule does not require Bash (unlike <code>native.genrule</code>).
+
+**ATTRIBUTES**
+
+
+| Name  | Description | Type | Mandatory | Default |
+| :------------- | :------------- | :------------- | :------------- | :------------- |
+| <a id="run_binary-name"></a>name |  A unique name for this target.   | <a href="https://bazel.build/docs/build-ref.html#name">Name</a> | required |  |
+| <a id="run_binary-args"></a>args |  Command line arguments of the binary.&lt;br/&gt;&lt;br/&gt;Subject to&lt;code&gt;&lt;a href="https://docs.bazel.build/versions/main/be/make-variables.html#location"&gt;$(location)&lt;/a&gt;&lt;/code&gt; expansion.   | List of strings | optional | [] |
+| <a id="run_binary-env"></a>env |  Environment variables of the action.&lt;br/&gt;&lt;br/&gt;Subject to  &lt;code&gt;&lt;a href="https://docs.bazel.build/versions/main/be/make-variables.html#location"&gt;$(location)&lt;/a&gt;&lt;/code&gt; expansion.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
+| <a id="run_binary-output_dir"></a>output_dir |  Set to True if you want the output to be a directory. Exactly one of <code>outs</code>, <code>output_dir</code> may be used. If you output a directory, there can only be one output, which will be a directory named the same as the target.   | Boolean | optional | False |
+| <a id="run_binary-outs"></a>outs |  Output files generated by the action.&lt;br/&gt;&lt;br/&gt;These labels are available for &lt;code&gt;$(location)&lt;/code&gt; expansion in &lt;code&gt;args&lt;/code&gt; and &lt;code&gt;env&lt;/code&gt;.   | List of labels | optional |  |
+| <a id="run_binary-srcs"></a>srcs |  Additional inputs of the action.&lt;br/&gt;&lt;br/&gt;These labels are available for &lt;code&gt;$(location)&lt;/code&gt; expansion in &lt;code&gt;args&lt;/code&gt; and &lt;code&gt;env&lt;/code&gt;.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
+| <a id="run_binary-tool"></a>tool |  The tool to run in the action.&lt;br/&gt;&lt;br/&gt;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 &lt;code&gt;$(location)&lt;/code&gt; expansion in &lt;code&gt;args&lt;/code&gt; and &lt;code&gt;env&lt;/code&gt;.   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | required |  |
+
+

lib/BUILD.bazel

@@ -8,6 +8,11 @@ exports_files(
     ],
 )
 
+toolchain_type(
+    name = "jq_toolchain_type",
+    visibility = ["//visibility:public"],
+)
+
 bzl_library(
     name = "docs",
     srcs = ["docs.bzl"],
@@ -64,6 +69,13 @@ bzl_library(
     deps = ["//lib/private:output_files"],
 )
 
+bzl_library(
+    name = "copy_file",
+    srcs = ["copy_file.bzl"],
+    visibility = ["//visibility:public"],
+    deps = ["//lib/private:copy_file"],
+)
+
 bzl_library(
     name = "copy_to_directory",
     srcs = ["copy_to_directory.bzl"],
@@ -71,11 +83,6 @@ bzl_library(
     deps = ["//lib/private:copy_to_directory"],
 )
 
-toolchain_type(
-    name = "jq_toolchain_type",
-    visibility = ["//visibility:public"],
-)
-
 bzl_library(
     name = "write_source_files",
     srcs = ["write_source_files.bzl"],
@@ -90,11 +97,18 @@ bzl_library(
 
 bzl_library(
     name = "diff_test",
-    srcs = ["diff_test.bzl"],  # keep
+    srcs = ["diff_test.bzl"],
     visibility = ["//visibility:public"],
     deps = ["//lib/private:diff_test"],
 )
 
+bzl_library(
+    name = "run_binary",
+    srcs = ["run_binary.bzl"],
+    visibility = ["//visibility:public"],
+    deps = ["//lib/private:run_binary"],
+)
+
 bzl_library(
     name = "transitions",
     srcs = ["transitions.bzl"],

lib/copy_file.bzl

@@ -25,6 +25,8 @@ The 'copy_file' rule does this with a simpler interface than genrule.
 
 The rule uses a Bash command on Linux/macOS/non-Windows, and a cmd.exe command
 on Windows (no Bash is required).
+
+This fork of bazel-skylib's copy_file adds directory support.
 """
 
 load(

lib/private/BUILD.bazel

@@ -5,6 +5,13 @@ exports_files(
     visibility = ["//docs:__pkg__"],
 )
 
+bzl_library(
+    name = "copy_file",
+    srcs = ["copy_file.bzl"],
+    visibility = ["//lib:__subpackages__"],
+    deps = [":directory_path"],
+)
+
 bzl_library(
     name = "copy_to_directory",
     srcs = ["copy_to_directory.bzl"],
@@ -97,3 +104,13 @@ bzl_library(
     srcs = ["diff_test.bzl"],
     visibility = ["//lib:__subpackages__"],
 )
+
+bzl_library(
+    name = "run_binary",
+    srcs = ["run_binary.bzl"],
+    visibility = ["//lib:__subpackages__"],
+    deps = [
+        ":expand_make_vars",
+        "@bazel_skylib//lib:dicts",
+    ],
+)

lib/private/expand_make_vars.bzl

@@ -1,30 +1,9 @@
 "Helpers to expand make variables"
 
-# Convert an runfiles rootpath to a runfiles manifestpath.
-# Runfiles rootpath is returned from ctx.expand_location $(rootpath) and $(rootpaths):
-# - ./file
-# - path/to/file
-# - ../external_repo/path/to/file
-# This is converted to the runfiles manifest path of:
-# - repo/path/to/file
-def _rootpath_to_runfiles_manifest_path(ctx, path):
-    if path.startswith("../"):
-        return path[len("../"):]
-    if path.startswith("./"):
-        path = path[len("./"):]
-    return ctx.workspace_name + "/" + path
-
-# Expand $(rootpath) and $(rootpaths) to runfiles manifest path.
-# Runfiles manifest path is of the form:
-# - repo/path/to/file
-def _expand_rootpath_to_manifest_path(ctx, input, targets):
-    paths = ctx.expand_location(input, targets)
-    return " ".join([_rootpath_to_runfiles_manifest_path(ctx, p) for p in paths.split(" ")])
-
 def expand_locations(ctx, input, targets = []):
     """Expand location templates.
 
-    Expands all `$(execpath ...)`, `$(rootpath ...)` and legacy `$(location ...)` templates in the
+    Expands all `$(execpath ...)`, `$(rootpath ...)` and deprecated `$(location ...)` templates in the
     given string by replacing with the expanded path. Expansion only works for labels that point to direct dependencies
     of this rule or that are explicitly listed in the optional argument targets.
 
@@ -44,20 +23,7 @@ def expand_locations(ctx, input, targets = []):
     - `<bin_dir>/path/to/file`
     - `<bin_dir>/external/external_repo/path/to/file`
 
-    The legacy `$(location)` and `$(locations)` expansions are deprecated as they return the runfiles manifest path of the
+    The deprecated `$(location)` and `$(locations)` expansions returns either the execpath or rootpath depending on the context.
-    format `repo/path/to/file` which behave differently than the built-in `$(location)` expansion in args of *_binary
-    and *_test rules which returns the rootpath.
-    See https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-binaries.
-
-    The legacy `$(location)` and `$(locations)` expansion also differs from how the builtin `ctx.expand_location()` expansions
-    of `$(location)` and `$(locations)` behave as that function returns either the execpath or rootpath depending on the context.
-    See https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables.
-
-    The behavior of `$(location)` and `$(locations)` expansion will be fixed in a future major release to match the
-    to default Bazel behavior and return the same path as `ctx.expand_location()` returns for these.
-
-    The recommended approach is to now use `$(rootpath)` where you previously used $(location). See the docstrings
-    of `nodejs_binary` or `params_file` for examples of how to use `$(rootpath)` in `templated_args` and `args` respectively.
 
     Args:
       ctx: context
@@ -67,39 +33,8 @@ def expand_locations(ctx, input, targets = []):
     Returns:
       The expanded path or the original path
     """
-    target = "@%s//%s:%s" % (ctx.workspace_name, "/".join(ctx.build_file_path.split("/")[:-1]), ctx.attr.name)
 
-    # Loop through input an expand all predefined source/output path variables
+    return ctx.expand_location(input, targets = targets)
-    # See https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables.
-    path = ""
-    length = len(input)
-    last = 0
-    for i in range(length):
-        # Support legacy $(location) and $(locations) expansions which return the runfiles manifest path
-        # in the format `repo/path/to/file`. This expansion is DEPRECATED. See docstring above.
-        # TODO: Change location to behave the same as the built-in $(location) expansion for args of *_binary
-        #       and *_test rules. This would be a BREAKING CHANGE.
-        if input[i:].startswith("$(location ") or input[i:].startswith("$(locations "):
-            j = input.find(")", i) + 1
-            if (j == 0):
-                fail("invalid \"%s\" expansion in string \"%s\" part of target %s" % (input[i:j], input, target))
-            path += input[last:i]
-            path += _expand_rootpath_to_manifest_path(ctx, "$(rootpath" + input[i + 10:j], targets)
-            last = j
-            i = j
-
-        # Expand $(execpath) $(execpaths) $(rootpath) $(rootpaths) with plain ctx.expand_location()
-        if input[i:].startswith("$(execpath ") or input[i:].startswith("$(execpaths ") or input[i:].startswith("$(rootpath ") or input[i:].startswith("$(rootpaths "):
-            j = input.find(")", i) + 1
-            if (j == 0):
-                fail("invalid \"%s\" expansion in string \"%s\" part of target %s" % (input[i:j], input, target))
-            path += input[last:i]
-            path += ctx.expand_location(input[i:j], targets)
-            last = j
-            i = j
-    path += input[last:]
-
-    return path
 
 def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "args"):
     """Expand make variables and substitute like genrule does.

lib/private/run_binary.bzl

@@ -0,0 +1,101 @@
+# 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.
+
+"""run_binary implementation"""
+
+load("@bazel_skylib//lib:dicts.bzl", "dicts")
+load("//lib/private:expand_make_vars.bzl", "expand_locations", "expand_variables")
+
+def _impl(ctx):
+    if ctx.attr.output_dir and ctx.outputs.outs:
+        fail("Only one of output_dir and outs may be specified")
+    if not ctx.attr.output_dir and not ctx.outputs.outs:
+        fail("One of output_dir and outs must be specified")
+
+    tool_as_list = [ctx.attr.tool]
+    tool_inputs, tool_input_mfs = ctx.resolve_tools(tools = tool_as_list)
+    args = ctx.actions.args()
+
+    # `expand_locations(...).split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
+    # _expand_locations returns an array of args to support $(execpaths) expansions.
+    # TODO: If the string has intentional spaces or if one or more of the expanded file
+    # locations has a space in the name, we will incorrectly split it into multiple arguments
+    for a in ctx.attr.args:
+        args.add_all([expand_variables(ctx, e, outs = ctx.outputs.outs, output_dir = ctx.attr.output_dir) for e in expand_locations(ctx, a, ctx.attr.srcs).split(" ")])
+    envs = {}
+    for k, v in ctx.attr.env.items():
+        envs[k] = " ".join([expand_variables(ctx, e, outs = ctx.outputs.outs, output_dir = ctx.attr.output_dir, attribute_name = "env") for e in expand_locations(ctx, v, ctx.attr.srcs).split(" ")])
+    if ctx.attr.output_dir:
+        outputs = [ctx.actions.declare_directory(ctx.attr.name)]
+    else:
+        outputs = ctx.outputs.outs
+    ctx.actions.run(
+        outputs = outputs,
+        inputs = ctx.files.srcs,
+        tools = tool_inputs,
+        executable = ctx.executable.tool,
+        arguments = [args],
+        mnemonic = "RunBinary",
+        use_default_shell_env = False,
+        env = dicts.add(ctx.configuration.default_shell_env, envs),
+        input_manifests = tool_input_mfs,
+    )
+    return DefaultInfo(
+        files = depset(outputs),
+        runfiles = ctx.runfiles(files = outputs),
+    )
+
+run_binary = rule(
+    implementation = _impl,
+    doc = "Runs a binary as a build action.<br/><br/>This rule does not require Bash (unlike" +
+          " <code>native.genrule</code>).",
+    attrs = {
+        "tool": attr.label(
+            doc = "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" +
+                  " <code>$(location)</code> expansion in <code>args</code> and <code>env</code>.",
+            executable = True,
+            allow_files = True,
+            mandatory = True,
+            cfg = "host",
+        ),
+        "env": attr.string_dict(
+            doc = "Environment variables of the action.<br/><br/>Subject to " +
+                  " <code><a href=\"https://docs.bazel.build/versions/main/be/make-variables.html#location\">$(location)</a></code>" +
+                  " expansion.",
+        ),
+        "srcs": attr.label_list(
+            allow_files = True,
+            doc = "Additional inputs of the action.<br/><br/>These labels are available for" +
+                  " <code>$(location)</code> expansion in <code>args</code> and <code>env</code>.",
+        ),
+        "output_dir": attr.bool(
+            doc = "Set to True if you want the output to be a directory." +
+                  " Exactly one of `outs`, `output_dir` may be used." +
+                  " If you output a directory, there can only be one output, which will be a" +
+                  " directory named the same as the target.",
+        ),
+        "outs": attr.output_list(
+            doc = "Output files generated by the action.<br/><br/>These labels are available for" +
+                  " <code>$(location)</code> expansion in <code>args</code> and <code>env</code>.",
+        ),
+        "args": attr.string_list(
+            doc = "Command line arguments of the binary.<br/><br/>Subject to" +
+                  "<code><a href=\"https://docs.bazel.build/versions/main/be/make-variables.html#location\">$(location)</a></code>" +
+                  " expansion.",
+        ),
+    },
+)

lib/run_binary.bzl

@@ -0,0 +1,11 @@
+"""Runs a binary as a build action. This rule does not require Bash (unlike native.genrule()).
+
+This fork of bazel-skylib's run_binary adds directory output support and better makevar expansions.
+"""
+
+load(
+    "//lib/private:run_binary.bzl",
+    _run_binary = "run_binary",
+)
+
+run_binary = _run_binary

lib/tests/run_binary/BUILD.bazel

@@ -0,0 +1,58 @@
+"tests for run_binary"
+
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
+load("//lib:run_binary.bzl", "run_binary")
+load("//lib:diff_test.bzl", "diff_test")
+load("//lib:copy_to_directory.bzl", "copy_to_directory")
+
+write_file(
+    name = "make_dir_a_sh",
+    out = "make_dir_a.sh",
+    content = [
+        "#!/usr/bin/env bash",
+        "set -o errexit -o nounset -o pipefail",
+        "outdir=$1",
+        "echo 'foo' > \"$outdir/foo\"",
+        "echo 'bar' > \"$outdir/bar\"",
+    ],
+    is_executable = True,
+)
+
+sh_binary(
+    name = "make_dir_a",
+    srcs = [":make_dir_a.sh"],
+)
+
+# target-under-test
+run_binary(
+    name = "dir_a",
+    args = ["$(@D)"],
+    output_dir = True,
+    tool = ":make_dir_a",
+)
+
+genrule(
+    name = "make_foo",
+    outs = ["foo"],
+    cmd = "echo 'foo' > $@",
+)
+
+genrule(
+    name = "make_bar",
+    outs = ["bar"],
+    cmd = "echo 'bar' > $@",
+)
+
+copy_to_directory(
+    name = "dir_b",
+    srcs = [
+        ":make_bar",
+        ":make_foo",
+    ],
+)
+
+diff_test(
+    name = "dir_test",
+    file1 = ":dir_a",
+    file2 = ":dir_b",
+)