Skip to content
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

bazel: add compile commands generator #24596

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Dec 17, 2024

Replace the hedron compilation database generator with a homegrown one. It's much simpler (the hedron thing is much more complex than what we want due to adding entries for headers as well as platforms/compilers we don't care about), and works much better for our setup.

The two main changes from the hedron thing is that we add -iquote src/v as the first header search path to make sure we resolve headers not in the sandbox, as well as remapping compiler paths to be something that works with clangd.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.3.x
  • v24.2.x
  • v24.1.x

Release Notes

  • none

This will make generating a compilation database much easier with
pre-defined shortcuts.
@travisdowns
Copy link
Member

but requires local
clang and recent enough libstdc++ to have the same symbols as the actual
build that uses libc++.

Any details on why? This part seems like it will be an ongoing PITA? Is it a limitation of the underlying hedron thing?

@rockwotj
Copy link
Contributor Author

It's a bad interaction between our custom toolchain and hedron thing. I can fix it with post-processing the compilation_commands.json file, or forking the hedron thing... I'll look into fixing this later, this at least makes the json file a reasonable size (65MiB instead of 900MiB) and the stdlib headers works for me on Ubuntu Noble.

The Hedron compile commands generator does this automatically, but it's
better to be explicit, especially if we fork the tool.
@rockwotj rockwotj requested a review from a team as a code owner December 18, 2024 22:17
@rockwotj rockwotj requested review from clee and removed request for a team December 18, 2024 22:17
This matches the root include we do in cmake and helps prevent jumping
into symlinks from the sandbox due to all the virtual includes we have.
This also prevents accidently adding `src/v` versions of various headers
when auto adding headers using clangd.

Additionally rewrite the args copy loop to be a single pass.
For toolchains llvm, the compiler is a shell script that just points to
the actual directory containing the LLVM toolchain. clangd uses the
directory of the first arg (assumed to be the real compiler, not a shell
wrapper) for search paths of stdlib headers. If we remap that then
clangd can find libc++ headers and we can then resolve the headers for
our Bazel provided compiler 🎉
@rockwotj
Copy link
Contributor Author

I pushed two commits @travisdowns:

  • The first one is to prevent from jumping to a header in the sandbox due to our usage of virtual includes. It also prevents accidentally autoincluding a header relative to the repo root (like #include "src/v/serde/parquet/value.h", I've done this and we're prone too in the way Bazel does includes and relies on the sandbox to enforce files actually exist).
  • The second is to remap the compiler path to be in a location that works better with clangd. The remapped compiler path allows clangd to find the libc++ headers for our built in toolchain.

With both of these changes I get a pretty darn good clangd experience IMO, and no red squiggles from my initial tests 🎉

@rockwotj rockwotj requested a review from travisdowns December 19, 2024 10:58
We've now replaced this with our own homegrown solution that is
much simpler and works much better for us.
@rockwotj rockwotj changed the title bazel: add compile commands shortcuts bazel: add compile commands generator Dec 19, 2024
Add seastar to the list of sources we support compiling, and also remove
the list of tool compilation commands from the json file, those result
in duplicates.
@rockwotj
Copy link
Contributor Author

I will also update the Wiki if we like this and merge it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants