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

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

Closed
aminya opened this issue Sep 20, 2023 · 11 comments

Comments

@aminya
Copy link

aminya commented Sep 20, 2023

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
Copy link
Contributor

cpsauer commented Nov 14, 2023

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
Copy link
Contributor

garymm commented Nov 20, 2023

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.

oliverlee pushed a commit to garymm/starflate that referenced this issue Nov 20, 2023
work around hedronvision/bazel-compile-commands-extractor#140

Change-Id: I86a23ad7ffc699773a9219a3c766e22df0946479
garymm added a commit to garymm/starflate that referenced this issue Nov 20, 2023
work around hedronvision/bazel-compile-commands-extractor#140

Change-Id: I86a23ad7ffc699773a9219a3c766e22df0946479
@eguiraud
Copy link

eguiraud commented Dec 8, 2023

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
Copy link
Contributor

cpsauer commented Dec 14, 2023

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
Copy link
Contributor

cpsauer commented Dec 14, 2023

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
Copy link
Contributor

cpsauer commented Dec 14, 2023

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
Copy link
Contributor

cpsauer commented Dec 14, 2023

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

@cpsauer
Copy link
Contributor

cpsauer commented Dec 14, 2023

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

@cpsauer
Copy link
Contributor

cpsauer commented Dec 14, 2023

I put up a PR over at bazelbuild/bazel#20540 that should fix.

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

@cpsauer
Copy link
Contributor

cpsauer commented Jan 20, 2024

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!

@cpsauer cpsauer closed this as completed Jan 20, 2024
@brt-ivanpauno
Copy link

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

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

No branches or pull requests

5 participants