fix: fix make var expansion in expand_template (#213)

tiponada4l · Aug 12, 2022 · 804f321 · 804f321
1 parent b3235b8
commit 804f321
Show file tree

Hide file tree

Showing 11 changed files with 165 additions and 119 deletions.
diff --git a/docs/expand_make_vars.md b/docs/expand_make_vars.md
diff --git 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
@@ -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
@@ -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
@@ -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`
+    - `<bin_dir>/path/to/file`
+    - `<bin_dir>/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
@@ -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 → lib/private/expand_variables.bzl b/lib/private/expand_make_vars.bzl → 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`
-    - `<bin_dir>/path/to/file`
-    - `<bin_dir>/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
@@ -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
@@ -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
@@ -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",
 )