From f61ce5d10b221c643ae48063c3484fadf84066d4 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 27 Nov 2021 16:52:54 +0100 Subject: [PATCH] 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 | 10 +- examples/make_simple/code/BUILD.bazel | 2 +- examples/make_simple/code/Makefile | 7 +- .../code/{clang_wrapper.sh => cxx_wrapper.sh} | 2 +- examples/make_simple/code/liba.cpp | 16 +- examples/make_simple/code/liba.h | 3 +- examples/make_simple/test_libb.cpp | 4 + foreign_cc/make.bzl | 7 + foreign_cc/private/configure_script.bzl | 138 +----------------- foreign_cc/private/make_env_vars.bzl | 137 +++++++++++++++++ foreign_cc/private/make_script.bzl | 24 +-- 11 files changed, 193 insertions(+), 157 deletions(-) rename examples/make_simple/code/{clang_wrapper.sh => cxx_wrapper.sh} (83%) create mode 100644 foreign_cc/private/make_env_vars.bzl diff --git a/examples/make_simple/BUILD.bazel b/examples/make_simple/BUILD.bazel index 2ea15f70..c02c9e61 100644 --- a/examples/make_simple/BUILD.bazel +++ b/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", diff --git a/examples/make_simple/code/BUILD.bazel b/examples/make_simple/code/BUILD.bazel index 8a38ba14..c89664d0 100644 --- a/examples/make_simple/code/BUILD.bazel +++ b/examples/make_simple/code/BUILD.bazel @@ -1,4 +1,4 @@ -exports_files(["clang_wrapper.sh"]) +exports_files(["cxx_wrapper.sh"]) filegroup( name = "srcs", diff --git a/examples/make_simple/code/Makefile b/examples/make_simple/code/Makefile index c81804c1..1ad9230e 100644 --- a/examples/make_simple/code/Makefile +++ b/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 diff --git a/examples/make_simple/code/clang_wrapper.sh b/examples/make_simple/code/cxx_wrapper.sh similarity index 83% rename from examples/make_simple/code/clang_wrapper.sh rename to examples/make_simple/code/cxx_wrapper.sh index d98fdff6..dc63ca51 100755 --- a/examples/make_simple/code/clang_wrapper.sh +++ b/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 diff --git a/examples/make_simple/code/liba.cpp b/examples/make_simple/code/liba.cpp index 71d7fef3..ab1b3d28 100644 --- a/examples/make_simple/code/liba.cpp +++ b/examples/make_simple/code/liba.cpp @@ -1,5 +1,19 @@ #include "liba.h" +#include + +#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!"; -} \ No newline at end of file +} + +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); +} diff --git a/examples/make_simple/code/liba.h b/examples/make_simple/code/liba.h index 110c79b7..a537dccc 100644 --- a/examples/make_simple/code/liba.h +++ b/examples/make_simple/code/liba.h @@ -5,5 +5,6 @@ #include std::string hello_liba(void); +double hello_math(double); -#endif \ No newline at end of file +#endif diff --git a/examples/make_simple/test_libb.cpp b/examples/make_simple/test_libb.cpp index dd4257ff..c4e60c5a 100644 --- a/examples/make_simple/test_libb.cpp +++ b/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!"; } diff --git a/foreign_cc/make.bzl b/foreign_cc/make.bzl index 19b9aa4b..305130d2 100644 --- a/foreign_cc/make.bzl +++ b/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, ) diff --git a/foreign_cc/private/configure_script.bzl b/foreign_cc/private/configure_script.bzl index eee7b64d..29556584 100644 --- a/foreign_cc/private/configure_script.bzl +++ b/foreign_cc/private/configure_script.bzl @@ -1,6 +1,5 @@ # buildifier: disable=module-docstring -load(":cc_toolchain_util.bzl", "absolutize_path_in_str") -load(":framework.bzl", "get_foreign_cc_dep") +load(":make_env_vars.bzl", "get_make_env_vars") 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/), 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/, 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]) diff --git a/foreign_cc/private/make_env_vars.bzl b/foreign_cc/private/make_env_vars.bzl new file mode 100644 index 00000000..c6185840 --- /dev/null +++ b/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/), 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/, 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]) diff --git a/foreign_cc/private/make_script.bzl b/foreign_cc/private/make_script.bzl index acddbfe4..cef9af7c 100644 --- a/foreign_cc/private/make_script.bzl +++ b/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