Make system module map generation faster and fully hermetic (#280)

* `system_module_maps` no longer performs any IO.
* The generated module map no longer references any non-hermetic paths
and can thus be cached remotely, even when toolchain or sysroot are
provided as absolute paths.

README.md

@@ -251,11 +251,6 @@ deps (also known as "depend on what you use") for `cc_*` rules. This feature
 can be enabled by enabling the `layering_check` feature on a per-target,
 per-package or global basis.
 
-If one of toolchain or sysroot are specified via an absolute path rather than
-managed by Bazel, the `layering_check` feature may require running
-`bazel clean --expunge` after making changes to the set of header files
-installed on the host.
-
 ## Prior Art
 
 Other examples of toolchain configuration:

toolchain/internal/BUILD.bazel

@@ -12,4 +12,4 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-exports_files(["generate_system_module_map.sh"])
+exports_files(["template.modulemap"])

toolchain/internal/configure.bzl

@@ -545,7 +545,14 @@ cc_toolchain(
         unfiltered_compile_flags = _list_to_string(_dict_value(toolchain_info.unfiltered_compile_flags_dict, target_pair)),
         extra_files_str = extra_files_str,
         host_tools_info = host_tools_info,
-        cxx_builtin_include_directories = _list_to_string(cxx_builtin_include_directories),
+        cxx_builtin_include_directories = _list_to_string([
+            # Filter out non-existing directories with absolute paths as they
+            # result in a -Wincomplete-umbrella warning when mentioned in the
+            # system module map.
+            dir
+            for dir in cxx_builtin_include_directories
+            if _is_hermetic_or_exists(rctx, dir, sysroot_prefix)
+        ]),
         extra_compiler_files = ("\"%s\"," % str(toolchain_info.extra_compiler_files)) if toolchain_info.extra_compiler_files else "",
     )
 
@@ -586,3 +593,9 @@ native_binary(
         llvm_dist_label_prefix = llvm_dist_label_prefix,
         host_dl_ext = host_dl_ext,
     )
+
+def _is_hermetic_or_exists(rctx, path, sysroot_prefix):
+    path = path.replace("%sysroot%", sysroot_prefix)
+    if not path.startswith("/"):
+        return True
+    return rctx.path(path).exists

toolchain/internal/generate_system_module_map.sh

@@ -1,35 +0,0 @@
-#!/bin/sh
-# Based on:
-# https://github.com/bazelbuild/bazel/blob/44c5a1bbb26d3c61b37529b38406f1f5b0832baf/tools/cpp/generate_system_module_map.sh
-#
-# Copyright 2020 The Bazel Authors. All rights reserved.
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#    http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-set -eu
-
-echo 'module "crosstool" [system] {'
-
-for dir in "$@"; do
-  find -L "${dir}" -type f 2>/dev/null | LANG=C sort | uniq | while read -r header; do
-    case "${dir}" in
-    /*) ;;
-    *) header="${EXECROOT_PREFIX}${header}" ;;
-    esac
-    # The module map is expected to contain all possibly transitively included headers, including
-    # those provided by the sysroot or the host machine.
-    echo "  textual header \"${header}\""
-  done
-done
-
-echo "}"

toolchain/internal/system_module_map.bzl

@@ -14,62 +14,81 @@
 
 load("@bazel_skylib//lib:paths.bzl", "paths")
 
+def _textual_header(file, *, include_prefixes, execroot_prefix):
+    path = file.path
+    for include_prefix in include_prefixes:
+        if path.startswith(include_prefix):
+            return "  textual header \"{}{}\"".format(execroot_prefix, path)
+
+    # The file is not under any of the include prefixes,
+    return None
+
+def _umbrella_submodule(path):
+    return """
+  module "{path}" {{
+    umbrella "{path}"
+  }}""".format(path = path)
+
 def _system_module_map(ctx):
     module_map = ctx.actions.declare_file(ctx.attr.name + ".modulemap")
 
-    dirs = []
-    non_hermetic = False
-    for dir in ctx.attr.cxx_builtin_include_directories:
-        if ctx.attr.sysroot_path and dir.startswith("%sysroot%"):
-            dir = ctx.attr.sysroot_path + dir[len("%sysroot%"):]
-        if dir.startswith("/"):
-            non_hermetic = True
-        dirs.append(paths.normalize(dir))
-
-    # If the action references a file outside of the execroot, it isn't safe to
-    # cache or run remotely.
-    execution_requirements = {}
-    if non_hermetic:
-        execution_requirements = {
-            "no-cache": "",
-            "no-remote": "",
-        }
+    absolute_path_dirs = []
+    relative_include_prefixes = []
+    for include_dir in ctx.attr.cxx_builtin_include_directories:
+        if ctx.attr.sysroot_path and include_dir.startswith("%sysroot%"):
+            include_dir = ctx.attr.sysroot_path + include_dir[len("%sysroot%"):]
+        include_dir = paths.normalize(include_dir)
+        if include_dir.startswith("/"):
+            absolute_path_dirs.append(include_dir)
+        else:
+            relative_include_prefixes.append(include_dir + "/")
 
     # The builtin include directories are relative to the execroot, but the
     # paths in the module map must be relative to the directory that contains
     # the module map.
     execroot_prefix = (module_map.dirname.count("/") + 1) * "../"
+    textual_header_closure = lambda file: _textual_header(
+        file,
+        include_prefixes = relative_include_prefixes,
+        execroot_prefix = execroot_prefix,
+    )
 
-    ctx.actions.run_shell(
-        outputs = [module_map],
-        inputs = ctx.attr.cxx_builtin_include_files[DefaultInfo].files,
-        command = """
-{tool} "$@" > {module_map}
-""".format(
-            tool = ctx.executable._generate_system_module_map.path,
-            module_map = module_map.path,
-        ),
-        arguments = dirs,
-        tools = [ctx.executable._generate_system_module_map],
-        env = {"EXECROOT_PREFIX": execroot_prefix},
-        execution_requirements = execution_requirements,
-        mnemonic = "LLVMSystemModuleMap",
-        progress_message = "Generating system module map",
+    template_dict = ctx.actions.template_dict()
+    template_dict.add_joined(
+        "%textual_headers%",
+        ctx.attr.cxx_builtin_include_files[DefaultInfo].files,
+        join_with = "\n",
+        map_each = textual_header_closure,
+        allow_closure = True,
+    )
+    template_dict.add_joined(
+        "%umbrella_submodules%",
+        depset(absolute_path_dirs),
+        join_with = "\n",
+        map_each = _umbrella_submodule,
+    )
+
+    ctx.actions.expand_template(
+        template = ctx.file._module_map_template,
+        output = module_map,
+        computed_substitutions = template_dict,
     )
     return DefaultInfo(files = depset([module_map]))
 
 system_module_map = rule(
-    doc = """Generates a Clang module map for the toolchain and sysroot headers.""",
+    doc = """Generates a Clang module map for the toolchain and sysroot headers.
+
+    Files under the configured built-in include directories that are managed by
+    Bazel are included as textual headers. All directories referenced by
+    absolute paths are included as umbrella submodules.""",
     implementation = _system_module_map,
     attrs = {
         "cxx_builtin_include_files": attr.label(mandatory = True),
         "cxx_builtin_include_directories": attr.string_list(mandatory = True),
         "sysroot_path": attr.string(),
-        "_generate_system_module_map": attr.label(
-            default = ":generate_system_module_map.sh",
+        "_module_map_template": attr.label(
+            default = "template.modulemap",
             allow_single_file = True,
-            cfg = "exec",
-            executable = True,
         ),
     },
 )

toolchain/internal/template.modulemap

@@ -0,0 +1,4 @@
+module "crosstool" [system] {
+%textual_headers%
+%umbrella_submodules%
+}