From 231dad5c92be6a4d3cbee01269ea5f23504c9ea5 Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Fri, 6 Oct 2023 08:32:55 -0700 Subject: [PATCH] refactor: remove output_dir from run_binary and expand_variables (#588) --- docs/expand_make_vars.md | 3 +-- docs/run_binary.md | 3 +-- lib/private/expand_variables.bzl | 38 +++++++++----------------------- lib/private/run_binary.bzl | 9 +------- 4 files changed, 14 insertions(+), 39 deletions(-) diff --git a/docs/expand_make_vars.md b/docs/expand_make_vars.md index c7f8ea8..1d2b788 100644 --- a/docs/expand_make_vars.md +++ b/docs/expand_make_vars.md @@ -54,7 +54,7 @@ The expanded path or the original path ## expand_variables 
-expand_variables(ctx, s, outs, output_dir, attribute_name)
+expand_variables(ctx, s, outs, attribute_name)
Expand make variables and substitute like genrule does. @@ -104,7 +104,6 @@ for more information of how these special variables are expanded. | ctx | starlark rule context | none | | s | expression to expand | none | | outs | declared outputs of the rule, for expanding references to outputs | [] | -| output_dir | whether the rule is expected to output a directory (TreeArtifact) Deprecated. For backward compatability with @aspect_bazel_lib 1.x. Pass output tree artifacts to outs instead. | False | | attribute_name | name of the attribute containing the expression. Used for error reporting. | "args" | **RETURNS** diff --git a/docs/run_binary.md b/docs/run_binary.md index 7686db8..97e2585 100644 --- a/docs/run_binary.md +++ b/docs/run_binary.md @@ -11,7 +11,7 @@ This fork of bazel-skylib's run_binary adds directory output support and better 
 run_binary(name, tool, srcs, args, env, outs, out_dirs, mnemonic, progress_message,
-           execution_requirements, stamp, output_dir, kwargs)
+           execution_requirements, stamp, kwargs)
Runs a binary as a build action. @@ -35,7 +35,6 @@ This rule does not require Bash (unlike `native.genrule`). | progress_message | Progress message to show to the user during the build, for example, "Compiling foo.cc to create foo.o". The message may contain %{label}, %{input}, or %{output} patterns, which are substituted with label string, first input, or output's path, respectively. Prefer to use patterns instead of static strings, because the former are more efficient. | None | | execution_requirements | Information for scheduling the action.

For example,

 execution_requirements = {     "no-cache": "1", },


See https://docs.bazel.build/versions/main/be/common-definitions.html#common.tags for useful keys. | None | | stamp | Whether to include build status files as inputs to the tool. Possible values:

- stamp = 0 (default): Never include build status files as inputs to the tool. This gives good build result caching. Most tools don't use the status files, so including them in --stamp builds makes those builds have many needless cache misses. (Note: this default is different from most rules with an integer-typed stamp attribute.) - stamp = 1: Always include build status files as inputs to the tool, even in [--nostamp](https://docs.bazel.build/versions/main/user-manual.html#flag--stamp) builds. This setting should be avoided, since it is non-deterministic. It potentially causes remote cache misses for the target and any downstream actions that depend on the result. - stamp = -1: Inclusion of build status files as inputs is controlled by the [--[no]stamp](https://docs.bazel.build/versions/main/user-manual.html#flag--stamp) flag. Stamped targets are not rebuilt unless their dependencies change.

When stamping is enabled, an additional two environment variables will be set for the action: - BAZEL_STABLE_STATUS_FILE - BAZEL_VOLATILE_STATUS_FILE

These files can be read and parsed by the action, for example to pass some values to a linker. | 0 | -| output_dir | If set to True then an output directory named the same as the target name is added to out_dirs.

Deprecated. For backward compatability with @aspect_bazel_lib 1.x. Use out_dirs instead. | False | | kwargs | Additional arguments | none | diff --git a/lib/private/expand_variables.bzl b/lib/private/expand_variables.bzl index f581944..a1f75bf 100644 --- a/lib/private/expand_variables.bzl +++ b/lib/private/expand_variables.bzl @@ -2,7 +2,7 @@ load("@bazel_skylib//lib:paths.bzl", _spaths = "paths") -def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "args"): +def expand_variables(ctx, s, outs = [], attribute_name = "args"): """Expand make variables and substitute like genrule does. Bazel [pre-defined variables](https://bazel.build/reference/be/make-variables#predefined_variables) @@ -45,9 +45,6 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar ctx: starlark rule context s: expression to expand outs: declared outputs of the rule, for expanding references to outputs - output_dir: whether the rule is expected to output a directory (TreeArtifact) - Deprecated. For backward compatability with @aspect_bazel_lib 1.x. Pass - output tree artifacts to outs instead. attribute_name: name of the attribute containing the expression. Used for error reporting. Returns: @@ -60,30 +57,17 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar ) additional_substitutions = {} - # TODO(2.0): remove output_dir in 2.x release - if output_dir: - if s.find("$@") != -1 or s.find("$(@)") != -1: - fail("$@ substitution may only be used with output_dir=False.") - - # We'll write into a newly created directory named after the rule - output_dir = _spaths.join( - ctx.bin_dir.path, - ctx.label.workspace_root, - ctx.label.package, - ctx.label.name, - ) - else: - if s.find("$@") != -1 or s.find("$(@)") != -1: - if len(outs) > 1: - fail("$@ substitution may only be used with a single out.") - if len(outs) == 1: - additional_substitutions["@"] = outs[0].path - if outs[0].is_directory: - output_dir = outs[0].path - else: - output_dir = outs[0].dirname + if s.find("$@") != -1 or s.find("$(@)") != -1: + if len(outs) > 1: + fail("$@ substitution may only be used with a single out.") + if len(outs) == 1: + additional_substitutions["@"] = outs[0].path + if outs[0].is_directory: + output_dir = outs[0].path else: - output_dir = rule_dir + output_dir = outs[0].dirname + else: + output_dir = rule_dir additional_substitutions["@D"] = output_dir additional_substitutions["RULEDIR"] = rule_dir diff --git a/lib/private/run_binary.bzl b/lib/private/run_binary.bzl index 7ba4210..3016790 100644 --- a/lib/private/run_binary.bzl +++ b/lib/private/run_binary.bzl @@ -119,8 +119,6 @@ def run_binary( progress_message = None, execution_requirements = None, stamp = 0, - # TODO: remove output_dir in 2.x release - output_dir = False, **kwargs): """Runs a binary as a build action. @@ -185,11 +183,6 @@ def run_binary( See https://docs.bazel.build/versions/main/be/common-definitions.html#common.tags for useful keys. - output_dir: If set to True then an output directory named the same as the target name - is added to out_dirs. - - Deprecated. For backward compatability with @aspect_bazel_lib 1.x. Use out_dirs instead. - stamp: Whether to include build status files as inputs to the tool. Possible values: - `stamp = 0` (default): Never include build status files as inputs to the tool. @@ -221,7 +214,7 @@ def run_binary( args = args, env = env, outs = outs, - out_dirs = out_dirs + ([name] if output_dir else []), + out_dirs = out_dirs, mnemonic = mnemonic, progress_message = progress_message, execution_requirements = execution_requirements,