From 72d3d3c75cc4a78d80a09d18178b2cd6a9a9dd94 Mon Sep 17 00:00:00 2001 From: 3405691582 Date: Wed, 3 Aug 2022 18:40:25 -0400 Subject: [PATCH] Don't assume llvm-ar and clang are adjacent. (#1879) The toolchain embeds the assumption that `clang` and `llvm-ar` are adjacent, which may not be true on all host platforms. Moreover, in #1842 and #1843, we test if Homebrow LLVM is in `PATH` by checking that `llvm-ar` is adjacent to `clang` and if it isn't, we `fail()` the build, even if `llvm-ar` *is* in `PATH`. Instead, actually check `PATH` with `repository_ctx.which`. This however necessitates the assumption that `llvm-ar` and other LLVM binutils are adjacent, and subsequently that `clang` and `ld.lld` are adjacent. It appears that we don't seem to always be using these tools, but we should avoid embedding wrong assumptions regardless. Update docs to reflect this change. --- .../clang_cc_toolchain_config.bzl | 13 ++++++------ bazel/cc_toolchains/clang_configuration.bzl | 21 ++++++++++--------- .../clang_detected_variables.tpl.bzl | 1 + docs/project/contribution_tools.md | 2 +- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl index 2ed6595e2952..74aeb3ac63c5 100644 --- a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl +++ b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl @@ -21,6 +21,7 @@ load( ":clang_detected_variables.bzl", "clang_include_dirs_list", "clang_resource_dir", + "clang_bindir", "llvm_bindir", "sysroot_dir", ) @@ -68,9 +69,9 @@ all_link_actions = [ def _impl(ctx): tool_paths = [ tool_path(name = "ar", path = llvm_bindir + "/llvm-ar"), - tool_path(name = "ld", path = llvm_bindir + "/ld.lld"), - tool_path(name = "cpp", path = llvm_bindir + "/clang-cpp"), - tool_path(name = "gcc", path = llvm_bindir + "/clang++"), + tool_path(name = "ld", path = clang_bindir + "/ld.lld"), + tool_path(name = "cpp", path = clang_bindir + "/clang-cpp"), + tool_path(name = "gcc", path = clang_bindir + "/clang++"), tool_path(name = "dwp", path = llvm_bindir + "/llvm-dwp"), tool_path(name = "gcov", path = llvm_bindir + "/llvm-cov"), tool_path(name = "nm", path = llvm_bindir + "/llvm-nm"), @@ -80,13 +81,13 @@ def _impl(ctx): ] action_configs = [ - action_config(action_name = name, enabled = True, tools = [tool(path = llvm_bindir + "/clang")]) + action_config(action_name = name, enabled = True, tools = [tool(path = clang_bindir + "/clang")]) for name in all_c_compile_actions ] + [ - action_config(action_name = name, enabled = True, tools = [tool(path = llvm_bindir + "/clang++")]) + action_config(action_name = name, enabled = True, tools = [tool(path = clang_bindir + "/clang++")]) for name in all_cpp_compile_actions ] + [ - action_config(action_name = name, enabled = True, tools = [tool(path = llvm_bindir + "/clang++")]) + action_config(action_name = name, enabled = True, tools = [tool(path = clang_bindir + "/clang++")]) for name in all_link_actions ] + [ action_config(action_name = name, enabled = True, tools = [tool(path = llvm_bindir + "/llvm-ar")]) diff --git a/bazel/cc_toolchains/clang_configuration.bzl b/bazel/cc_toolchains/clang_configuration.bzl index ec9f826e2143..88be63aa91d7 100644 --- a/bazel/cc_toolchains/clang_configuration.bzl +++ b/bazel/cc_toolchains/clang_configuration.bzl @@ -129,21 +129,22 @@ def _configure_clang_toolchain_impl(repository_ctx): sysroot_dir, ) - # Ensure the detected clang path has some LLVM tools in it, in attempt to - # warn the user if they don't have these necessary LLVM tools in their PATH. - # This check isn't exhaustive, only a best attempt at warning the developer. - llvm_check_result = repository_ctx.execute([ - clang.dirname.get_child("llvm-ar"), - "--version" - ]) - if llvm_check_result.return_code != 0: - fail("`llvm-ar` not found beside clang: is LLVM in PATH?") + # We expect that the LLVM binutils live adjacent to llvm-ar. + # First look for llvm-ar adjacent to clang, so that if found, + # it is most likely to match the same version as clang. + # Otherwise, try PATH. + arpath = clang.dirname.get_child("llvm-ar") + if not arpath.exists: + arpath = repository_ctx.which("llvm-ar") + if not arpath: + fail("`llvm-ar` not found in PATH or adjacent to clang") repository_ctx.template( "clang_detected_variables.bzl", repository_ctx.attr._clang_detected_variables_template, substitutions = { - "{LLVM_BINDIR}": str(clang.dirname), + "{LLVM_BINDIR}": str(arpath.dirname), + "{CLANG_BINDIR}": str(clang.dirname), "{CLANG_RESOURCE_DIR}": resource_dir, "{CLANG_INCLUDE_DIRS_LIST}": str( [str(path) for path in include_dirs], diff --git a/bazel/cc_toolchains/clang_detected_variables.tpl.bzl b/bazel/cc_toolchains/clang_detected_variables.tpl.bzl index 8ce79c2bb169..f0308c7e4026 100644 --- a/bazel/cc_toolchains/clang_detected_variables.tpl.bzl +++ b/bazel/cc_toolchains/clang_detected_variables.tpl.bzl @@ -9,6 +9,7 @@ This file gets processed by a repository rule, substituting the """ llvm_bindir = "{LLVM_BINDIR}" +clang_bindir = "{CLANG_BINDIR}" clang_resource_dir = "{CLANG_RESOURCE_DIR}" clang_include_dirs_list = {CLANG_INCLUDE_DIRS_LIST} sysroot_dir = "{SYSROOT}" diff --git a/docs/project/contribution_tools.md b/docs/project/contribution_tools.md index 6aa21a25a026..c8e7a4a02ff9 100644 --- a/docs/project/contribution_tools.md +++ b/docs/project/contribution_tools.md @@ -153,7 +153,7 @@ export PATH="$(brew --prefix llvm)/bin:${PATH}" Carbon expects the `PATH` to include the installed tooling. If set, `CC` should also point at `clang`. Our build environment will detect the `clang` binary using `CC` then `PATH`, and will expect the rest of the LLVM toolchain to be -available in the same directory as `clang`. However, various scripts and tools +available in the same directory as `llvm-ar`. However, various scripts and tools assume that the LLVM toolchain will be in `PATH`, particularly for tools like `clang-format` and `clang-tidy`.