Add support for bzlmod #198

gferon · 2023-05-15T14:42:55Z

This is a fairly straightforward change that adds support for using bazel-toolchain with bzlmod. We've been using this patch on a rather large checkout with success.

The llvm.toolchain extension is only a wrapper around the existing llvm_toolchain repository rule and supports the same attributes.

For trying it out, add the following to your MODULE.bazel:

bazel_dep(name = "com_grail_bazel_toolchain", version = "0.8")
git_override(module_name = "com_grail_bazel_toolchain", remote = "https://github.com/grailbio/bazel-toolchain", commit = "cf915e5c7cfcd19a3e71de54e385e01240b865dc")

llvm = use_extension("@com_grail_bazel_toolchain//toolchain:extensions.bzl", "llvm")
llvm.toolchain(
    llvm_version = "15.0.0"
)

use_repo(llvm, "llvm_toolchain")

register_toolchains("@llvm_toolchain//:all")

siddharthab

Thank you so much for this. Unsure about how version strings work in this, but otherwise all good.

MODULE.bazel

README.md

toolchain/extensions.bzl

MODULE.bazel

toolchain/rules.bzl

gferon · 2023-05-15T15:56:43Z

I would also be more comfortable if we immediately add CI to verify that it works as intended.

siddharthab · 2023-05-15T16:32:45Z

Thank you so much @jsharpe for reviewing this. I have not been active with Bazel for a few years now and am not caught up with modules. I will leave it to you to merge this.

toolchain/extensions.bzl

gferon · 2023-05-16T08:49:31Z

@jsharpe thanks for the thorough review, is there a way you can approve GitHub workflows for this PR so that I can iterate on the tests until everything passes?

EDIT: maybe you approved, it's running now

jsharpe · 2023-05-16T08:52:59Z

@jsharpe thanks for the thorough review, is there a way you can approve GitHub workflows for this PR so that I can iterate on the tests until everything passes?

I don't think there is unfortunately. I think its a limitation as this is your first PR against this repo. We could get this merged and use a subsquent PR to setup CI and then I don't think we'd have the same limitation?

gferon · 2023-05-16T08:54:24Z

We could get this merged and use a subsquent PR

Sounds good, I know there are more adjustments to be made, for example all flags aren't forwarded to llvm_toolchain and CI will need adjusting. I'm definitely up to finish adding support for bzlmod.

gferon · 2023-05-16T12:28:01Z

@jsharpe I excluded Bazel 5.1.0 from the CI job matrix with bzlmod but I think a better solution would be to bump to 5.4.0 which supports the --enable_bzlmod flag. Otherwise, I believe this is ready to merge and improve upon.

jsharpe · 2023-05-16T14:34:23Z

FYI, When i've had similar failures to those in CI its been due to redefinitions of the platforms repo in the WORKSPACE.bzlmod file

fmeum · 2023-07-11T15:16:34Z

Let me know if you want me to look into particular failures.

https://github.com/grailbio/bazel-toolchain/actions/runs/5361378105/jobs/9727258799?pr=198 is difficult to migrate as is: The abseil-cpp BCR module only defines the deps needed to use it, not to test it. This may require adding it as an http_archive in the same extension as com_google_googletest instead of using the BCR module.

.github/workflows/tests.yml

tests/WORKSPACE

jsharpe · 2023-09-08T15:27:36Z

tests/openssl/openssl.bazel

+    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)


This name will need to be conditionally selected based on whether BZLMOD is enabled.

Actually, how about renaming the original name of the repository to also match the new one with bzlmod, it's not really a breaking change IMO. EDIT: Using repo_name for tests is also an easier option.

jsharpe · 2023-09-08T15:29:27Z

Something isn't quite right with the external_test as it seems to be running the non-bzlmod version for the bzlmod testcase.

Otherwise the changes look good to me - we're soo close to getting this landed.

gferon · 2023-09-08T15:53:50Z

Something isn't quite right with the external_test as it seems to be running the non-bzlmod version for the bzlmod testcase.

I switched from --experimental_enable_bzlmod to --enable_bzlmod in the external tests shell script and it looks like it works now (tested locally).

jsharpe · 2023-09-08T16:21:43Z

Something isn't quite right with the external_test as it seems to be running the non-bzlmod version for the bzlmod testcase.

I switched from --experimental_enable_bzlmod to --enable_bzlmod in the external tests shell script and it looks like it works now (tested locally).

That won’t work for the 5.1.0 tests..

siddharthab · 2023-09-08T16:31:31Z

A recent change has upped the minimum supported version to 6.0.0 from 5.1.0, if it helps.

siddharthab · 2023-09-08T16:51:51Z

MODULE.bazel

@@ -0,0 +1,9 @@
+module(
+    name = "grail_llvm_toolchain",


I think it will be OK to call it simply "llvm_toolchain". I am trying to get this project moved to the bazel-contrib repo, where the simpler name would make more sense.

Done! I would love to see this moved to bazel-contrib.

gferon · 2023-09-11T08:11:35Z

So, in theory I have fixed (after breaking them) the last few CI issues, except https://github.com/grailbio/bazel-toolchain/actions/runs/6124327795/job/16624119392 that I haven't taken enough time to understand.

EDIT: I excluded one more rules_go test for the time being, it should work now!

siddharthab

Looks very good. Just some final nits.

tests/scripts/run_external_tests.sh

toolchain/internal/configure.bzl

tests/scripts/run_external_tests.sh

siddharthab · 2023-09-12T16:44:54Z

Let me merge this for you now, and I can iterate on the failing tests on my side.

siddharthab · 2023-09-12T16:46:13Z

Thank you so much for all your hard work on this.

siddharthab approved these changes May 15, 2023

View reviewed changes

MODULE.bazel Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

jsharpe reviewed May 15, 2023

View reviewed changes

toolchain/extensions.bzl Outdated Show resolved Hide resolved

gferon commented May 15, 2023

View reviewed changes

MODULE.bazel Outdated Show resolved Hide resolved

jsharpe reviewed May 15, 2023

View reviewed changes

toolchain/rules.bzl Outdated Show resolved Hide resolved

jsharpe reviewed May 15, 2023

View reviewed changes

toolchain/rules.bzl Outdated Show resolved Hide resolved

gferon requested a review from jsharpe May 15, 2023 16:22

gferon force-pushed the bzlmod branch from b190f86 to 18d7095 Compare May 15, 2023 20:32

gferon marked this pull request as draft May 15, 2023 21:36

jsharpe reviewed May 16, 2023

View reviewed changes

toolchain/extensions.bzl Outdated Show resolved Hide resolved

gferon changed the title ~~Add support for bzlmod~~ Add initial support for bzlmod May 16, 2023

gferon marked this pull request as ready for review May 16, 2023 08:54

gferon force-pushed the bzlmod branch from cfb0c26 to 074082e Compare May 16, 2023 12:30

Add initial support for bzlmod

cf915e5

gferon force-pushed the bzlmod branch from 074082e to cf915e5 Compare May 16, 2023 12:31

jsharpe added 7 commits June 24, 2023 00:04

Attempting to fix CI

19f9223

Merge commit 'c65ef7a45907016a754e5bf5bfabac76eb702fd3' into HEAD

34be57c

Remove googletest as its not needed as a direct dep

4b95739

Switch to bzlmod for rules_rust

5bd2421

Fix debian test

093bf96

Add missing rules_cc dep

e30d76f

Add missing dep on platforms

139cd28

Mark bzlmod tests as experimental

92a1d84

gferon requested a review from siddharthab September 8, 2023 13:12

fmeum approved these changes Sep 8, 2023

View reviewed changes

.github/workflows/tests.yml Outdated Show resolved Hide resolved

Fix committed merge conflict

b4fe5d8

jsharpe reviewed Sep 8, 2023

View reviewed changes

tests/WORKSPACE Outdated Show resolved Hide resolved

jsharpe reviewed Sep 8, 2023

View reviewed changes

Fix replace all gone wrong

026e147

gferon added 2 commits September 8, 2023 17:55

Remove unused matrix keys in CI job

ab72225

Remove leftover typo

9d8767a

Use true/false in GH Actions to toggle bzlmod

f0cbaea

siddharthab reviewed Sep 8, 2023

View reviewed changes

gferon added 4 commits September 11, 2023 09:55

Fix one more wrongly replaced occurence

7b03f9e

Remove useless CI job (since support is now 6.x+)

ffc0ff3

Rename module to llvm_toolchain

9eb4810

Fix wrong return type

0ca0e17

Exclude one more rules_go test

6a0e0f3

gferon requested review from fmeum, siddharthab and jsharpe September 11, 2023 08:19

gferon changed the title ~~Add initial support for bzlmod~~ Add support for bzlmod Sep 11, 2023

siddharthab approved these changes Sep 11, 2023

View reviewed changes

tests/scripts/run_external_tests.sh Show resolved Hide resolved

toolchain/internal/configure.bzl Show resolved Hide resolved

tests/scripts/run_external_tests.sh Outdated Show resolved Hide resolved

Fix rules_go tests with bzlmod

377fbb4

siddharthab merged commit 5c6f806 into bazel-contrib:master Sep 12, 2023
23 of 27 checks passed

gferon deleted the bzlmod branch September 12, 2023 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for bzlmod #198

Add support for bzlmod #198

gferon commented May 15, 2023 •

edited

Loading

siddharthab left a comment

gferon commented May 15, 2023

siddharthab commented May 15, 2023

gferon commented May 16, 2023 •

edited

Loading

jsharpe commented May 16, 2023

gferon commented May 16, 2023 •

edited

Loading

gferon commented May 16, 2023 •

edited

Loading

jsharpe commented May 16, 2023

fmeum commented Jul 11, 2023

jsharpe Sep 8, 2023

gferon Sep 8, 2023 •

edited

Loading

jsharpe commented Sep 8, 2023

gferon commented Sep 8, 2023

jsharpe commented Sep 8, 2023

siddharthab commented Sep 8, 2023

siddharthab Sep 8, 2023

gferon Sep 11, 2023 •

edited

Loading

gferon commented Sep 11, 2023 •

edited

Loading

siddharthab left a comment

siddharthab commented Sep 12, 2023

siddharthab commented Sep 12, 2023

Add support for bzlmod #198

Add support for bzlmod #198

Conversation

gferon commented May 15, 2023 • edited Loading

siddharthab left a comment

Choose a reason for hiding this comment

gferon commented May 15, 2023

siddharthab commented May 15, 2023

gferon commented May 16, 2023 • edited Loading

jsharpe commented May 16, 2023

gferon commented May 16, 2023 • edited Loading

gferon commented May 16, 2023 • edited Loading

jsharpe commented May 16, 2023

fmeum commented Jul 11, 2023

jsharpe Sep 8, 2023

Choose a reason for hiding this comment

gferon Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

jsharpe commented Sep 8, 2023

gferon commented Sep 8, 2023

jsharpe commented Sep 8, 2023

siddharthab commented Sep 8, 2023

siddharthab Sep 8, 2023

Choose a reason for hiding this comment

gferon Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

gferon commented Sep 11, 2023 • edited Loading

siddharthab left a comment

Choose a reason for hiding this comment

siddharthab commented Sep 12, 2023

siddharthab commented Sep 12, 2023

gferon commented May 15, 2023 •

edited

Loading

gferon commented May 16, 2023 •

edited

Loading

gferon commented May 16, 2023 •

edited

Loading

gferon commented May 16, 2023 •

edited

Loading

gferon Sep 8, 2023 •

edited

Loading

gferon Sep 11, 2023 •

edited

Loading

gferon commented Sep 11, 2023 •

edited

Loading