Point _virtual_includes to stable locations so IDE integrations survive builds

[cpsauer]

Hi wonderful Bazelers,

This minor change makes it possible for IDE integrations to play nicely with _virtual_includes.

Prior to this change, _virtual_include symlinks, themselves in stable, accumulating locations in bazel-out, were pointing into unstable locations in execroot and execroot/external, often invalidated by the next build. This leads to spurious, confusing error messages for the user when subsequent builds made headers no longer findable. Sad times. But with this change, the symlinks stay valid, since they point into the workspace and the accumulating caches in <output_base>/external.

Before we tracked it down, we received large numbers of issue reports about this over at https://github.com/hedronvision/bazel-compile-commands-extractor, a rather popular tool that enables autocomplete for the C language family across editors and platforms. I strongly suspect it solves similar issues in other IDE adapters, whether they've been traced back to this yet or not. FWIW, I'd previously fixed similar issues in the internal symlinks and include paths of the bazelbuild IntelliJ plugin and Tulsi, back in the day.

Thanks so much,
Chris
(ex-Googler)

[eguiraud]

Hi, I'm working on a medium-sized Python+C++ repo (~70k LOC) which is affected by the issue this patch fixes.

I just tried out this patch on top of the latest bazel trunk HEAD and I can confirm it fixes the issue (and it does not seem to cause any problem as far as a full bazel test can tell).

[cpsauer]

Thanks so much for double checking and reporting back, Enrico!

[MartinSkold]

This would unlock a lot of nice IDE features in my company's multiplatform bazel monorepo. Would love to have it in :)

[brtdylan]

We use the awesome compile command extractor tool for IDE/clangd integration with our C++/Bazel codebase and are seeing this issue with the clobbered dirs in the symlink path as well! Will try to give this patch a shot with our repo and report back, but I believe this will resolve the issue for us too

[aminya]

This allows the use of the compile-commands extractor without needing to rebuild via Bazel, which saves a lot of time.

[comius]

I'd love to accept this change. Then we could even drop the extra parameter from ctx.actions.symlink, becuase it's used only here.

Unfortunately it supports internal functionality, which I don't have a clue how to support.
cc @justinhorvitz (cl/446709489 for reference).

[justinhorvitz]

I'd love to accept this change. Then we could even drop the extra parameter from ctx.actions.symlink, becuase it's used only here.

Unfortunately it supports internal functionality, which I don't have a clue how to support. cc @justinhorvitz (cl/446709489 for reference).

This parameter was added by an ex-Googler, so it may take me some time to get up to speed on exactly what's going on. My basic understanding is that it's necessary because we use immutable snapshots as "source" and that applying this PR will result in not picking up changes to the symlink target, since it will point to an immutable snapshot.

[justinhorvitz]

@comius do we have a switch to make this parameter True for blaze and False for bazel? If not, I guess we could introduce a flag. It would need to be default True to start with, but I could help to get the default changed to False if necessary.

[cpsauer]

Thanks so much for engaging, Ivo and Justin. We really appreciate it.

(Some quick notes:)
(Wish I could see the CL! LMK if there's anything more I should know about those blaze-only immutable snapshots.)
(I'd seen the parameter was only used here (and the commit adding), but figured I'd put up the simplest initial PR, with a self-note to return to the potentially dead parameter later, thinking it might have other/internal uses. Looks like that was the right bet.)
(Your guy's fallback tiers sound great to me: see if there's a way to not need it internally; followed by conditionioning on blaze, bifurcating behavior but only minorly, while keeping the right defaults for everyone; and then a flag if we really have to, but that seems worse.)

[jmbalasalle]

Seems like there's some good progress on this one - thank you. We'd also love to have this fix, definitely helps our process and increases efficiency!

[justinhorvitz]

I put together a change that diverges this boolean between bazel and blaze. External review URL: https://bazel-review.googlesource.com/c/bazel/+/238113.

It would be nice to have a test that fails if this is set to True for bazel. @cpsauer any chance you could put together such a test case? If so, I'll merge it into my commit.

[cpsauer]

Thanks so much for doing, Justin.

[cpsauer]

Test: With you in spirit, but I'm not sure I have comparative advantage; I'm not sure where you'd want it to go or how to format. Probably faster for you guys to just do if you want? The behavior to check is that _virtual_includes symlinks in the main workspace end up pointing into the main workspace itself rather than execroot, and for external workspaces, <output_base>/external rather than /external

[justinhorvitz]

I was hoping for a test that illustrates what's going wrong from the IDE's perspective, since I don't really understand that myself. In other words, it would verify that the error messages described above go away, not just that the symlinks point somewhere.

I'm going to go ahead and submit the change with a TODO about a test, although I'm not planning on writing one myself.

[brentleyjones]

@bazel-io flag

[keertk]

@bazel-io fork 7.1.0

[iancha1992]

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

[brt-ivanpauno]

Could this same fix be added in a future 6.x minor or patch release?
IIUC, it should be applied here instead

[iancha1992]

Could this same fix be added in a future 6.x minor or patch release? IIUC, it should be applied here instead

cc: @Wyverald, @meteorcloudy

[meteorcloudy]

@brt-ivanpauno We'll only create patch releases for Bazel 6 for critical security issues, currently this is not planned.

[brt-ivanpauno]

@brt-ivanpauno We'll only create patch releases for Bazel 6 for critical security issues, currently this is not planned.

Okay, that makes sense.
Thanks for the reply @meteorcloudy and thanks for fixing this @cpsauer !!