Pass toolchain and user env variables to make invocation (#777)

* Pass toolchain and user env variables to make invocation

* Rename configure --> make

* Add integration test coverage for make flag passing

This requires making the make_simple Makefile more realistic by

* using CXX and forwarding it to the wrapper;
* using CXXFLAGS instead of CXX_FLAGS and not overwriting its contents.

examples/make_simple/BUILD.bazel

@@ -3,9 +3,12 @@ load("@rules_foreign_cc//foreign_cc:defs.bzl", "make")
 
 make(
     name = "make_lib",
-    build_data = ["//make_simple/code:clang_wrapper.sh"],
+    build_data = ["//make_simple/code:cxx_wrapper.sh"],
+    copts = [
+        "-DREQUIRED_DEFINE",
+    ],
     env = {
-        "CLANG_WRAPPER": "$(execpath //make_simple/code:clang_wrapper.sh)",
+        "CXX_WRAPPER": "$(execpath //make_simple/code:cxx_wrapper.sh)",
     },
     lib_source = "//make_simple/code:srcs",
     out_static_libs = ["liba.a"],
@@ -16,6 +19,9 @@ cc_test(
     srcs = [
         "test_libb.cpp",
     ],
+    copts = [
+        "-std=c++11",
+    ],
     tags = ["windows"],
     deps = [
         ":make_lib",

examples/make_simple/code/BUILD.bazel

@@ -1,4 +1,4 @@
-exports_files(["clang_wrapper.sh"])
+exports_files(["cxx_wrapper.sh"])
 
 filegroup(
     name = "srcs",

examples/make_simple/code/Makefile

@@ -1,19 +1,18 @@
 BUILD_DIR=build-out
 
 UNAME:=$(shell uname)
-CXX_FLAGS :=
 
 ifneq (,$(findstring NT, $(UNAME)))
   # If Windows
-  CXX_FLAGS := /MD
+  CXXFLAGS := $(CXXFLAGS) /MD
 else
-  CXX_FLAGS := -fPIC
+  CXXFLAGS := $(CXXFLAGS) -fPIC
 endif
 
 default all $(BUILD_DIR)/lib/liba.a: liba.cpp liba.h
 	rm -rf $(BUILD_DIR)/lib
 	mkdir -p $(BUILD_DIR)/lib
-	$(CLANG_WRAPPER) $(CXX_FLAGS) -o $(BUILD_DIR)/lib/liba.o -c liba.cpp
+	$(CXX_WRAPPER) $(CXXFLAGS) -o $(BUILD_DIR)/lib/liba.o -c liba.cpp
 	ar rcs $(BUILD_DIR)/lib/liba.a $(BUILD_DIR)/lib/liba.o
 
 install: $(BUILD_DIR)/lib/liba.a

examples/make_simple/code/cxx_wrapper.sh

@@ -4,5 +4,5 @@ if [[ $(uname) == *"NT"* ]]; then
  # If Windows
   exec clang-cl "$@"
 else
-  exec clang "$@"
+  exec "$CXX" "$@"
 fi

examples/make_simple/code/liba.cpp

@@ -1,5 +1,19 @@
 #include "liba.h"
 
+#include <math.h>
+
+#ifndef REQUIRED_DEFINE
+// '-DREQUIRED_DEFINE' is set via the copts attribute of the make rule.
+#error "REQUIRED_DEFINE is not defined"
+#endif
+
 std::string hello_liba(void) {
 	return "Hello from LIBA!";
-}
+}
+
+double hello_math(double a) {
+	// On Unix, this call requires linking to libm.so. The Bazel toolchain adds
+	// the required `-lm` linkopt automatically and rules_foreign_cc forwards
+	// this option to make invocation via the CXXFLAGS variable.
+	return acos(a);
+}

examples/make_simple/code/liba.h

@@ -5,5 +5,6 @@
 #include <string>
 
 std::string hello_liba(void);
+double hello_math(double);
 
-#endif
+#endif

examples/make_simple/test_libb.cpp

@@ -10,5 +10,9 @@ int main(int argc, char* argv[])
   if (result != "Hello from LIBA!") {
     throw std::runtime_error("Wrong result: " + result);
   }
+  double math_result = hello_math(0.5);
+  if (math_result < 1.047 || math_result > 1.048) {
+    throw std::runtime_error("Wrong math_result: " + std::to_string(math_result));
+  }
   std::cout << "Everything's fine!";
 }

foreign_cc/make.bzl

@@ -53,6 +53,8 @@ def _create_make_script(configureParameters):
         for arg in ctx.attr.args
     ])
 
+    user_env = expand_locations(ctx, ctx.attr.env, data)
+
     make_commands = []
     prefix = "{} ".format(expand_locations(ctx, attrs.tool_prefix, data)) if attrs.tool_prefix else ""
     for target in ctx.attr.targets:
@@ -65,8 +67,13 @@ def _create_make_script(configureParameters):
         ))
 
     return create_make_script(
+        workspace_name = ctx.workspace_name,
+        tools = tools,
+        flags = flags,
         root = root,
+        deps = ctx.attr.deps,
         inputs = inputs,
+        env_vars = user_env,
         make_commands = make_commands,
     )
 

foreign_cc/private/configure_script.bzl

@@ -1,6 +1,5 @@
 # buildifier: disable=module-docstring
-load(":cc_toolchain_util.bzl", "absolutize_path_in_str")
+load(":make_env_vars.bzl", "get_make_env_vars")
-load(":framework.bzl", "get_foreign_cc_dep")
 load(":make_script.bzl", "pkgconfig_script")
 
 # buildifier: disable=function-docstring
@@ -72,7 +71,7 @@ def create_configure_script(
 
     script.append("##mkdirs## $$BUILD_TMPDIR$$/$$INSTALL_PREFIX$$")
     script.append("{env_vars} {prefix}\"{configure}\" --prefix=$$BUILD_TMPDIR$$/$$INSTALL_PREFIX$$ {user_options}".format(
-        env_vars = _get_env_vars(workspace_name, tools, flags, env_vars, deps, inputs),
+        env_vars = get_make_env_vars(workspace_name, tools, flags, env_vars, deps, inputs),
         prefix = configure_prefix,
         configure = configure_path,
         user_options = " ".join(user_options),
@@ -92,136 +91,3 @@ def _get_autogen_env_vars(autogen_env_vars):
         vars[key] = autogen_env_vars.get(key)
     vars["NOCONFIGURE"] = "1"
     return vars
-
-# buildifier: disable=function-docstring
-def _get_env_vars(
-        workspace_name,
-        tools,
-        flags,
-        user_vars,
-        deps,
-        inputs):
-    vars = _get_configure_variables(workspace_name, tools, flags, user_vars)
-    deps_flags = _define_deps_flags(deps, inputs)
-
-    if "LDFLAGS" in vars.keys():
-        vars["LDFLAGS"] = vars["LDFLAGS"] + deps_flags.libs
-    else:
-        vars["LDFLAGS"] = deps_flags.libs
-
-    # -I flags should be put into preprocessor flags, CPPFLAGS
-    # https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Preset-Output-Variables.html
-    vars["CPPFLAGS"] = deps_flags.flags
-
-    return " ".join(["{}=\"{}\""
-        .format(key, _join_flags_list(workspace_name, vars[key])) for key in vars])
-
-def _define_deps_flags(deps, inputs):
-    # It is very important to keep the order for the linker => put them into list
-    lib_dirs = []
-
-    # Here go libraries built with Bazel
-    gen_dirs_set = {}
-    for lib in inputs.libs:
-        dir_ = lib.dirname
-        if not gen_dirs_set.get(dir_):
-            gen_dirs_set[dir_] = 1
-            lib_dirs.append("-L$$EXT_BUILD_ROOT$$/" + dir_)
-
-    include_dirs_set = {}
-    for include_dir in inputs.include_dirs:
-        include_dirs_set[include_dir] = "-I$$EXT_BUILD_ROOT$$/" + include_dir
-    for header in inputs.headers:
-        include_dir = header.dirname
-        if not include_dirs_set.get(include_dir):
-            include_dirs_set[include_dir] = "-I$$EXT_BUILD_ROOT$$/" + include_dir
-    include_dirs = include_dirs_set.values()
-
-    # For the external libraries, we need to refer to the places where
-    # we copied the dependencies ($EXT_BUILD_DEPS/<lib_name>), because
-    # we also want configure to find that same files with pkg-config
-    # -config or other mechanics.
-    # Since we need the names of include and lib directories under
-    # the $EXT_BUILD_DEPS/<lib_name>, we ask the provider.
-    gen_dirs_set = {}
-    for dep in deps:
-        external_deps = get_foreign_cc_dep(dep)
-        if external_deps:
-            for artifact in external_deps.artifacts.to_list():
-                if not gen_dirs_set.get(artifact.gen_dir):
-                    gen_dirs_set[artifact.gen_dir] = 1
-
-                    dir_name = artifact.gen_dir.basename
-                    include_dirs.append("-I$$EXT_BUILD_DEPS$$/{}/{}".format(dir_name, artifact.include_dir_name))
-                    lib_dirs.append("-L$$EXT_BUILD_DEPS$$/{}/{}".format(dir_name, artifact.lib_dir_name))
-
-    return struct(
-        libs = lib_dirs,
-        flags = include_dirs,
-    )
-
-# See https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html
-_CONFIGURE_FLAGS = {
-    "ARFLAGS": "cxx_linker_static",
-    "ASFLAGS": "assemble",
-    "CFLAGS": "cc",
-    "CXXFLAGS": "cxx",
-    "LDFLAGS": "cxx_linker_executable",
-    # missing: cxx_linker_shared
-}
-
-_CONFIGURE_TOOLS = {
-    "AR": "cxx_linker_static",
-    "CC": "cc",
-    "CXX": "cxx",
-    # missing: cxx_linker_executable
-}
-
-def _get_configure_variables(workspace_name, tools, flags, user_env_vars):
-    vars = {}
-
-    for flag in _CONFIGURE_FLAGS:
-        flag_value = getattr(flags, _CONFIGURE_FLAGS[flag])
-        if flag_value:
-            vars[flag] = flag_value
-
-    # Merge flags lists
-    for user_var in user_env_vars:
-        toolchain_val = vars.get(user_var)
-        if toolchain_val:
-            vars[user_var] = toolchain_val + [user_env_vars[user_var]]
-
-    tools_dict = {}
-    for tool in _CONFIGURE_TOOLS:
-        tool_value = getattr(tools, _CONFIGURE_TOOLS[tool])
-        if tool_value:
-            # Force absolutize of tool paths, which may relative to the exec root (e.g. hermetic toolchains built from source)
-            tool_value_absolute = _absolutize(workspace_name, tool_value, True)
-
-            # If the tool path contains whitespaces (e.g. C:\Program Files\...),
-            # MSYS2 requires that the path is wrapped in double quotes
-            if " " in tool_value_absolute:
-                tool_value_absolute = "\\\"" + tool_value_absolute + "\\\""
-
-            tools_dict[tool] = [tool_value_absolute]
-
-    # Replace tools paths if user passed other values
-    for user_var in user_env_vars:
-        toolchain_val = tools_dict.get(user_var)
-        if toolchain_val:
-            tools_dict[user_var] = [user_env_vars[user_var]]
-
-    vars.update(tools_dict)
-
-    # Put all other environment variables, passed by the user
-    for user_var in user_env_vars:
-        if not vars.get(user_var):
-            vars[user_var] = [user_env_vars[user_var]]
-
-    return vars
-
-def _absolutize(workspace_name, text, force = False):
-    return absolutize_path_in_str(workspace_name, "$$EXT_BUILD_ROOT$$/", text, force)
-
-def _join_flags_list(workspace_name, flags):
-    return " ".join([_absolutize(workspace_name, flag) for flag in flags])

foreign_cc/private/make_env_vars.bzl

@@ -0,0 +1,137 @@
+"""Helper methods to assemble make env variables from Bazel information."""
+
+load(":cc_toolchain_util.bzl", "absolutize_path_in_str")
+load(":framework.bzl", "get_foreign_cc_dep")
+
+# buildifier: disable=function-docstring
+def get_make_env_vars(
+        workspace_name,
+        tools,
+        flags,
+        user_vars,
+        deps,
+        inputs):
+    vars = _get_make_variables(workspace_name, tools, flags, user_vars)
+    deps_flags = _define_deps_flags(deps, inputs)
+
+    if "LDFLAGS" in vars.keys():
+        vars["LDFLAGS"] = vars["LDFLAGS"] + deps_flags.libs
+    else:
+        vars["LDFLAGS"] = deps_flags.libs
+
+    # -I flags should be put into preprocessor flags, CPPFLAGS
+    # https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Preset-Output-Variables.html
+    vars["CPPFLAGS"] = deps_flags.flags
+
+    return " ".join(["{}=\"{}\""
+        .format(key, _join_flags_list(workspace_name, vars[key])) for key in vars])
+
+def _define_deps_flags(deps, inputs):
+    # It is very important to keep the order for the linker => put them into list
+    lib_dirs = []
+
+    # Here go libraries built with Bazel
+    gen_dirs_set = {}
+    for lib in inputs.libs:
+        dir_ = lib.dirname
+        if not gen_dirs_set.get(dir_):
+            gen_dirs_set[dir_] = 1
+            lib_dirs.append("-L$$EXT_BUILD_ROOT$$/" + dir_)
+
+    include_dirs_set = {}
+    for include_dir in inputs.include_dirs:
+        include_dirs_set[include_dir] = "-I$$EXT_BUILD_ROOT$$/" + include_dir
+    for header in inputs.headers:
+        include_dir = header.dirname
+        if not include_dirs_set.get(include_dir):
+            include_dirs_set[include_dir] = "-I$$EXT_BUILD_ROOT$$/" + include_dir
+    include_dirs = include_dirs_set.values()
+
+    # For the external libraries, we need to refer to the places where
+    # we copied the dependencies ($EXT_BUILD_DEPS/<lib_name>), because
+    # we also want configure to find that same files with pkg-config
+    # -config or other mechanics.
+    # Since we need the names of include and lib directories under
+    # the $EXT_BUILD_DEPS/<lib_name>, we ask the provider.
+    gen_dirs_set = {}
+    for dep in deps:
+        external_deps = get_foreign_cc_dep(dep)
+        if external_deps:
+            for artifact in external_deps.artifacts.to_list():
+                if not gen_dirs_set.get(artifact.gen_dir):
+                    gen_dirs_set[artifact.gen_dir] = 1
+
+                    dir_name = artifact.gen_dir.basename
+                    include_dirs.append("-I$$EXT_BUILD_DEPS$$/{}/{}".format(dir_name, artifact.include_dir_name))
+                    lib_dirs.append("-L$$EXT_BUILD_DEPS$$/{}/{}".format(dir_name, artifact.lib_dir_name))
+
+    return struct(
+        libs = lib_dirs,
+        flags = include_dirs,
+    )
+
+# See https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html
+_MAKE_FLAGS = {
+    "ARFLAGS": "cxx_linker_static",
+    "ASFLAGS": "assemble",
+    "CFLAGS": "cc",
+    "CXXFLAGS": "cxx",
+    "LDFLAGS": "cxx_linker_executable",
+    # missing: cxx_linker_shared
+}
+
+_MAKE_TOOLS = {
+    "AR": "cxx_linker_static",
+    "CC": "cc",
+    "CXX": "cxx",
+    # missing: cxx_linker_executable
+}
+
+def _get_make_variables(workspace_name, tools, flags, user_env_vars):
+    vars = {}
+
+    for flag in _MAKE_FLAGS:
+        flag_value = getattr(flags, _MAKE_FLAGS[flag])
+        if flag_value:
+            vars[flag] = flag_value
+
+    # Merge flags lists
+    for user_var in user_env_vars:
+        toolchain_val = vars.get(user_var)
+        if toolchain_val:
+            vars[user_var] = toolchain_val + [user_env_vars[user_var]]
+
+    tools_dict = {}
+    for tool in _MAKE_TOOLS:
+        tool_value = getattr(tools, _MAKE_TOOLS[tool])
+        if tool_value:
+            # Force absolutize of tool paths, which may relative to the exec root (e.g. hermetic toolchains built from source)
+            tool_value_absolute = _absolutize(workspace_name, tool_value, True)
+
+            # If the tool path contains whitespaces (e.g. C:\Program Files\...),
+            # MSYS2 requires that the path is wrapped in double quotes
+            if " " in tool_value_absolute:
+                tool_value_absolute = "\\\"" + tool_value_absolute + "\\\""
+
+            tools_dict[tool] = [tool_value_absolute]
+
+    # Replace tools paths if user passed other values
+    for user_var in user_env_vars:
+        toolchain_val = tools_dict.get(user_var)
+        if toolchain_val:
+            tools_dict[user_var] = [user_env_vars[user_var]]
+
+    vars.update(tools_dict)
+
+    # Put all other environment variables, passed by the user
+    for user_var in user_env_vars:
+        if not vars.get(user_var):
+            vars[user_var] = [user_env_vars[user_var]]
+
+    return vars
+
+def _absolutize(workspace_name, text, force = False):
+    return absolutize_path_in_str(workspace_name, "$$EXT_BUILD_ROOT$$/", text, force)
+
+def _join_flags_list(workspace_name, flags):
+    return " ".join([_absolutize(workspace_name, flag) for flag in flags])

foreign_cc/private/make_script.bzl

@@ -1,19 +1,17 @@
 """A module for creating the build script for `make` builds"""
 
+load(":make_env_vars.bzl", "get_make_env_vars")
+
+# buildifier: disable=function-docstring
 def create_make_script(
+        workspace_name,
+        tools,
+        flags,
         root,
+        env_vars,
+        deps,
         inputs,
         make_commands):
-    """Constructs Make script to be passed to cc_external_rule_impl.
-
-    Args:
-        root (str): sources root relative to the $EXT_BUILD_ROOT
-        inputs (struct): An InputFiles provider
-        make_commands (list): Lines of bash which invoke make
-
-    Returns:
-        list: Lines of bash which make up the build script
-    """
     ext_build_dirs = inputs.ext_build_dirs
 
     script = pkgconfig_script(ext_build_dirs)
@@ -21,7 +19,11 @@ def create_make_script(
     script.append("##symlink_contents_to_dir## $$EXT_BUILD_ROOT$$/{} $$BUILD_TMPDIR$$".format(root))
 
     script.append("##enable_tracing##")
-    script.extend(make_commands)
+    configure_vars = get_make_env_vars(workspace_name, tools, flags, env_vars, deps, inputs)
+    script.extend(["{env_vars} {command}".format(
+        env_vars = configure_vars,
+        command = command,
+    ) for command in make_commands])
     script.append("##disable_tracing##")
     return script