Replace `escape_locations` with `escape_locations_and_make_variables` everywhere (#861)

Co-authored-by: James Sharpe <james.sharpe@zenotech.com>

examples/make_simple/BUILD.bazel

@@ -3,14 +3,19 @@ load("@rules_foreign_cc//foreign_cc:defs.bzl", "make")
 
 make(
     name = "make_lib",
-    build_data = ["//make_simple/code:cxx_wrapper.sh"],
+    build_data = [
+        "//make_simple/code:cxx_wrapper.sh",
+        "README.md",
+    ],
     copts = [
         "-DREQUIRED_DEFINE",
     ],
     env = {
         "CXX_WRAPPER": "$(execpath //make_simple/code:cxx_wrapper.sh)",
+        "README_DIR": "$$(dirname $(execpath README.md))",
     },
     lib_source = "//make_simple/code:srcs",
+    out_data_dirs = ["share"],
     out_static_libs = ["liba.a"],
 )
 

examples/make_simple/code/Makefile

@@ -16,6 +16,7 @@ default all $(BUILD_DIR)/lib/liba.a: liba.cpp liba.h
 	ar rcs $(BUILD_DIR)/lib/liba.a $(BUILD_DIR)/lib/liba.o
 
 install: $(BUILD_DIR)/lib/liba.a
-	mkdir -p $(PREFIX)/lib $(PREFIX)/include
+	mkdir -p $(PREFIX)/lib $(PREFIX)/include $(PREFIX)/share
 	cp -rpv $(BUILD_DIR)/lib $(PREFIX)
 	cp -p liba.h $(PREFIX)/include
+	cp $(README_DIR)/README.md $(PREFIX)/share/

examples/third_party/openssl/BUILD.openssl.bazel

@@ -61,7 +61,7 @@ configure_make_variant(
         # as NASM is unsed to build OpenSSL rather than MASM
         "ASFLAGS=\" \"",
     ],
-    configure_prefix = "$PERL",
+    configure_prefix = "$$PERL",
     env = {
         # The Zi flag must be set otherwise OpenSSL fails to build due to missing .pdb files
         "CFLAGS": "-Zi",

foreign_cc/cmake.bzl

@@ -142,7 +142,6 @@ load(
     "CC_EXTERNAL_RULE_FRAGMENTS",
     "cc_external_rule_impl",
     "create_attrs",
-    "expand_locations",
     "expand_locations_and_make_variables",
 )
 load("//foreign_cc/private:transitions.bzl", "make_variant")
@@ -216,7 +215,7 @@ def _create_configure_script(configureParameters):
 
     # Generate a list of arguments for cmake's build command
     build_args = " ".join([
-        expand_locations(ctx, arg, data)
+        expand_locations_and_make_variables(ctx, arg, "build_args", data)
         for arg in ctx.attr.build_args
     ])
 
@@ -240,7 +239,7 @@ def _create_configure_script(configureParameters):
     if ctx.attr.install:
         # Generate a list of arguments for cmake's install command
         install_args = " ".join([
-            expand_locations(ctx, arg, data)
+            expand_locations_and_make_variables(ctx, arg, "install_args", data)
             for arg in ctx.attr.install_args
         ])
 
@@ -251,7 +250,7 @@ def _create_configure_script(configureParameters):
             config = configuration,
         ))
 
-    prefix = expand_locations(ctx, {"prefix": attrs.tool_prefix}, data)["prefix"] if attrs.tool_prefix else ""
+    prefix = expand_locations_and_make_variables(ctx, attrs.tool_prefix, "tool_prefix", data) if attrs.tool_prefix else ""
 
     configure_script = create_cmake_script(
         workspace_name = ctx.workspace_name,
@@ -263,7 +262,7 @@ def _create_configure_script(configureParameters):
         root = root,
         no_toolchain_file = no_toolchain_file,
         user_cache = dict(ctx.attr.cache_entries),
-        user_env = expand_locations_and_make_variables(ctx, "env", data),
+        user_env = expand_locations_and_make_variables(ctx, ctx.attr.env, "env", data),
         options = attrs.generate_args,
         cmake_commands = cmake_commands,
         cmake_prefix = prefix,

foreign_cc/configure.bzl

@@ -16,7 +16,6 @@ load(
     "CC_EXTERNAL_RULE_FRAGMENTS",
     "cc_external_rule_impl",
     "create_attrs",
-    "expand_locations",
     "expand_locations_and_make_variables",
 )
 load("//foreign_cc/private:transitions.bzl", "make_variant")
@@ -75,11 +74,11 @@ def _create_configure_script(configureParameters):
         for arg in ctx.attr.args
     ])
 
-    user_env = expand_locations_and_make_variables(ctx, "env", data)
+    user_env = expand_locations_and_make_variables(ctx, ctx.attr.env, "env", data)
 
     make_commands = []
-    prefix = "{} ".format(expand_locations(ctx, attrs.tool_prefix, data)) if attrs.tool_prefix else ""
+    prefix = "{} ".format(expand_locations_and_make_variables(ctx, attrs.tool_prefix, "tool_prefix", data)) if attrs.tool_prefix else ""
-    configure_prefix = "{} ".format(expand_locations(ctx, ctx.attr.configure_prefix, data)) if ctx.attr.configure_prefix else ""
+    configure_prefix = "{} ".format(expand_locations_and_make_variables(ctx, ctx.attr.configure_prefix, "configure_prefix", data)) if ctx.attr.configure_prefix else ""
 
     for target in ctx.attr.targets:
         # Configure will have generated sources into `$BUILD_TMPDIR` so make sure we `cd` there

foreign_cc/make.bzl

@@ -15,7 +15,7 @@ load(
     "CC_EXTERNAL_RULE_FRAGMENTS",
     "cc_external_rule_impl",
     "create_attrs",
-    "expand_locations",
+    "expand_locations_and_make_variables",
 )
 load("//foreign_cc/private:make_script.bzl", "create_make_script")
 load("//foreign_cc/private:transitions.bzl", _make_variant = "make_variant")
@@ -53,10 +53,10 @@ def _create_make_script(configureParameters):
         for arg in ctx.attr.args
     ])
 
-    user_env = expand_locations(ctx, ctx.attr.env, data)
+    user_env = expand_locations_and_make_variables(ctx, ctx.attr.env, "env", data)
 
     make_commands = []
-    prefix = "{} ".format(expand_locations(ctx, attrs.tool_prefix, data)) if attrs.tool_prefix else ""
+    prefix = "{} ".format(expand_locations_and_make_variables(ctx, attrs.tool_prefix, "tool_prefix", data)) if attrs.tool_prefix else ""
     for target in ctx.attr.targets:
         make_commands.append("{prefix}{make} -C $$BUILD_TMPDIR$$ {target} {args} PREFIX={install_prefix}".format(
             prefix = prefix,

foreign_cc/ninja.bzl

@@ -10,7 +10,7 @@ load(
     "CC_EXTERNAL_RULE_FRAGMENTS",
     "cc_external_rule_impl",
     "create_attrs",
-    "expand_locations",
+    "expand_locations_and_make_variables",
 )
 load("//toolchains/native_tools:tool_access.bzl", "get_ninja_data")
 
@@ -66,7 +66,7 @@ def _create_ninja_script(configureParameters):
     if ctx.attr.directory:
         directory = ctx.expand_location(ctx.attr.directory, data)
 
-    prefix = "{} ".format(expand_locations(ctx, attrs.tool_prefix, data)) if attrs.tool_prefix else ""
+    prefix = "{} ".format(expand_locations_and_make_variables(ctx, attrs.tool_prefix, "tool_prefix", data)) if attrs.tool_prefix else ""
 
     # Generate commands for all the targets, ensuring there's
     # always at least 1 call to the default target.

foreign_cc/private/framework.bzl

@@ -315,7 +315,7 @@ def get_env_prelude(ctx, lib_name, data_dependencies, target_root):
         env.update({"PATH": _normalize_path(linker_path) + ":" + env.get("PATH")})
 
     # Add all user defined variables
-    user_vars = expand_locations_and_make_variables(ctx, "env", data_dependencies)
+    user_vars = expand_locations_and_make_variables(ctx, ctx.attr.env, "env", data_dependencies)
     env.update(user_vars)
 
     # If user has defined a PATH variable (e.g. PATH, LD_LIBRARY_PATH, CPATH) prepend it to the existing variable
@@ -927,16 +927,38 @@ def _expand_command_path(binary, path, command):
     else:
         return command
 
-def expand_locations_and_make_variables(ctx, attr_name, data):
+def expand_locations_and_make_variables(ctx, unexpanded, attr_name, data):
-    unexpanded = getattr(ctx.attr, attr_name)
+    """Expand locations and make variables while ensuring that `execpath` is always set to an absolute path
-    location_expanded = expand_locations(ctx, unexpanded, data)
 
+    This function is not expected to be passed to any action.env argument but instead rendered into
+    build scripts.
+
+    Args:
+        ctx (ctx): The rule's context object
+        unexpanded (dict, list, str): Variables to expand, can be a variety of different types
+        attr_name: The attribute from which `unexpanded` has been obtained (only used when reporting errors)
+        data (list): A list of targets to use for locations expansion
+
+    Returns:
+        (dict, list, str): expandable with locations and make variables expanded (does not apply to the keys of a dict)
+    """
+    location_expanded = _expand_locations(ctx, unexpanded, data)
+
+    if type(location_expanded) == type(dict()):
+        return {key: _expand_make_variables_in_string(ctx, value, attr_name) for key, value in location_expanded.items()}
+    elif type(location_expanded) == type(list()):
+        return [_expand_make_variables_in_string(ctx, value, attr_name) for value in location_expanded]
+    elif type(location_expanded) == type(""):
+        return _expand_make_variables_in_string(ctx, location_expanded, attr_name)
+    else:
+        fail("Unsupported type: {}".format(type(location_expanded)))
+
+def _expand_make_variables_in_string(ctx, expandable, attr_name):
     # Make variable expansion will treat $$ as escaped values for $ and strip the second one.
     # Double-escape $s which we insert in expand_locations.
-    make_variable_expanded = {k: ctx.expand_make_variables(attr_name, v.replace("$$EXT_BUILD_ROOT$$", "$$$$EXT_BUILD_ROOT$$$$"), {}) for k, v in location_expanded.items()}
+    return ctx.expand_make_variables(attr_name, expandable.replace("$$EXT_BUILD_ROOT$$", "$$$$EXT_BUILD_ROOT$$$$"), {})
-    return make_variable_expanded
 
-def expand_locations(ctx, expandable, data):
+def _expand_locations(ctx, expandable, data):
     """Expand locations on a dictionary while ensuring `execpath` is always set to an absolute path
 
     This function is not expected to be passed to any action.env argument but instead rendered into
@@ -948,31 +970,20 @@ def expand_locations(ctx, expandable, data):
         data (list): A list of targets
 
     Returns:
-        dict: An expanded dict of environment variables
+        (dict, list, str): expandable with locations expanded (does not apply to the keys of a dict)
     """
     if type(expandable) == type(dict()):
-        expanded_env = dict()
+        return {key: _expand_locations_in_string(ctx, value, data) for key, value in expandable.items()}
-        for key, value in expandable.items():
-            # If `EXT_BUILD_ROOT` exists in the string, we assume the user has added it themselves
-            if "EXT_BUILD_ROOT" in value:
-                expanded_env.update({key: ctx.expand_location(value, data)})
-            else:
-                expanded_env.update({key: ctx.expand_location(value.replace("$(execpath ", "$$EXT_BUILD_ROOT$$/$(execpath "), data)})
-        return expanded_env
     elif type(expandable) == type(list()):
-        expanded_vars = list()
+        return [_expand_locations_in_string(ctx, value, data) for value in expandable]
-        for value in expandable:
-            # If `EXT_BUILD_ROOT` exists in the string, we assume the user has added it themselves
-            if "EXT_BUILD_ROOT" in value:
-                expanded_vars.append(ctx.expand_location(value, data))
-            else:
-                expanded_vars.append(ctx.expand_location(value.replace("$(execpath ", "$$EXT_BUILD_ROOT$$/$(execpath "), data))
-        return expanded_vars
     elif type(expandable) == type(""):
-        # If `EXT_BUILD_ROOT` exists in the string, we assume the user has added it themselves
+        return _expand_locations_in_string(ctx, expandable, data)
-        if "EXT_BUILD_ROOT" in expandable:
-            return ctx.expand_location(expandable, data)
-        else:
-            return ctx.expand_location(expandable.replace("$(execpath ", "$$EXT_BUILD_ROOT$$/$(execpath "), data)
     else:
         fail("Unsupported type: {}".format(type(expandable)))
+
+def _expand_locations_in_string(ctx, expandable, data):
+    # If `EXT_BUILD_ROOT` exists in the string, we assume the user has added it themselves
+    if "EXT_BUILD_ROOT" in expandable:
+        return ctx.expand_location(expandable, data)
+    else:
+        return ctx.expand_location(expandable.replace("$(execpath ", "$$EXT_BUILD_ROOT$$/$(execpath "), data)