Update and extend clang20 #203
Conversation
martin4861
force-pushed
the
martin4861/extend-clang20
branch
from
58b1f79 to
17d66c3
Compare
RReichert
left a comment
RReichert
left a comment
There was a problem hiding this comment.
I've put some general comments, but I'm not a good bazel person, so I leave it to others to have better reviews.
|
|
||
| constraint_setting( | ||
| name = "toolchain", | ||
| default_constraint_value = ":llvm_toolchain", |
There was a problem hiding this comment.
this might be a bit scary.
mostly because merging will mean that any repository that upgrades its rules_swiftnav file will automatically get pushed to using the latest compiler, right?
if that is the case, we will need to check in with other teams, namely IOT and Integrity. I would go with the slow transition to using clang-20 with Orion then slowly move down each repository till we get to starling.
There was a problem hiding this comment.
You're right. Even though I tested it in many repos, a slower migration is needed due to some requirements, where we need to stick on clang-14 for the moment.
| "llvm-cov": "wrappers/llvm-cov", | ||
| "llvm-profdata": "wrappers/llvm-profdata", | ||
| "ld": "wrappers/ld.ldd", | ||
| "ld": "wrappers/ld.ldd", # This should be ld.lld? |
There was a problem hiding this comment.
This should be ld.lld?
I question if it should be ld.lld since its not the operating system's default linker, but that discussion might lead to other issues, maybe this is okay for now since all the tools are coming from LLVM project
There was a problem hiding this comment.
I think, it should be ld.lld from the llvm project, because ld.ldd could be a typo. I think, that ld is not used, because the toolchain always uses clang for compiling and linking (which then calls the linker). I might revisit this in a follow up PR.
| }), | ||
| host_system_name = X86_64_LINUX, | ||
| target_cpu = "k8", | ||
| target_cpu = "k8", # this should be aarch64? |
There was a problem hiding this comment.
this should be aarch64?
Wouldk8be a placeholder for kubernetes often abbreviated to k8s? This is indeed strange
There was a problem hiding this comment.
The naming is unfortunate. k8 means amd64, whereas k8s stands for kubernetes. Thats for clang14, it is working as expected currently. Therefore I keep it as it is for now, but maybe I revisit it in a follow up PR.
martin4861
force-pushed
the
martin4861/extend-clang20
branch
from
073acb2 to
5f84cac
Compare
reimerix
left a comment
reimerix
left a comment
There was a problem hiding this comment.
With limited knowledge about what we're doing here: LGTM!
clang20toolchains forgraviton2andgraviton3