Apply review comments

MODULE.bazel

@@ -1,10 +1,9 @@
 module(
-    name = "com_grail_bazel_toolchain",
+    name = "grail_llvm_toolchain",
     version = "0.0.0",
     compatibility_level = 0,
 )
 
-bazel_dep(name = "bazel_skylib", version = "1.4.1")
-bazel_dep(name = "rules_cc", version = "0.0.6")
-bazel_dep(name = "rules_foreign_cc", version = "0.9.0")
+bazel_dep(name = "bazel_skylib", version = "1.4.2")
+bazel_dep(name = "rules_cc", version = "0.0.8")
 bazel_dep(name = "platforms", version = "0.0.6")

README.md

@@ -25,9 +25,9 @@ Minimum bazel version: **4.2.1**
 If you're using `bzlmod`, add the following to `MODULE.bazel`:
 
 ```starlark
-bazel_dep(name = "com_grail_bazel_toolchain", version = "0.8.2")
+bazel_dep(name = "grail_llvm_toolchain", version = "0.8.2")
 
-llvm = use_extension("@com_grail_bazel_toolchain//toolchain:extensions.bzl", "llvm")
+llvm = use_extension("@grail_llvm_toolchain//toolchain/extensions:llvm.bzl", "llvm")
 llvm.toolchain(
    llvm_version = "15.0.6",
 )
@@ -54,11 +54,11 @@ http_archive(
     url = "https://github.com/grailbio/bazel-toolchain/archive/refs/tags/{tag}.tar.gz".format(tag = BAZEL_TOOLCHAIN_TAG),
 )
 
-load("@com_grail_bazel_toolchain//toolchain:deps.bzl", "bazel_toolchain_dependencies")
+load("@grail_llvm_toolchain//toolchain:deps.bzl", "bazel_toolchain_dependencies")
 
 bazel_toolchain_dependencies()
 
-load("@com_grail_bazel_toolchain//toolchain:rules.bzl", "llvm_toolchain")
+load("@grail_llvm_toolchain//toolchain:rules.bzl", "llvm_toolchain")
 
 llvm_toolchain(
     name = "llvm_toolchain",
@@ -190,7 +190,7 @@ The following mechanisms are available for using an LLVM toolchain:
    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
    `http_archive`; the BUILD file of the package should be similar to
-   `@com_grail_bazel_toolchain//toolchain:BUILD.llvm_repo`. If using only
+   `@grail_llvm_toolchain//toolchain:BUILD.llvm_repo`. If using only
    `http_archive`, maybe consider using the `urls` attribute instead to get more
    flexibility if you need.
 4. All the above options rely on host OS information, and are not suited for
@@ -227,7 +227,7 @@ and the target platform. For example, see the [WORKSPACE](tests/WORKSPACE) file
 the [test script](tests/scripts/run_xcompile_tests.sh) for cross-compilation.
 ```
 bazel build \
-  --platforms=@com_grail_bazel_toolchain//platforms:linux-x86_64 \
+  --platforms=@grail_llvm_toolchain//platforms:linux-x86_64 \
   --extra_toolchains=@llvm_toolchain_with_sysroot//:cc-toolchain-x86_64-linux \
   //...
 ```

tests/BUILD.bazel

@@ -121,7 +121,7 @@ sh_test(
     srcs = ["file_dependency_test.sh"],
     args = [
         "$(execpath @llvm_toolchain_llvm//:bin/clang-format)",
-        "$(locations @llvm_toolchain_llvm//:lib)",
+        "$(rootpaths @llvm_toolchain_llvm//:lib)",
     ],
     data = [
         "@llvm_toolchain_llvm//:bin/clang-format",

tests/MODULE.bazel

@@ -1,8 +1,7 @@
 module(name = "com_grail_bazel_toolchain_tests")
 
-bazel_dep(name = "com_grail_bazel_toolchain", version = "0.8.2")
-
-local_path_override(module_name = "com_grail_bazel_toolchain", path = "..")
+bazel_dep(name = "grail_llvm_toolchain", version = "0.8.2")
+local_path_override(module_name = "grail_llvm_toolchain", path = "..")
 
 bazel_dep(name = "bazel_skylib", version = "1.4.1")
 bazel_dep(name = "platforms", version = "0.0.6")
@@ -13,11 +12,11 @@ bazel_dep(name = "rules_rust", version = "0.25.1")
 
 # We have to patch abseil-cpp to add a dep on googletest
 bazel_dep(name = "abseil-cpp", repo_name = "com_google_absl")
-archive_override(module_name = "abseil-cpp", urls = ["https://github.com/abseil/abseil-cpp/archive/refs/tags/20230125.3.tar.gz"], patches = ["//patches:abseil-cpp.patch"], strip_prefix="abseil-cpp-20230125.3")
+archive_override(module_name = "abseil-cpp", urls = ["https://github.com/abseil/abseil-cpp/archive/refs/tags/20230125.3.tar.gz"], patches = ["//patches:abseil-cpp.patch"], strip_prefix = "abseil-cpp-20230125.3")
 
 # Temporary overrides until newer versions of these rulesets are released in the central registry
-git_override(module_name = "rules_foreign_cc", remote = "https://github.com/bazelbuild/rules_foreign_cc.git", commit="6ecc134b114f6e086537f5f0148d166467042226")
-git_override(module_name = "rules_rust", remote = "https://github.com/bazelbuild/rules_rust.git", commit="bf9ddeb7c83a9fe8a7d87c76134cdd8e16131b62")
+git_override(module_name = "rules_foreign_cc", remote = "https://github.com/bazelbuild/rules_foreign_cc.git", commit = "6ecc134b114f6e086537f5f0148d166467042226")
+git_override(module_name = "rules_rust", remote = "https://github.com/bazelbuild/rules_rust.git", commit = "bf9ddeb7c83a9fe8a7d87c76134cdd8e16131b62")
 
 rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
 rust.toolchain(edition = "2021")
@@ -28,7 +27,7 @@ use_repo(
 
 register_toolchains("@rust_toolchains//:all")
 
-llvm = use_extension("@com_grail_bazel_toolchain//toolchain:extensions.bzl", "llvm")
+llvm = use_extension("@grail_llvm_toolchain//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

tests/WORKSPACE

@@ -20,11 +20,11 @@ local_repository(
 )
 
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
-load("@com_grail_bazel_toolchain//toolchain:deps.bzl", "bazel_toolchain_dependencies")
+load("@grail_llvm_toolchain//toolchain:deps.bzl", "bazel_toolchain_dependencies")
 
 bazel_toolchain_dependencies()
 
-load("@com_grail_bazel_toolchain//toolchain:rules.bzl", "llvm_toolchain")
+load("@grail_llvm_toolchain//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

tests/openssl/openssl.bazel

@@ -50,10 +50,10 @@ CONF_FILES = [
 
 genrule(
     name = "configure",
-    srcs = [("@com_grail_bazel_toolchain//tests/openssl:" + f) for f in CONF_FILES],
+    srcs = [("@grail_llvm_toolchain//tests/openssl:" + f) for f in CONF_FILES],
     outs = CONF_FILES,
     cmd = "\n".join([
-        "cp $(location @com_grail_bazel_toolchain//tests/openssl:{0}) $(location {0})".format(f)
+        "cp $(location @grail_llvm_toolchain//tests/openssl:{0}) $(location {0})".format(f)
         for f in CONF_FILES
     ]),
     visibility = ["//visibility:private"],

tests/scripts/run_xcompile_tests.sh

@@ -36,7 +36,7 @@ echo ""
 echo "Testing static linked user libraries and dynamic linked system libraries"
 build_args=(
   --incompatible_enable_cc_toolchain_resolution
-  --platforms=@com_grail_bazel_toolchain//platforms:linux-x86_64
+  --platforms=@grail_llvm_toolchain//platforms:linux-x86_64
   --extra_toolchains=@llvm_toolchain_with_sysroot//:cc-toolchain-x86_64-linux
   --symlink_prefix=/
   --color=yes

toolchain/extensions.bzl → toolchain/extensions/llvm.bzl

@@ -1,14 +1,16 @@
-"Module extensions for use with bzlmod"
+"""LLVM extension for use with bzlmod"""
 
-load("@com_grail_bazel_toolchain//toolchain:rules.bzl", "llvm_toolchain")
+load("@grail_llvm_toolchain//toolchain:rules.bzl", "llvm_toolchain")
 load(
-    "@com_grail_bazel_toolchain//toolchain/internal:repo.bzl",
+    "@grail_llvm_toolchain//toolchain/internal:repo.bzl",
     _llvm_config_attrs = "llvm_config_attrs",
     _llvm_repo_attrs = "llvm_repo_attrs",
 )
 
 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")
         for toolchain_attr in mod.tags.toolchain:
             llvm_toolchain(
                 name = toolchain_attr.name,
@@ -18,8 +20,6 @@ def _llvm_impl_(module_ctx):
                 sha256 = toolchain_attr.sha256,
                 strip_prefix = toolchain_attr.strip_prefix,
                 urls = toolchain_attr.urls,
-                bzlmod = True,
-                bzlmod_module_version = module_ctx.modules[0].version,
             )
 
 _attrs = {

toolchain/internal/configure.bzl

@@ -40,6 +40,10 @@ load(
     _aliased_tools = "aliased_tools",
 )
 
+# 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 _include_dirs_str(rctx, key):
     dirs = rctx.attr.cxx_builtin_include_directories.get(key)
     if not dirs:
@@ -60,15 +64,17 @@ def llvm_register_toolchains():
         return
     arch = _arch(rctx)
 
-    (key, toolchain_root) = _host_os_arch_dict_value(rctx, "toolchain_roots")
+    if not rctx.attr.toolchain_roots:
+        toolchain_root = "@@%s_llvm//" % rctx.attr.name if BZLMOD_ENABLED else "@%s_llvm//" % rctx.attr.name
+    else:
+        toolchain_root = _host_os_arch_dict_value(rctx, "toolchain_roots")
+
     if not toolchain_root:
         fail("LLVM toolchain root missing for ({}, {})".format(os, arch))
-    (key, llvm_version) = _host_os_arch_dict_value(rctx, "llvm_versions")
+    (_key, llvm_version) = _host_os_arch_dict_value(rctx, "llvm_versions")
     if not llvm_version:
         fail("LLVM version string missing for ({}, {})".format(os, arch))
 
-    config_repo_path = "external/%s/" % rctx.name
-
     use_absolute_paths_llvm = rctx.attr.absolute_paths
     use_absolute_paths_sysroot = use_absolute_paths_llvm
 

toolchain/rules.bzl

@@ -36,7 +36,7 @@ toolchain = repository_rule(
     implementation = _llvm_config_impl,
 )
 
-def llvm_toolchain(name, bzlmod = False, bzlmod_module_version = None, **kwargs):
+def llvm_toolchain(name, **kwargs):
     if kwargs.get("llvm_version") == kwargs.get("llvm_versions"):
         fail("Exactly one of llvm_version or llvm_versions must be set")
 
@@ -46,17 +46,7 @@ def llvm_toolchain(name, bzlmod = False, bzlmod_module_version = None, **kwargs)
             for k, v in kwargs.items()
             if (k not in _llvm_config_attrs.keys()) or (k in _common_attrs.keys())
         }
-        llvm_repository_name = name + "_llvm"
         llvm(name = name + "_llvm", **llvm_args)
-        if bzlmod:
-            if not bzlmod_module_version:
-                bzlmod_module_version = "override"
-            kwargs.update(toolchain_roots = {"": "@@com_grail_bazel_toolchain~{module_version}~llvm~{repository_name}//".format(
-                module_version = bzlmod_module_version,
-                repository_name = llvm_repository_name,
-            )})
-        else:
-            kwargs.update(toolchain_roots = {"": "@%s_llvm//" % name})
 
     if not kwargs.get("llvm_versions"):
         kwargs.update(llvm_versions = {"": kwargs.get("llvm_version")})