Migrate from toolchains_llvm to llvm - #24832
Conversation
The nice error message reporting comes from the time when we used the host OS's clang-format. Now that we use a hermetic one, we don't need verbose failures, but we do need to fail-fast in case the glob ever becomes unsound.
|
+a:@tyler-yankee for feature review or delegation, please. |
tyler-yankee
left a comment
There was a problem hiding this comment.
Just some clarifying questions.
@tyler-yankee reviewed 5 files and all commit messages, and made 4 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri and rpoyner-tri).
MODULE.bazel line 271 at r1 (raw file):
name = "llvm", dev_dependency = True, llvm_versions = {"": "22.1.7"},
BTW if we care, it seems https://registry.bazel.build/modules/llvm/ does allow what the PR description claims to lose - under "Selecting the LLVM version":
llvm = use_extension("@llvm//extensions:llvm.bzl", "llvm")
llvm.version(llvm_version = "22.1.0")
but would this only be viable if you're actually using the whole toolchain (and not a local one)?
MODULE.bazel line 273 at r1 (raw file):
llvm_versions = {"": "22.1.7"}, )
It's not clear to me why we don't have/need(?) a register_toolchains, use_repo, etc. with llvm.
ah, because we're actually using a local toolchain, and this is only pulling in the tools that we grab elsewhere? I could buy that.
MODULE.bazel line 262 at r1 (raw file):
bazel_dep( name = "llvm", version = "0.8.9",
BTW why not 0.8.16? Bazel version incompatibility, newer clang-format (though, see other comment), something else?
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 5 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on rpoyner-tri and tyler-yankee).
MODULE.bazel line 262 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
BTW why not 0.8.16? Bazel version incompatibility, newer clang-format (though, see other comment), something else?
Ah, I didn't actually look closely, this was just the original PR text. Bumped to latest now.
MODULE.bazel line 271 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
BTW if we care, it seems https://registry.bazel.build/modules/llvm/ does allow what the PR description claims to lose - under "Selecting the LLVM version":
llvm = use_extension("@llvm//extensions:llvm.bzl", "llvm") llvm.version(llvm_version = "22.1.0")but would this only be viable if you're actually using the whole toolchain (and not a local one)?
Right. Configuring the model extension doesn't change the version of the particular @llvm//tools:... aliases we're using.
MODULE.bazel line 273 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
It's not clear to me why we don't have/need(?) a
register_toolchains,use_repo, etc. withllvm.ah, because we're actually using a local toolchain, and this is only pulling in the tools that we grab elsewhere? I could buy that.
Exactly.
Any suggestions on documentation that would have helped?
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri and rpoyner-tri).
a discussion (no related file):
Closes #24151.
Does llvm have better backing for its downloads, or? Is it still downloading and extracting the whole LLVM project?
MODULE.bazel line 273 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Exactly.
Any suggestions on documentation that would have helped?
Looking up we already have
# Load developer dependencies that are modules.
for this section, which I had originally missed, and certainly doesn't hurt.
WDYT?
# Obtain LLVM for its development tools only. We don't register its toolchain
# because our local C/C++ toolchain is already configured above.
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),tyler-yankee, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on rpoyner-tri and tyler-yankee).
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Closes #24151.
Does
llvmhave better backing for its downloads, or? Is it still downloading and extracting the whole LLVM project?
The downloads are smaller, but are still served single-source: https://github.com/hermeticbuild/hermetic-llvm/releases/tag/llvm-22.1.7-2. I figured it closes the issue because at least if nothing else the failure mode (/ error message) is going to be different, as well as any approach we'd use to fix the code.
MODULE.bazel line 273 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Looking up we already have
# Load developer dependencies that are modules.for this section, which I had originally missed, and certainly doesn't hurt.
WDYT?
# Obtain LLVM for its development tools only. We don't register its toolchain # because our local C/C++ toolchain is already configured above.
Done.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on rpoyner-tri).
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on jwnimmer-tri).
rpoyner-tri
left a comment
There was a problem hiding this comment.
+(status: squashing now)
@rpoyner-tri made 1 comment.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),tyler-yankee (waiting on jwnimmer-tri).
Extracted from #24810.
Closes #24151.
This no longer lets us independently choose the
clang-formatversion from thebazel_depmodule version, but that should be fine.This change is