From 804f321f648d3812e2795c115bdcba2169c62caf Mon Sep 17 00:00:00 2001 From: Greg Magolan <4302968+tiponada4l@users.noreply.github.com> Date: Thu, 11 Aug 2022 19:30:45 -0700 Subject: [PATCH] fix: fix make var expansion in expand_template (#213) --- docs/expand_make_vars.md | 21 +++- lib/BUILD.bazel | 6 +- lib/expand_make_vars.bzl | 16 +-- lib/private/BUILD.bazel | 26 ++++- lib/private/expand_locations.bzl | 37 +++++++ lib/private/expand_template.bzl | 63 +++++++++++ ...and_make_vars.bzl => expand_variables.bzl} | 104 ++---------------- lib/private/params_file.bzl | 2 +- lib/private/run_binary.bzl | 3 +- lib/tests/BUILD.bazel | 4 +- lib/tests/expand_make_vars_test.bzl | 2 +- 11 files changed, 165 insertions(+), 119 deletions(-) create mode 100644 lib/private/expand_locations.bzl create mode 100644 lib/private/expand_template.bzl rename lib/private/{expand_make_vars.bzl => expand_variables.bzl} (51%) diff --git a/docs/expand_make_vars.md b/docs/expand_make_vars.md index b7849b6..cd595b3 100644 --- a/docs/expand_make_vars.md +++ b/docs/expand_make_vars.md @@ -15,10 +15,11 @@ Template expansion This performs a simple search over the template file for the keys in substitutions, and replaces them with the corresponding values. -Values may also use location templates as documented in [expand_locations](#expand_locations) -as well as [configuration variables] such as `$(BINDIR)`, `$(TARGET_CPU)`, and `$(COMPILATION_MODE)`. - -[configuration variables]: https://docs.bazel.build/versions/main/skylark/lib/ctx.html#var +Values may also use location templates as documented in +[expand_locations](https://github.com/aspect-build/bazel-lib/blob/main/docs/expand_make_vars.md#expand_locations) +as well as [configuration variables](https://docs.bazel.build/versions/main/skylark/lib/ctx.html#var) +such as `$(BINDIR)`, `$(TARGET_CPU)`, and `$(COMPILATION_MODE)` as documented in +[expand_variables](https://github.com/aspect-build/bazel-lib/blob/main/docs/expand_make_vars.md#expand_variables). **ATTRIBUTES** @@ -108,6 +109,16 @@ genrule-like substitutions of: - `$(RULEDIR)`: The output directory of the rule, that is, the directory corresponding to the name of the package containing the rule under the bin tree. + - $(BUILD_FILE_PATH)": ctx.build_file_path + + - $(VERSION_FILE)": ctx.version_file.path + + - $(INFO_FILE)": ctx.info_file.path + + - $(TARGET)": "@%s//%s:%s" % (ctx.label.workspace_name, ctx.label.package, ctx.label.name) + + - $(WORKSPACE)": ctx.workspace_name + See https://docs.bazel.build/versions/main/be/general.html#genrule.cmd and https://docs.bazel.build/versions/main/be/make-variables.html#predefined_genrule_variables for more information of how these special variables are expanded. @@ -122,7 +133,7 @@ for more information of how these special variables are expanded. | 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 | "args" | +| attribute_name | name of the attribute containing the expression. Used for error reporting. | "args" | **RETURNS** diff --git a/lib/BUILD.bazel b/lib/BUILD.bazel index 7079297..1f855c0 100644 --- a/lib/BUILD.bazel +++ b/lib/BUILD.bazel @@ -32,7 +32,11 @@ bzl_library( bzl_library( name = "expand_make_vars", srcs = ["expand_make_vars.bzl"], - deps = ["//lib/private:expand_make_vars"], + deps = [ + "//lib/private:expand_locations", + "//lib/private:expand_template", + "//lib/private:expand_variables", + ], ) bzl_library( diff --git a/lib/expand_make_vars.bzl b/lib/expand_make_vars.bzl index 0ad6248..f219bdc 100644 --- a/lib/expand_make_vars.bzl +++ b/lib/expand_make_vars.bzl @@ -1,17 +1,9 @@ "Public API for expanding variables" -load( - "//lib/private:expand_make_vars.bzl", - _expand_locations = "expand_locations", - _expand_template = "expand_template", - _expand_variables = "expand_variables", -) +load("//lib/private:expand_locations.bzl", _expand_locations = "expand_locations") +load("//lib/private:expand_variables.bzl", _expand_variables = "expand_variables") +load("//lib/private:expand_template.bzl", _expand_template = "expand_template") expand_locations = _expand_locations expand_variables = _expand_variables - -expand_template = rule( - doc = _expand_template.doc, - implementation = _expand_template.implementation, - attrs = _expand_template.attrs, -) +expand_template = _expand_template diff --git a/lib/private/BUILD.bazel b/lib/private/BUILD.bazel index 3aaf1ea..8905dfc 100644 --- a/lib/private/BUILD.bazel +++ b/lib/private/BUILD.bazel @@ -52,7 +52,7 @@ bzl_library( bzl_library( name = "params_file", srcs = ["params_file.bzl"], - deps = [":expand_make_vars"], + deps = [":expand_locations"], ) bzl_library( @@ -71,8 +71,25 @@ bzl_library( ) bzl_library( - name = "expand_make_vars", - srcs = ["expand_make_vars.bzl"], + name = "expand_locations", + srcs = ["expand_locations.bzl"], + deps = [ + "@bazel_skylib//lib:paths", + ], +) + +bzl_library( + name = "expand_template", + srcs = ["expand_template.bzl"], + deps = [ + ":expand_locations", + "@bazel_skylib//lib:dicts", + ], +) + +bzl_library( + name = "expand_variables", + srcs = ["expand_variables.bzl"], deps = [ "@bazel_skylib//lib:paths", ], @@ -129,7 +146,8 @@ bzl_library( name = "run_binary", srcs = ["run_binary.bzl"], deps = [ - ":expand_make_vars", + ":expand_locations", + ":expand_variables", "//lib:stamping", "@bazel_skylib//lib:dicts", ], diff --git a/lib/private/expand_locations.bzl b/lib/private/expand_locations.bzl new file mode 100644 index 0000000..8f2d668 --- /dev/null +++ b/lib/private/expand_locations.bzl @@ -0,0 +1,37 @@ +"Helpers to expand location" + +def expand_locations(ctx, input, targets = []): + """Expand location templates. + + 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. + + See https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables. + + Use `$(rootpath)` and `$(rootpaths)` to expand labels to the runfiles path that a built binary can use + to find its dependencies. This path is of the format: + - `./file` + - `path/to/file` + - `../external_repo/path/to/file` + + Use `$(execpath)` and `$(execpaths)` to expand labels to the execroot (where Bazel runs build actions). + This is of the format: + - `./file` + - `path/to/file` + - `external/external_repo/path/to/file` + - `/path/to/file` + - `/external/external_repo/path/to/file` + + The deprecated `$(location)` and `$(locations)` expansions returns either the execpath or rootpath depending on the context. + + Args: + ctx: context + input: String to be expanded + targets: List of targets for additional lookup information. + + Returns: + The expanded path or the original path + """ + + return ctx.expand_location(input, targets = targets) diff --git a/lib/private/expand_template.bzl b/lib/private/expand_template.bzl new file mode 100644 index 0000000..2377029 --- /dev/null +++ b/lib/private/expand_template.bzl @@ -0,0 +1,63 @@ +"expand_template rule" + +load(":expand_locations.bzl", _expand_locations = "expand_locations") +load(":expand_variables.bzl", _expand_variables = "expand_variables") + +def _expand_template_impl(ctx): + template = ctx.file.template + + substitutions = {} + for k, v in ctx.attr.substitutions.items(): + substitutions[k] = " ".join([_expand_variables(ctx, e, outs = [ctx.outputs.out], attribute_name = "substitutions") for e in _expand_locations(ctx, v, ctx.attr.data).split(" ")]) + + ctx.actions.expand_template( + template = template, + output = ctx.outputs.out, + substitutions = substitutions, + is_executable = ctx.attr.is_executable, + ) + +expand_template_lib = struct( + doc = """Template expansion + +This performs a simple search over the template file for the keys in substitutions, +and replaces them with the corresponding values. + +Values may also use location templates as documented in +[expand_locations](https://github.com/aspect-build/bazel-lib/blob/main/docs/expand_make_vars.md#expand_locations) +as well as [configuration variables](https://docs.bazel.build/versions/main/skylark/lib/ctx.html#var) +such as `$(BINDIR)`, `$(TARGET_CPU)`, and `$(COMPILATION_MODE)` as documented in +[expand_variables](https://github.com/aspect-build/bazel-lib/blob/main/docs/expand_make_vars.md#expand_variables). +""", + implementation = _expand_template_impl, + attrs = { + "template": attr.label( + doc = "The template file to expand.", + mandatory = True, + allow_single_file = True, + ), + "substitutions": attr.string_dict( + doc = "Mapping of strings to substitutions.", + mandatory = True, + ), + "out": attr.output( + doc = "Where to write the expanded file.", + mandatory = True, + ), + "is_executable": attr.bool( + doc = "Whether to mark the output file as executable.", + default = False, + mandatory = False, + ), + "data": attr.label_list( + doc = "List of targets for additional lookup information.", + allow_files = True, + ), + }, +) + +expand_template = rule( + doc = expand_template_lib.doc, + implementation = expand_template_lib.implementation, + attrs = expand_template_lib.attrs, +) diff --git a/lib/private/expand_make_vars.bzl b/lib/private/expand_variables.bzl similarity index 51% rename from lib/private/expand_make_vars.bzl rename to lib/private/expand_variables.bzl index 8204021..6aed332 100644 --- a/lib/private/expand_make_vars.bzl +++ b/lib/private/expand_variables.bzl @@ -2,42 +2,6 @@ load("@bazel_skylib//lib:paths.bzl", _spaths = "paths") -def expand_locations(ctx, input, targets = []): - """Expand location templates. - - 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. - - See https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables. - - Use `$(rootpath)` and `$(rootpaths)` to expand labels to the runfiles path that a built binary can use - to find its dependencies. This path is of the format: - - `./file` - - `path/to/file` - - `../external_repo/path/to/file` - - Use `$(execpath)` and `$(execpaths)` to expand labels to the execroot (where Bazel runs build actions). - This is of the format: - - `./file` - - `path/to/file` - - `external/external_repo/path/to/file` - - `/path/to/file` - - `/external/external_repo/path/to/file` - - The deprecated `$(location)` and `$(locations)` expansions returns either the execpath or rootpath depending on the context. - - Args: - ctx: context - input: String to be expanded - targets: List of targets for additional lookup information. - - Returns: - The expanded path or the original path - """ - - return ctx.expand_location(input, targets = targets) - def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "args"): """Expand make variables and substitute like genrule does. @@ -58,6 +22,16 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar - `$(RULEDIR)`: The output directory of the rule, that is, the directory corresponding to the name of the package containing the rule under the bin tree. + - $(BUILD_FILE_PATH)": ctx.build_file_path + + - $(VERSION_FILE)": ctx.version_file.path + + - $(INFO_FILE)": ctx.info_file.path + + - $(TARGET)": "@%s//%s:%s" % (ctx.label.workspace_name, ctx.label.package, ctx.label.name) + + - $(WORKSPACE)": ctx.workspace_name + See https://docs.bazel.build/versions/main/be/general.html#genrule.cmd and https://docs.bazel.build/versions/main/be/make-variables.html#predefined_genrule_variables for more information of how these special variables are expanded. @@ -69,7 +43,7 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar 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 + attribute_name: name of the attribute containing the expression. Used for error reporting. Returns: `s` with the variables expanded @@ -81,7 +55,7 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar ) additional_substitutions = {} - # TODO: remove output_dir in 2.x release + # 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.") @@ -117,57 +91,3 @@ def expand_variables(ctx, s, outs = [], output_dir = False, attribute_name = "ar additional_substitutions["WORKSPACE"] = ctx.workspace_name return ctx.expand_make_variables(attribute_name, s, additional_substitutions) - -def _expand_template_impl(ctx): - template = ctx.file.template - substitutions = ctx.attr.substitutions - - subs = dict({ - k: expand_locations(ctx, v, ctx.attr.data) - for k, v in substitutions.items() - }, **ctx.var) - - ctx.actions.expand_template( - template = template, - output = ctx.outputs.out, - substitutions = subs, - is_executable = ctx.attr.is_executable, - ) - -expand_template = struct( - doc = """Template expansion - -This performs a simple search over the template file for the keys in substitutions, -and replaces them with the corresponding values. - -Values may also use location templates as documented in [expand_locations](#expand_locations) -as well as [configuration variables] such as `$(BINDIR)`, `$(TARGET_CPU)`, and `$(COMPILATION_MODE)`. - -[configuration variables]: https://docs.bazel.build/versions/main/skylark/lib/ctx.html#var -""", - implementation = _expand_template_impl, - attrs = { - "template": attr.label( - doc = "The template file to expand.", - mandatory = True, - allow_single_file = True, - ), - "substitutions": attr.string_dict( - doc = "Mapping of strings to substitutions.", - mandatory = True, - ), - "out": attr.output( - doc = "Where to write the expanded file.", - mandatory = True, - ), - "is_executable": attr.bool( - doc = "Whether to mark the output file as executable.", - default = False, - mandatory = False, - ), - "data": attr.label_list( - doc = "List of targets for additional lookup information.", - allow_files = True, - ), - }, -) diff --git a/lib/private/params_file.bzl b/lib/private/params_file.bzl index ae8c395..11d02c8 100644 --- a/lib/private/params_file.bzl +++ b/lib/private/params_file.bzl @@ -1,6 +1,6 @@ "params_file rule" -load("//lib/private:expand_make_vars.bzl", "expand_locations") +load(":expand_locations.bzl", "expand_locations") _ATTRS = { "args": attr.string_list(), diff --git a/lib/private/run_binary.bzl b/lib/private/run_binary.bzl index da43cec..d390c3d 100644 --- a/lib/private/run_binary.bzl +++ b/lib/private/run_binary.bzl @@ -15,7 +15,8 @@ """run_binary implementation""" load("@bazel_skylib//lib:dicts.bzl", "dicts") -load("//lib/private:expand_make_vars.bzl", "expand_locations", "expand_variables") +load(":expand_locations.bzl", "expand_locations") +load(":expand_variables.bzl", "expand_variables") load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp") def _impl(ctx): diff --git a/lib/tests/BUILD.bazel b/lib/tests/BUILD.bazel index 5a638b3..b9ba38b 100644 --- a/lib/tests/BUILD.bazel +++ b/lib/tests/BUILD.bazel @@ -24,7 +24,7 @@ write_file( "#!/bin/bash", "set -o errexit", """[ "{thing}" == "stuff" ]""", - """[ "%path%" == "BINDIR/lib/tests/template.txt" ]""", + """[ "%path%" == "{BINDIR}/lib/tests/template.txt" ]""", ], ) @@ -36,7 +36,7 @@ expand_template( substitutions = { "{thing}": "stuff", "%path%": "$(execpath :gen_template)", - "BINDIR": "$(BINDIR)", + "{BINDIR}": "$(BINDIR)", }, template = "template.txt", ) diff --git a/lib/tests/expand_make_vars_test.bzl b/lib/tests/expand_make_vars_test.bzl index 8e692b4..580a327 100644 --- a/lib/tests/expand_make_vars_test.bzl +++ b/lib/tests/expand_make_vars_test.bzl @@ -3,7 +3,7 @@ See https://docs.bazel.build/versions/main/skylark/testing.html#for-testing-star """ load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest") -load("//lib/private:expand_make_vars.bzl", "expand_variables") +load("//lib:expand_make_vars.bzl", "expand_variables") def _variables_test_impl(ctx): env = unittest.begin(ctx)