Skip to content

[WIP] bazel: Add LLVM toolchain #29415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

phlax
Copy link
Member

@phlax phlax commented Sep 4, 2023

Ideally we use this toolchain for compiling and can then remove the compiler from our container platform/toolchains

More immediately, adding this means we can use the clang tools (clang-format, clang-tidy, llvm-cov, etc) with bazel - this remove the host requirement, and should prevent issues around running tooling with incorrect versions (in combination with #29397 and some further dev)

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 4, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #29415 was opened by phlax.

see: more, trace.

@phlax phlax assigned keith and unassigned moderation Sep 4, 2023
@phlax phlax changed the title bazel: Add LLVM toolchain [WIP] bazel: Add LLVM toolchain Sep 4, 2023
@phlax phlax marked this pull request as draft September 4, 2023 18:20
@phlax
Copy link
Member Author

phlax commented Sep 4, 2023

@keith i had to set the version to 14.0.0 (our current) as there is not a toolchain for ubuntu < 22 - but that should be ok for now

i also had to shift some of the setup around to get it working, not exactly ideally

@phlax phlax force-pushed the llvm-toolchain branch 4 times, most recently from f20ff24 to 2e5aed5 Compare September 5, 2023 17:05
@phlax
Copy link
Member Author

phlax commented Sep 6, 2023

hmm - so the problem with this plan is that the llvm repo/tarball still moves around as a 4GB block where its invoked locally

for the most part CI can run without invoking any of the bins locally, but if eg you try to use the (100MB) clang-tidy or clang-format bins it pulls the full 4GB - this makes linting have a much bigger overhead than compiling

in the cases of clang-tidy/format specifically there are python packages that you can use to just pull the bins and nothing else

another problem is that the binary versions dont seem to be well maintained - checking from llvm release to the next there are not the same set of assets. Fwiw the python (invoked) bins mentioned above suffer from similar issue with sporadic releases

The version problem is already there - whatever distribution method we use we are relying on prebuilt binaries that work on ubuntu 20.04 - the current version may be the last one that these are built for

cc @keith

@keith
Copy link
Member

keith commented Sep 6, 2023

there are some llvm threads about improving the release process for these binaries so I'm optimistic that part will get solved in the future. not sure what to say about the binary size problem though. That was always something we were going to have to take the hit on if we moved to this. If this was the default then we could probably stomach doing that download once since you would keep the binaries locally until they were upgraded again anyways

@phlax
Copy link
Member Author

phlax commented Sep 6, 2023

That was always something we were going to have to take the hit on if we moved to this.

one consequence is that we cant use this without removing from the container image otherwise ci downoads 4G x2

If this was the default then we could probably stomach doing that download once since you would keep the binaries locally until they were upgraded again anyways

its more about CI and limiting what an RBE client needs to download on a run (esp where there is no change)

for the compiler i agree we just need to take the hit

for clang tools im not so sure - for now we can hook up the python way as it ~matches our current version

upgrading tho we will then need an upstream to be available for both the full compiler toolchain and any tooling

as we are in this situation anyway (no current upgrade path) im happy to land this and start using the python distributed tools

its gonna be a bit fragile but not much worse than current

the only other blocker i hit was that i got a compiler error when trying to use for compile toolchain - ill re-enable it so we can see

@phlax
Copy link
Member Author

phlax commented Sep 6, 2023

#29455

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 11, 2023
@phlax phlax added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Oct 11, 2023
@phlax
Copy link
Member Author

phlax commented Mar 11, 2024

this is probably worth another try

the llvm_toolchain has moved to bazel-contrib

there is also a potential gcc toolchain here https://github.com/f0rmiga/gcc-toolchain

@mathetake
Copy link
Member

I like this - this also will make upgrades of LLVM much easier

@phlax phlax force-pushed the llvm-toolchain branch 2 times, most recently from 4a8d3a1 to db494a7 Compare January 20, 2025 13:10
Signed-off-by: Keith Smiley <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants