Resolve Labels for toolchain roots and sysroots correctly under bzlmod (

#235)

Currently the repo rule and tag class accept string-form labels for
toolchain root packages and sysroots. Under `bzlmod` this is problematic
because users may pass us labels that point at repos that are not in
this module's repo mapping. To support such labels, they need to be
passed to us as actual `Label`s (not strings).

This necessitates some (**breaking**) changes to interface for the
module extension tags. The repo rule interface remains the same.

For the "llvm" module extension, two new tags have been introduced:
- `toolchain_root`, and 
- `sysroot` 

Alternatives considered:
1. Using a `label_keyed_string_dict` would not work if we still want to
    support absolute paths in these attributes.
2. Using string aliases instead of string labels, and then a separate
    attribute for a side table that maps labels to their aliases could also
    work. This would have to be done only for the module extension
    and not the repo rule, because specifying labels in repo rules
    eagerly fetches them.

I've also enabled bzlmod-enabled tests for the system paths, absolute
paths, and cross tests in CI.

Fixes #234. cc: @steve-261370

---------

Co-authored-by: Siddhartha Bagaria <starsid@gmail.com>

.github/workflows/tests.yml

@@ -74,26 +74,45 @@ jobs:
             ubuntu_22_04,
             linux_sysroot,
           ]
+        bzlmod: [true, false]
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
       - name: Test
+        env:
+          USE_BZLMOD: ${{ matrix.bzlmod }}
         run: tests/scripts/${{ matrix.script }}_test.sh
   xcompile_test:
+    strategy:
+      fail-fast: false
+      matrix:
+        bzlmod: [true, false]
     runs-on: macos-latest
     steps:
       - uses: actions/checkout@v4
       - name: Test
+        env:
+          USE_BZLMOD: ${{ matrix.bzlmod }}
         run: tests/scripts/run_xcompile_tests.sh
   abs_paths_test:
+    strategy:
+      fail-fast: false
+      matrix:
+        bzlmod: [true, false]
     runs-on: ubuntu-latest
     steps:
       - name: Install APT packages
         run: sudo apt-get -y install libtinfo5
       - uses: actions/checkout@v4
       - name: Test
+        env:
+          USE_BZLMOD: ${{ matrix.bzlmod }}
         run: tests/scripts/run_tests.sh -t @llvm_toolchain_with_absolute_paths//:cc-toolchain-x86_64-linux
   sys_paths_test:
+    strategy:
+      fail-fast: false
+      matrix:
+        bzlmod: [true, false]
     runs-on: ubuntu-latest
     steps:
       - name: Install APT packages
@@ -107,4 +126,6 @@ jobs:
           local_path: /opt/llvm-16
         run: wget --no-verbose "https://github.com/llvm/llvm-project/releases/download/${release}/${archive}${ext}" && tar -xf "${archive}${ext}" && mv "${archive}" "${local_path}"
       - name: Test
+        env:
+          USE_BZLMOD: ${{ matrix.bzlmod }}
         run: tests/scripts/run_tests.sh -t @llvm_toolchain_with_system_llvm//:cc-toolchain-x86_64-linux

README.md

@@ -115,7 +115,8 @@ The following mechanisms are available for using an LLVM toolchain:
    the archive is downloaded and extracted as a separate repository with the
    suffix `_llvm`.
 3. You can also specify your own bazel package paths or local absolute paths
-   for each host os-arch pair through the `toolchain_roots` attribute. Note
+   for each host os-arch pair through the `toolchain_roots` attribute (without
+   bzlmod) or the `toolchain_root` module extension tags (with bzlmod). Note
    that the keys here are different and less granular than the keys in the `urls`
    attribute. When using a bazel package path, each of the values is typically
    a package in the user's workspace or configured through `local_repository` or
@@ -132,11 +133,12 @@ The following mechanisms are available for using an LLVM toolchain:
 
 ### Sysroots
 
-A sysroot can be specified through the `sysroot` attribute. This can be either
-a path on the user's system, or a bazel `filegroup` like label. One way to
-create a sysroot is to use `docker export` to get a single archive of the
-entire filesystem for the image you want. Another way is to use the build
-scripts provided by the [Chromium
+A sysroot can be specified through the `sysroot` attribute (without bzlmod) or
+the `sysroot` module extension tag (with bzlmod). This can be either a path on
+the user's system, or a bazel `filegroup` like label. One way to create a
+sysroot is to use `docker export` to get a single archive of the entire
+filesystem for the image you want. Another way is to use the build scripts
+provided by the [Chromium
 project](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/linux/sysroot.md).
 
 ### Cross-compilation

tests/MODULE.bazel

@@ -56,8 +56,6 @@ llvm = use_extension("@toolchains_llvm//toolchain/extensions:llvm.bzl", "llvm")
 # When updating this version, also update the versions associated with
 # llvm_toolchain below, sys_paths_test in the workflows file, and xcompile_test
 # through the `llvm_toolchain_with_sysroot` toolchain.
-LLVM_VERSION = "15.0.6"
-
 LLVM_VERSIONS = {
     "": "16.0.0",
     "darwin-aarch64": "16.0.5",
@@ -113,35 +111,40 @@ use_repo(llvm, "llvm_toolchain_13_0_0")
 llvm.toolchain(
     name = "llvm_toolchain_with_absolute_paths",
     absolute_paths = True,
-    llvm_version = LLVM_VERSION,
-    # We can share the downloaded LLVM distribution with the first configuration.
-    toolchain_roots = {
-        "": "@llvm_toolchain_llvm//",
-    },
+    llvm_versions = LLVM_VERSIONS,
+)
+# We can share the downloaded LLVM distribution with the first configuration.
+llvm.toolchain_root(
+    name = "llvm_toolchain_with_absolute_paths",
+    label = "@llvm_toolchain_llvm//:BUILD",
 )
 use_repo(llvm, "llvm_toolchain_with_absolute_paths")
 
 # Toolchain example with system LLVM; tested in GitHub CI.
 llvm.toolchain(
     name = "llvm_toolchain_with_system_llvm",
-    llvm_version = LLVM_VERSION,
-    # For this toolchain to work, the LLVM distribution archive would need to be unpacked here.
-    toolchain_roots = {"": "/opt/llvm-16"},
+    llvm_versions = LLVM_VERSIONS,
+)
+# For this toolchain to work, the LLVM distribution archive would need to be unpacked here.
+llvm.toolchain_root(
+    name = "llvm_toolchain_with_system_llvm",
+    path = "/opt/llvm-16",
 )
 use_repo(llvm, "llvm_toolchain_with_system_llvm")
 
 # Toolchain example with a sysroot.
 llvm.toolchain(
     name = "llvm_toolchain_with_sysroot",
     llvm_versions = LLVM_VERSIONS,
-    sysroot = {
-        # TODO: Use an apparent repo name after #234 has been fixed.
-        "linux-x86_64": "@@org_chromium_sysroot_linux_x64//:sysroot",
-    },
-    # We can share the downloaded LLVM distribution with the first configuration.
-    toolchain_roots = {
-        # TODO: Use an apparent repo name after #234 has been fixed.
-        "": "@@toolchains_llvm~~llvm~llvm_toolchain_llvm//",
-    },
+)
+# We can share the downloaded LLVM distribution with the first configuration.
+llvm.toolchain_root(
+    name = "llvm_toolchain_with_sysroot",
+    label = "@llvm_toolchain_llvm//:BUILD",
+)
+llvm.sysroot(
+    name = "llvm_toolchain_with_sysroot",
+    targets = ["linux-x86_64"],
+    label = "@@org_chromium_sysroot_linux_x64//:sysroot",
 )
 use_repo(llvm, "llvm_toolchain_with_sysroot")

tests/WORKSPACE

@@ -29,8 +29,6 @@ load("@toolchains_llvm//toolchain:rules.bzl", "llvm_toolchain")
 # When updating this version, also update the versions associated with
 # llvm_toolchain below, sys_paths_test in the workflows file, and xcompile_test
 # through the `llvm_toolchain_with_sysroot` toolchain.
-LLVM_VERSION = "16.0.0"
-
 LLVM_VERSIONS = {
     "": "16.0.0",
     "darwin-aarch64": "16.0.5",
@@ -81,7 +79,7 @@ llvm_register_toolchains()
 llvm_toolchain(
     name = "llvm_toolchain_with_absolute_paths",
     absolute_paths = True,
-    llvm_version = LLVM_VERSION,
+    llvm_versions = LLVM_VERSIONS,
     # We can share the downloaded LLVM distribution with the first configuration.
     toolchain_roots = {
         "": "@llvm_toolchain_llvm//",
@@ -91,7 +89,7 @@ llvm_toolchain(
 ## Toolchain example with system LLVM; tested in GitHub CI.
 llvm_toolchain(
     name = "llvm_toolchain_with_system_llvm",
-    llvm_version = LLVM_VERSION,
+    llvm_versions = LLVM_VERSIONS,
     # For this toolchain to work, the LLVM distribution archive would need to be unpacked here.
     toolchain_roots = {"": "/opt/llvm-16"},
 )

tests/scripts/archlinux_test.sh

@@ -27,7 +27,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Run tests

tests/scripts/bazel.sh

@@ -37,7 +37,7 @@ bazel="${TMPDIR:-/tmp}/bazelisk"
 readonly bazel
 
 common_args=(
-  "--enable_bzlmod=${USE_BZLMOD:-false}"
+  "--enable_bzlmod=${USE_BZLMOD:-true}"
 )
 
 # shellcheck disable=SC2034
@@ -48,6 +48,7 @@ common_test_args=(
   "--show_progress_rate_limit=30"
   "--keep_going"
   "--test_output=errors"
+  "--features=layering_check"
 )
 
 # Do not run autoconf to configure local CC toolchains.

tests/scripts/centos_test.sh

@@ -30,7 +30,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Need system glibc and headers.

tests/scripts/debian_test.sh

@@ -24,7 +24,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

tests/scripts/fedora_test.sh

@@ -24,7 +24,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Need system glibc headers (e.g. features.h).

tests/scripts/linux_sysroot_test.sh

@@ -24,7 +24,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

tests/scripts/suse_leap_test.sh

@@ -29,7 +29,7 @@ echo "git root: ${git_root}"
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD -volume="${git_root}:/src" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

tests/scripts/suse_tumbleweed_test.sh

@@ -29,7 +29,7 @@ echo "git root: ${git_root}"
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

tests/scripts/ubuntu_20_04_test.sh

@@ -24,7 +24,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

tests/scripts/ubuntu_22_04_test.sh

@@ -24,7 +24,7 @@ readonly git_root
 
 for image in "${images[@]}"; do
   docker pull "${image}"
-  docker run --rm --entrypoint=/bin/bash --volume="${git_root}:/src:ro" "${image}" -c """
+  docker run --rm --entrypoint=/bin/bash --env USE_BZLMOD --volume="${git_root}:/src:ro" "${image}" -c """
 set -exuo pipefail
 
 # Common setup

toolchain/extensions/llvm.bzl

@@ -6,21 +6,62 @@ load(
     _llvm_config_attrs = "llvm_config_attrs",
     _llvm_repo_attrs = "llvm_repo_attrs",
 )
+load(
+    "//toolchain/internal:common.bzl",
+    _is_absolute_path = "is_absolute_path",
+)
+
+def _root_dict(roots, cls, name, strip_target):
+    res = {}
+    for root in roots:
+        targets = list(root.targets)
+        if not targets:
+            targets = [""]
+        for target in targets:
+            if res.get(target):
+                fail("duplicate target '%s' found for %s with name '%s'" % (target, cls, name))
+            if bool(root.path) == (root.label):
+                fail("target '%s' for %s with name '%s' must have either path or label, but not both" % (target, cls, name))
+            if root.path:
+                if not _is_absolute_path(root.path):
+                    fail("target '%s' for %s with name '%s' must have an absolute path value" % (target, cls, name))
+                res.update([(target, root.path)])
+                continue
+            label_str = str(root.label)
+            if strip_target:
+                label_str = label_str.split(":")[0]
+            res.update([(target, label_str)])
+
+    return res
 
 def _llvm_impl_(module_ctx):
     for mod in module_ctx.modules:
         if not mod.is_root:
             fail("Only the root module can use the 'llvm' extension")
+        toolchain_names = []
         for toolchain_attr in mod.tags.toolchain:
+            name = toolchain_attr.name
+            toolchain_names.append(name)
             attrs = {
                 key: getattr(toolchain_attr, key)
                 for key in dir(toolchain_attr)
                 if not key.startswith("_")
             }
+            attrs["toolchain_roots"] = _root_dict([root for root in mod.tags.toolchain_root if root.name == name], "toolchain_root", name, True)
+            attrs["sysroot"] = _root_dict([sysroot for sysroot in mod.tags.sysroot if sysroot.name == name], "sysroot", name, False)
+
             llvm_toolchain(
                 **attrs
             )
 
+        # Check that every defined toolchain_root or sysroot has a corresponding toolchain.
+        for root in mod.tags.toolchain_root:
+            if root.name not in toolchain_names:
+                fail("toolchain_root '%s' does not have a corresponding toolchain" % root.name)
+        for root in mod.tags.sysroot:
+            if root.name not in toolchain_names:
+                fail("sysroot '%s' does not have a corresponding toolchain" % root.name)
+
 _attrs = {
     "name": attr.string(doc = """\
         Base name for the generated repositories, allowing more than one LLVM toolchain to be registered.
@@ -29,11 +70,30 @@ _attrs = {
 _attrs.update(_llvm_config_attrs)
 _attrs.update(_llvm_repo_attrs)
 
+_attrs.pop("toolchain_roots", None)
+_attrs.pop("sysroot", None)
+
 llvm = module_extension(
     implementation = _llvm_impl_,
     tag_classes = {
         "toolchain": tag_class(
             attrs = _attrs,
         ),
+        "toolchain_root": tag_class(
+            attrs = {
+                "name": attr.string(doc = "Same name as the toolchain tag.", default = "llvm_toolchain"),
+                "targets": attr.string_list(doc = "Specific targets, if any; empty list means this applies to all."),
+                "label": attr.label(doc = "Dummy label whose package path is the toolchain root package."),
+                "path": attr.string(doc = "Absolute path to the toolchain root."),
+            },
+        ),
+        "sysroot": tag_class(
+            attrs = {
+                "name": attr.string(doc = "Same name as the toolchain tag.", default = "llvm_toolchain"),
+                "targets": attr.string_list(doc = "Specific targets, if any; empty list means this applies to all."),
+                "label": attr.label(doc = "Label containing the files with its package path as the sysroot path."),
+                "path": attr.string(doc = "Absolute path to the sysroot."),
+            },
+        ),
     },
 )

toolchain/internal/common.bzl

@@ -164,6 +164,9 @@ def canonical_dir_path(path):
         return path + "/"
     return path
 
+def is_absolute_path(val):
+    return val and val[0] == "/" and (len(val) == 1 or val[1] != "/")
+
 def pkg_name_from_label(label):
     if label.workspace_name:
         return "@" + label.workspace_name + "//" + label.package