hedronvision / bazel-compile-commands-extractor

Goal: Enable awesome tooling for Bazel users of the C language family.
Other
697 stars 115 forks source link

Using `refresh_compile_commands` macro overwrites `bazel-bin` directory resulting in invalid includes #140

Closed aminya closed 10 months ago

aminya commented 1 year ago

I am using the refresh_compile_commands macro as specified in the readme.

However, when I run bazel run refresh_compile_commands, the bazel-bin directory content is replaced with refresh_compile commands Python files. This breaks clangd as some includes are no longer in the path they are expected.

To fix this, I have to rerun the bazel build command once more.

image
cpsauer commented 1 year ago

Hey Amin! My understanding is that all paths in the compilation database should be in-workspace or going through bazel-out (since that's what's in the execution sandbox) and that bazel-bin is just a convenience symlink in the workspace that gets reset to the output of the last build and is not depended upon by this tool. Are you seeing otherwise, with bazel-bin entries in your compile_commands.json, or is there a chance something else is at play here?

garymm commented 1 year ago

I'm not sure exactly why, but I'm seeing something similar. Running bazel run @hedron_compile_commands//:refresh_all results in clangd not finding includes. Running bazel build //... afterwards fixes it.

eguiraud commented 11 months ago

Hi, I have the same problem.

I think the directory that is actually overwritten is $BAZEL_CACHE/_bazel_$USER/$HASH/execroot/$WORKSPACE_NAME/external. More details below.

In my case the missing includes come from Google benchmark. At bazel build time, Google benchmark is picked up via:

-Ibazel-out/k8-fastbuild/bin/external/googlebenchmark/_virtual_includes/benchmark

and that flag is correctly stored in the compile_commands.json.

However, the way Bazel does things is that the actual benchmark/benchmark.h header is a symlink to:

/home/$USER/.cache/bazel/_bazel_$USER/$HAS/execroot/$WORKSPACE_NAME/external/googlebenchmark/include/benchmark/benchmark.h

...and running bazel run @hedron_compile_commands//:refresh_all completely overwrites the contents of .../execroot/$WORKSPACE_NAME/external, effectively breaking the include path for clangd, clang-tidy, etc.

cpsauer commented 11 months ago

Oh. Oh! Great analysis, Enrico. I understand what's going on and have seen (& fixed) a variant of it before.

We're going to need a Bazel change here. Without it, I don't think there's a good way to handle this in any of the bazel IDE adapters, but the good news is that it'll fix things across all of 'em.

What we want changed here is for the _virtual_includes symlinks to point not into <output_base>/execroot/<workspace_name>/external but instead into <output_base>/external.

The former is unstable, reset on every build to just the external dependencies used in that build. Pointing into it is a tempting trap...that broke literally all of Bazel's official editor plugins before we helped fix them. That's why our external symlink points into the other, which is where external workspaces are cached and accumulated. More in ImplemenationReadme for anyone interested. If we change Bazel's symlinking logic to point into the latter, then the symlinks should stay valid, fixing this issue.

cpsauer commented 11 months ago

Enrico, would you be down to file a Bazel PR fixing (or at least an issue)? I'm underwater at the moment, so I'm hoping to recruit from the folks benefitting, doubly since you have a test case ready to go.

cpsauer commented 11 months ago

The code change would change the actions.symlink in https://github.com/bazelbuild/bazel/blob/master/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl

I think, additionally there's a case that needs to be handled here for user code, where <output_base>/execroot/<workspace_name>/<top_level_package> is also unstable, and we'd want to symlink directly to the user's code.

One solution might be to try to just resolve the destination of the symlink before creating it, handling both cases directly. They're already absolute paths, so I don't think there's harm there.

cpsauer commented 11 months ago

heh, wait, could it be as easy as removing use_exec_root_for_source = True, in their call?

cpsauer commented 11 months ago

Yep. That's it. I'm on it. Unreal.

cpsauer commented 11 months ago

I put up a PR over at https://github.com/bazelbuild/bazel/pull/20540 that should fix.

Any amount you'd like to chime in over there to encourage merging would be much appreciated :)

cpsauer commented 10 months ago

The fix has made it into the bazelbuild mainline!

Thanks all for helping track this down and get it in--should be released as part of Bazel 7.1 and the next rolling release!

brt-ivanpauno commented 8 months ago

IIUC, this is still an issue in bazel 6.x. I have asked upstream if a backport is possible.