configure: specify labels to toolchain roots in `toolchain_roots_labe…

…l_map`

As the comments within detail, this is necessary because we want to pass
`Label`s through to the repo rule/tag class so that their repository is
resolved in the context of the caller's repo mapping.

Additionally:
  - we want to allow 1:many mappings from system specification to
    toolchain root Label so a `Label -> string` attribute (i.e.
    `label_keyed_string_dict`) wouldn't work
  - we also still want to support absolute paths so even if we
    compromised on the 1:many mappings bit, flipping the map wouldn't
    work

Adding a level of indirection (`toolchain_roots_label_map`) was the
least-worst solution I could come up with.

While this is a breaking change, it shouldn't be too bad though; there's
no potential for silent errors and the error message provides a good
hint about what change needs to be made I think.

tests/MODULE.bazel

@@ -134,7 +134,10 @@ llvm.toolchain(
     llvm_version = LLVM_VERSION,
     # We can share the downloaded LLVM distribution with the first configuration.
     toolchain_roots = {
-        "": "@llvm_toolchain_llvm//",
+        "": "previous_llvm_toolchain",
+    },
+    toolchain_roots_label_map = {
+        "@llvm_toolchain_llvm": "previous_llvm_toolchain",
     },
 )
 use_repo(llvm, "llvm_toolchain_with_absolute_paths")
@@ -159,7 +162,10 @@ llvm.toolchain(
     },
     # We can share the downloaded LLVM distribution with the first configuration.
     toolchain_roots = {
-        "": "@llvm_toolchain_llvm//",
+        "": "previous_llvm_toolchain",
+    },
+    toolchain_roots_label_map = {
+        "@llvm_toolchain_llvm": "previous_llvm_toolchain",
     },
 )
 use_repo(llvm, "llvm_toolchain_with_sysroot")

tests/WORKSPACE

@@ -84,7 +84,10 @@ llvm_toolchain(
     llvm_version = LLVM_VERSION,
     # We can share the downloaded LLVM distribution with the first configuration.
     toolchain_roots = {
-        "": "@llvm_toolchain_llvm//",
+        "": "previous_llvm_toolchain",
+    },
+    toolchain_roots_label_map = {
+        "@llvm_toolchain_llvm": "previous_llvm_toolchain",
     },
 )
 
@@ -121,7 +124,10 @@ llvm_toolchain(
     },
     # We can share the downloaded LLVM distribution with the first configuration.
     toolchain_roots = {
-        "": "@llvm_toolchain_llvm//",
+        "": "previous_llvm_toolchain",
+    },
+    toolchain_roots_label_map = {
+        "@llvm_toolchain_llvm": "previous_llvm_toolchain",
     },
 )
 

toolchain/internal/common.bzl

@@ -134,6 +134,10 @@ def canonical_dir_path(path):
         return path + "/"
     return path
 
+# When bzlmod is enabled, canonical repos names have @@ in them, while under
+# workspace builds, there is never a @@ in labels.
+BZLMOD_ENABLED = "@@" in str(Label("//:unused"))
+
 def pkg_name_from_label(label):
     if label.workspace_name:
         return "@" + label.workspace_name + "//" + label.package

toolchain/internal/configure.bzl

@@ -31,16 +31,19 @@ load(
     _pkg_path_from_label = "pkg_path_from_label",
     _supported_targets = "SUPPORTED_TARGETS",
     _toolchain_tools = "toolchain_tools",
+    _BZLMOD_ENABLED = "BZLMOD_ENABLED",
 )
 load(
     "//toolchain/internal:sysroot.bzl",
     _default_sysroot_path = "default_sysroot_path",
     _sysroot_paths_dict = "sysroot_paths_dict",
 )
 
-# When bzlmod is enabled, canonical repos names have @@ in them, while under
-# workspace builds, there is never a @@ in labels.
-BZLMOD_ENABLED = "@@" in str(Label("//:unused"))
+def _make_string_label(repository, package, target):
+    return "{prefix}{repo}//{package}:{target}".format(
+        prefix = "@@" if _BZLMOD_ENABLED else "@",
+        repo = repository, package = package, target = target,
+    )
 
 def llvm_config_impl(rctx):
     _check_os_arch_keys(rctx.attr.sysroot)
@@ -55,40 +58,61 @@ def llvm_register_toolchains():
 """)
         return
     arch = _arch(rctx)
+    system_llvm = False
 
     if not rctx.attr.toolchain_roots:
-        toolchain_root = "@@%s_llvm//" % rctx.attr.name if BZLMOD_ENABLED else "@%s_llvm//" % rctx.attr.name
+        # target name does not matter here; it's dropped later
+        toolchain_root = Label(_make_string_label("%s_llvm" % rctx.attr.name, "", "BUILD.bazel"))
     else:
         (_key, toolchain_root) = _host_os_arch_dict_value(rctx, "toolchain_roots")
 
-    if not toolchain_root:
-        fail("LLVM toolchain root missing for ({}, {})".format(os, arch))
+        if not toolchain_root:
+            fail("LLVM toolchain root missing for ({}, {})".format(os, arch))
+
+        # Check if the toolchain root is a system path.
+        if toolchain_root[0] == "/" and (len(toolchain_root) == 1 or toolchain_root[1] != "/"):
+            use_absolute_paths_llvm = True
+            system_llvm = True
+        else:
+            # Otherwise, interpret as a key into `toolchain_roots_label_map`:
+
+            # Flip the map (note: no error on duplicate "keys" for now)
+            label_map = { key: label for label, key in rctx.attr.toolchain_roots_label_map.items() }
+
+            if not toolchain_root in label_map:
+                fail(("Value in `toolchain_roots` {key} was interpreted as a `toolchain_roots_label_map` " +
+                    "key but it is not present in `toolchain_roots_label_map`: {map}").format(
+                    key = toolchain_root, map = label_map,
+                ))
+            toolchain_root = label_map[toolchain_root]
+
     (_key, llvm_version) = _host_os_arch_dict_value(rctx, "llvm_versions")
     if not llvm_version:
         fail("LLVM version string missing for ({}, {})".format(os, arch))
 
     use_absolute_paths_llvm = rctx.attr.absolute_paths
     use_absolute_paths_sysroot = use_absolute_paths_llvm
 
-    # Check if the toolchain root is a system path.
-    system_llvm = False
-    if toolchain_root[0] == "/" and (len(toolchain_root) == 1 or toolchain_root[1] != "/"):
-        use_absolute_paths_llvm = True
-        system_llvm = True
-
     # Paths for LLVM distribution:
     if system_llvm:
         llvm_dist_path_prefix = _canonical_dir_path(toolchain_root)
     else:
-        llvm_dist_label = Label(toolchain_root + ":BUILD.bazel")  # Exact target does not matter.
+        # Exact target does not matter but it does need to exist!
+        #
+        # TODO: should we just use the target name in the Label that was given?
+        llvm_dist_label = Label(_make_string_label(
+            toolchain_root.workspace_name, toolchain_root.package, "BUILD.bazel",
+        ))
         if use_absolute_paths_llvm:
             llvm_dist_path_prefix = _canonical_dir_path(str(rctx.path(llvm_dist_label).dirname))
         else:
             llvm_dist_path_prefix = _pkg_path_from_label(llvm_dist_label)
 
     if not use_absolute_paths_llvm:
         llvm_dist_rel_path = _canonical_dir_path("../../" + llvm_dist_path_prefix)
-        llvm_dist_label_prefix = toolchain_root + ":"
+        llvm_dist_label_prefix = _make_string_label(
+            toolchain_root.workspace_name, toolchain_root.package, "",
+        )
 
         # tools can only be defined as absolute paths or in a subdirectory of
         # config_repo_path, because their paths are relative to the package

toolchain/internal/repo.bzl

@@ -242,8 +242,39 @@ llvm_config_attrs.update({
                "all hosts, e.g. with the llvm_toolchain_repo rule. " +
                "If the value begins with exactly one forward slash '/', then the value is " +
                "assumed to be a system path and the toolchain is configured to use absolute " +
-               "paths. Else, the value will be assumed to be a bazel package containing the " +
-               "filegroup targets as in BUILD.llvm_repo."),
+               "paths. Else, the value will be interpreted as a key into the " +
+               "`toolchain_roots_label_map` attribute. This can be used to specify a bazel package " +
+               "containing the filegroup targets as in BUILD.llvm_repo."),
+    ),
+    "toolchain_roots_label_map": attr.label_keyed_string_dict(
+        mandatory = False,
+        # NOTE: Ideally this attribute wouldn't exist and we'd just be able to
+        # have `toolchain_roots` specify labels directly.
+        #
+        # We need this attribute for a few reasons:
+        #   - We want users to be able to specify roots that point to repos that
+        #     are in *their* repo mapping but not necessary in this repository's
+        #     mapping. To do this, we need `Label`s (not just label-like strings
+        #     but actual `Label`s) to be passed to this rule so that the repo of
+        #     the desired toolchain root is resolved in the context of the
+        #     user's mapping. (this is especially relevant for bzlmod)
+        #   - The interface `toolchain_roots` has permits labels OR absolute
+        #     paths as roots; this does not allow of attributes (i.e.
+        #     `label_keyed_string_dict`) that resolve arguments as Labels for
+        #     us. Further, we don't have the ability to create "string to Label"
+        #     attribute maps which is somewhat necessary for `toolchain_roots`:
+        #     we specifically *do* want to be able to have multiple system keys
+        #     be mapped to the same toolchain root. Hence, this extra level of
+        #     indirection.
+        #
+        # This map is inverted (Label -> key instead of key -> Label) because
+        # there isn't a `string_keyed_label_dict`.
+        doc = ("Inverted map from toolchain root Label to string identifier.\n" +
+                "This attribute is to be used with `toolchain_roots` in order to specify roots " +
+                "that come from a Bazel package. Keys can be any identifier that do not start " +
+                "with a forward slash '/`.\n" +
+                "Note that the target name in the Label to the root provided here is ignored: " +
+                "only the Label's repository and package are used."),
     ),
     "absolute_paths": attr.bool(
         default = False,