Address more comments

toolchain/BUILD.toolchain.tpl

@@ -16,7 +16,7 @@ package(default_visibility = ["//visibility:public"])
 
 load("@bazel_skylib//rules:native_binary.bzl", "native_binary")
 load("@rules_cc//cc:defs.bzl", "cc_toolchain", "cc_toolchain_suite")
-load("//toolchain/internal:system_module_map.bzl", "system_module_map")
+load("@toolchains_llvm//toolchain/internal:system_module_map.bzl", "system_module_map")
 load("%{cc_toolchain_config_bzl}", "cc_toolchain_config")
 
 # Following filegroup targets are used when not using absolute paths and shared

toolchain/cc_toolchain_config.bzl

@@ -36,24 +36,23 @@ def cc_toolchain_config(
         host_os,
         target_arch,
         target_os,
+        target_system_name,
         toolchain_path_prefix,
         tools_path_prefix,
         wrapper_bin_prefix,
         compiler_configuration,
-        llvm_version,
+        cxx_builtin_include_directories,
         host_tools_info = {}):
     host_os_arch_key = _os_arch_pair(host_os, host_arch)
     target_os_arch_key = _os_arch_pair(target_os, target_arch)
     _check_os_arch_keys([host_os_arch_key, target_os_arch_key])
-    major_llvm_version = int(llvm_version.split(".")[0])
 
     # A bunch of variables that get passed straight through to
     # `create_cc_toolchain_config_info`.
     # TODO: What do these values mean, and are they actually all correct?
     host_system_name = host_arch
     (
         toolchain_identifier,
-        target_system_name,
         target_cpu,
         target_libc,
         compiler,
@@ -62,7 +61,6 @@ def cc_toolchain_config(
     ) = {
         "darwin-x86_64": (
             "clang-x86_64-darwin",
-            "x86_64-apple-macosx",
             "darwin",
             "macosx",
             "clang",
@@ -71,7 +69,6 @@ def cc_toolchain_config(
         ),
         "darwin-aarch64": (
             "clang-aarch64-darwin",
-            "aarch64-apple-macosx",
             "darwin",
             "macosx",
             "clang",
@@ -80,7 +77,6 @@ def cc_toolchain_config(
         ),
         "linux-aarch64": (
             "clang-aarch64-linux",
-            "aarch64-unknown-linux-gnu",
             "aarch64",
             "glibc_unknown",
             "clang",
@@ -89,7 +85,6 @@ def cc_toolchain_config(
         ),
         "linux-x86_64": (
             "clang-x86_64-linux",
-            "x86_64-unknown-linux-gnu",
             "k8",
             "glibc_unknown",
             "clang",
@@ -175,6 +170,7 @@ def cc_toolchain_config(
     # always link C++ libraries.
     cxx_standard = compiler_configuration["cxx_standard"]
     stdlib = compiler_configuration["stdlib"]
+    sysroot_path = compiler_configuration["sysroot_path"]
     if stdlib == "builtin-libc++" and is_xcompile:
         stdlib = "stdc++"
     if stdlib == "builtin-libc++":
@@ -207,7 +203,7 @@ def cc_toolchain_config(
             # have the sysroot directory on the search path and then add the
             # toolchain directory back after we are done.
             link_flags.extend([
-                "-L{}/usr/lib".format(compiler_configuration["sysroot_path"]),
+                "-L{}/usr/lib".format(sysroot_path),
                 "-lc++",
                 "-lc++abi",
             ])
@@ -261,44 +257,6 @@ def cc_toolchain_config(
     ## NOTE: framework paths is missing here; unix_cc_toolchain_config
     ## doesn't seem to have a feature for this.
 
-    # C++ built-in include directories.
-    # This contains both the includes shipped with the compiler as well as the sysroot (or host)
-    # include directories. While Bazel's default undeclared inclusions check does not seem to be
-    # triggered by header files under the execroot, we still include those paths here as they are
-    # visible via the "built_in_include_directories" attribute of CcToolchainInfo as well as to keep
-    # them in sync with the directories included in the system module map generated for the stricter
-    # "layering_check" feature.
-    cxx_builtin_include_directories = [
-        toolchain_path_prefix + "include/c++/v1",
-        toolchain_path_prefix + "include/{}/c++/v1".format(target_system_name),
-        toolchain_path_prefix + "lib/clang/{}/include".format(llvm_version),
-        toolchain_path_prefix + "lib/clang/{}/share".format(llvm_version),
-        toolchain_path_prefix + "lib64/clang/{}/include".format(llvm_version),
-        toolchain_path_prefix + "lib/clang/{}/include".format(major_llvm_version),
-        toolchain_path_prefix + "lib/clang/{}/share".format(major_llvm_version),
-        toolchain_path_prefix + "lib64/clang/{}/include".format(major_llvm_version),
-    ]
-
-    sysroot_path = compiler_configuration["sysroot_path"]
-    sysroot_prefix = ""
-    if sysroot_path:
-        sysroot_prefix = "%sysroot%"
-    if target_os == "linux":
-        cxx_builtin_include_directories.extend([
-            sysroot_prefix + "/include",
-            sysroot_prefix + "/usr/include",
-            sysroot_prefix + "/usr/local/include",
-        ])
-    elif target_os == "darwin":
-        cxx_builtin_include_directories.extend([
-            sysroot_prefix + "/usr/include",
-            sysroot_prefix + "/System/Library/Frameworks",
-        ])
-    else:
-        fail("Unreachable")
-
-    cxx_builtin_include_directories.extend(compiler_configuration["additional_include_dirs"])
-
     ## NOTE: make variables are missing here; unix_cc_toolchain_config doesn't
     ## pass these to `create_cc_toolchain_config_info`.
 

toolchain/internal/configure.bzl

@@ -299,6 +299,52 @@ def _cc_toolchain_str(
     # them into `dict`s.
     host_tools_info = json.decode(json.encode(host_tools_info))
 
+    # C++ built-in include directories.
+    # This contains both the includes shipped with the compiler as well as the sysroot (or host)
+    # include directories. While Bazel's default undeclared inclusions check does not seem to be
+    # triggered by header files under the execroot, we still include those paths here as they are
+    # visible via the "built_in_include_directories" attribute of CcToolchainInfo as well as to keep
+    # them in sync with the directories included in the system module map generated for the stricter
+    # "layering_check" feature.
+    toolchain_path_prefix = toolchain_info.llvm_dist_path_prefix
+    llvm_version = toolchain_info.llvm_version
+    major_llvm_version = int(llvm_version.split(".")[0])
+    target_system_name = {
+        "darwin-x86_64": "x86_64-apple-macosx",
+        "darwin-aarch64": "aarch64-apple-macosx",
+        "linux-aarch64": "aarch64-unknown-linux-gnu",
+        "linux-x86_64": "x86_64-unknown-linux-gnu",
+    }[target_pair]
+    cxx_builtin_include_directories = [
+        toolchain_path_prefix + "include/c++/v1",
+        toolchain_path_prefix + "include/{}/c++/v1".format(target_system_name),
+        toolchain_path_prefix + "lib/clang/{}/include".format(llvm_version),
+        toolchain_path_prefix + "lib/clang/{}/share".format(llvm_version),
+        toolchain_path_prefix + "lib64/clang/{}/include".format(llvm_version),
+        toolchain_path_prefix + "lib/clang/{}/include".format(major_llvm_version),
+        toolchain_path_prefix + "lib/clang/{}/share".format(major_llvm_version),
+        toolchain_path_prefix + "lib64/clang/{}/include".format(major_llvm_version),
+    ]
+
+    sysroot_prefix = ""
+    if sysroot_path:
+        sysroot_prefix = "%sysroot%"
+    if target_os == "linux":
+        cxx_builtin_include_directories.extend([
+            sysroot_prefix + "/include",
+            sysroot_prefix + "/usr/include",
+            sysroot_prefix + "/usr/local/include",
+        ])
+    elif target_os == "darwin":
+        cxx_builtin_include_directories.extend([
+            sysroot_prefix + "/usr/include",
+            sysroot_prefix + "/System/Library/Frameworks",
+        ])
+    else:
+        fail("Unreachable")
+
+    cxx_builtin_include_directories.extend(toolchain_info.additional_include_dirs_dict.get(target_pair, []))
+
     template = """
 # CC toolchain for cc-clang-{suffix}.
 
@@ -308,11 +354,11 @@ cc_toolchain_config(
     host_os = "{host_os}",
     target_arch = "{target_arch}",
     target_os = "{target_os}",
+    target_system_name = "{target_system_name}",
     toolchain_path_prefix = "{llvm_dist_path_prefix}",
     tools_path_prefix = "{tools_path_prefix}",
     wrapper_bin_prefix = "{wrapper_bin_prefix}",
     compiler_configuration = {{
-      "additional_include_dirs": {additional_include_dirs},
       "sysroot_path": "{sysroot_path}",
       "stdlib": "{stdlib}",
       "cxx_standard": "{cxx_standard}",
@@ -327,8 +373,8 @@ cc_toolchain_config(
       "coverage_link_flags": {coverage_link_flags},
       "unfiltered_compile_flags": {unfiltered_compile_flags},
     }},
-    llvm_version = "{llvm_version}",
     host_tools_info = {host_tools_info},
+    cxx_builtin_include_directories = {cxx_builtin_include_directories},
 )
 
 toolchain(
@@ -436,8 +482,8 @@ filegroup(
 system_module_map(
     name = "module-{suffix}",
     cxx_builtin_include_files = ":include-components-{suffix}",
-    cxx_builtin_include_directories = cxx_builtin_include_directories,
-    sysroot_path = %{sysroot_path},
+    cxx_builtin_include_directories = {cxx_builtin_include_directories},
+    sysroot_path = "{sysroot_path}",
 )
 
 cc_toolchain(
@@ -463,14 +509,14 @@ cc_toolchain(
         host_arch = host_arch,
         target_settings = _list_to_string(_dict_value(toolchain_info.target_settings_dict, target_pair)),
         target_os_bzl = target_os_bzl,
+        target_system_name = target_system_name,
         host_os_bzl = host_os_bzl,
         llvm_dist_label_prefix = toolchain_info.llvm_dist_label_prefix,
         llvm_dist_path_prefix = toolchain_info.llvm_dist_path_prefix,
         tools_path_prefix = toolchain_info.tools_path_prefix,
         wrapper_bin_prefix = toolchain_info.wrapper_bin_prefix,
         sysroot_label_str = sysroot_label_str,
         sysroot_path = sysroot_path,
-        additional_include_dirs = _list_to_string(toolchain_info.additional_include_dirs_dict.get(target_pair, [])),
         stdlib = _dict_value(toolchain_info.stdlib_dict, target_pair, "builtin-libc++"),
         cxx_standard = _dict_value(toolchain_info.cxx_standard_dict, target_pair, "c++17"),
         compile_flags = _list_to_string(_dict_value(toolchain_info.compile_flags_dict, target_pair)),
@@ -483,9 +529,9 @@ cc_toolchain(
         coverage_compile_flags = _list_to_string(_dict_value(toolchain_info.coverage_compile_flags_dict, target_pair)),
         coverage_link_flags = _list_to_string(_dict_value(toolchain_info.coverage_link_flags_dict, target_pair)),
         unfiltered_compile_flags = _list_to_string(_dict_value(toolchain_info.unfiltered_compile_flags_dict, target_pair)),
-        llvm_version = toolchain_info.llvm_version,
         extra_files_str = extra_files_str,
         host_tools_info = host_tools_info,
+        cxx_builtin_include_directories = _list_to_string(cxx_builtin_include_directories),
     )
 
 def _convenience_targets_str(rctx, use_absolute_paths, llvm_dist_rel_path, llvm_dist_label_prefix, host_dl_ext):

toolchain/internal/generate_system_module_map.sh

@@ -17,6 +17,8 @@
 # limitations under the License.
 
 set -eu
+# shellcheck disable=SC3040
+set -o pipefail 2>/dev/null || true
 
 echo 'module "crosstool" [system] {'