From 2857aa751043e61ca7a4aa03a397a889aa0ebbcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20F=C3=A9ron?= Date: Fri, 8 Sep 2023 12:40:02 +0200 Subject: [PATCH] Apply review comments --- MODULE.bazel | 7 +++---- README.md | 12 ++++++------ tests/BUILD.bazel | 2 +- tests/MODULE.bazel | 13 ++++++------- tests/WORKSPACE | 4 ++-- tests/openssl/openssl.bazel | 4 ++-- tests/scripts/run_xcompile_tests.sh | 2 +- toolchain/extensions/BUILD.bazel | 0 toolchain/{extensions.bzl => extensions/llvm.bzl} | 10 +++++----- toolchain/internal/configure.bzl | 14 ++++++++++---- toolchain/rules.bzl | 12 +----------- 11 files changed, 37 insertions(+), 43 deletions(-) create mode 100644 toolchain/extensions/BUILD.bazel rename toolchain/{extensions.bzl => extensions/llvm.bzl} (79%) diff --git a/MODULE.bazel b/MODULE.bazel index fd648a44..85f86083 100644 --- a/MODULE.bazel +++ b/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") diff --git a/README.md b/README.md index 269c3f2a..17ca7599 100644 --- a/README.md +++ b/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 \ //... ``` diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel index 9380f30c..0664ed38 100644 --- a/tests/BUILD.bazel +++ b/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", diff --git a/tests/MODULE.bazel b/tests/MODULE.bazel index c6c78dc6..6303062c 100644 --- a/tests/MODULE.bazel +++ b/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 diff --git a/tests/WORKSPACE b/tests/WORKSPACE index eb298161..a3987be4 100644 --- a/tests/WORKSPACE +++ b/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 diff --git a/tests/openssl/openssl.bazel b/tests/openssl/openssl.bazel index 34795609..12aee585 100644 --- a/tests/openssl/openssl.bazel +++ b/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"], diff --git a/tests/scripts/run_xcompile_tests.sh b/tests/scripts/run_xcompile_tests.sh index d734caab..58aa869e 100755 --- a/tests/scripts/run_xcompile_tests.sh +++ b/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 diff --git a/toolchain/extensions/BUILD.bazel b/toolchain/extensions/BUILD.bazel new file mode 100644 index 00000000..e69de29b diff --git a/toolchain/extensions.bzl b/toolchain/extensions/llvm.bzl similarity index 79% rename from toolchain/extensions.bzl rename to toolchain/extensions/llvm.bzl index 1b7796e0..36524cdb 100644 --- a/toolchain/extensions.bzl +++ b/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 = { diff --git a/toolchain/internal/configure.bzl b/toolchain/internal/configure.bzl index eaea5a1f..cbce5fae 100644 --- a/toolchain/internal/configure.bzl +++ b/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 diff --git a/toolchain/rules.bzl b/toolchain/rules.bzl index e0c46888..2a34934f 100644 --- a/toolchain/rules.bzl +++ b/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")})