Exclude convenience symlinks after changing the output base

[cameron-martin]

When the output base is changed after the convenience symlinks have been created, queries such as //... try to traverse into them and load them as packages since they are only excluded based on whether the resolved location is in the output base. This adds some heuristics to determine if a symlink is a convenience symlink:

1. The symlink name has an appropriate suffix.
2. An ancestor of the symlink target at the appropriate level is called execroot.
3. The execroot directory contains a file called DO_NOT_BUILD_HERE.

These heuristics should work if both the output base and the symlink prefix change while being quite robust to false positives.

This is important for IDE integration, where the output base is often changed for queries, to prevent concurrent builds from blocking them. An example of this is the vscode-bazel extension.

Fixes #10653
Fixes #13951

[brentleyjones]

@iancha1992 Can we get someone to review this?

[iancha1992]

@iancha1992 Can we get someone to review this?

@Wyverald @jin @meteorcloudy

[Wyverald]

@haxorz, is team-Core the right label for this? If not, do you know which team owns the convenience symlinks?

[haxorz]

@cameron-martin -

(1) Was the DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN sentinel file (https://bazel.build/run/build#specifying-build-targets) considered a as workaround for #10653 ?

(2) Is it intentional in your PR that we'd be changing the semantics for a traversal of a directory containing a dirent whose name just happens to start with the symlink prefix? (Consider bazel query //foo/... i.e. no bazel build even creating the convenience symlinks in the first place.) I think we'd want to do something a little more involved and care about the target of the link, and perhaps even the siblings of the target.

@Wyverald - team-Core vestigially owns the convenience symlinks functionality. We own the directory traversal functionality more strongly and actively.

[cameron-martin]

@haxorz Using a DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN sentinel file sounds way more robust. There is also the DO_NOT_BUILD_HERE file in the execroot that could be inspected to exclude this directory. Is it a good idea to use this file for this purpose?

[haxorz]

+cc @lberki because back in the day he was involved both the things mentioned below.

Using a DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN sentinel file sounds way more robust

Yeah.

In 1e66ccd we changed Bazel to respect the DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN sentinel file as an follow up to changing Bazel to follow directory symlinks when resolving recursive target patterns. This is not a coincidence; that behavior change had some unintended consequences internally due to legacy repo contents... so we added that feature as a workaround.

Yes, you could use that workaround. I was suggesting you could instruct your IDE users to create a DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN file in the top-level directory. This would work, right (but be annoying)?

There is also the DO_NOT_BUILD_HERE file in the execroot that could be inspected to exclude this directory. Is it a good idea to use this file for this purpose?

This would be more complicated but would be most robust because Bazel would be trying to prevent the situation in #10653 from mattering at all.

A nitpick would be to either rename that or use a new one with a different name. DO_NOT_BUILD_HERE is currently pretty narrow (see the single consumption in BlazeOptionHandler.java). So I'd recommend instead involving a new sentinel file (maybe DONT_TRAVERSE_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN, so as to have the name be self-documenting wrt DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN). You could change Bazel to create this file (as a sibling to DO_NOT_BUILD_HERE!) in the output base, and then modify the recursive target pattern traversal code to respect it.

I will preemptively say though: If you do this, the merge with Blaze internally at Google is going to require some care. DO_NOT_BUILD_HERE was introduced in 2009, and the claim back then was that there were some usages of blaze query inside the output base (you can even still see this in the Bazel source code; check out the comment for Command#canRunInOutputDirectory). So if you proceed with this approach, someone internally is going to have to set aside time to investigate this and potentially figure out a way to obsolete it.

[lberki]

I did some research internally and it looks running any Bazel command under the execroot is rare enough that it can be safely ignored (see internal thread; I can't discuss the details here) and I'll send out a change to remove it as soon as the presubmits pass.

I consider DONT_FOLLOW_SYMLINKS_WHEN_TRAVERSING_THIS_DIRECTORY_VIA_A_RECURSIVE_TARGET_PATTERN a hack so I'd rather not copy that pattern. It's Yet Another Knob we need to support forever. Instead how about using a heuristics: If

1. The symlink ends with -bin, -out -genfiles, -testlogs or -<basename of the directory it is in> (the prefix is configurable with --symlink_prefix, the suffixes are not)
2. It points directly to a directory whose ancestor of the appropriate level is called execroot
3. The execroot directory contains a file called DO_NOT_BUILD_HERE
4. OPTIONAL: If DO_NOT_BUILD_HERE contains the name of the directory being inspected although that can result in a false negative since symlinks can lead to any given directory being accessible under multiple names

that directory is not traversed?

[lberki]

Update: I found a corner case that @Wyverald added to the build of Bazel like a day ago: if you run Bazel query from a genrule AND you are not running in a sandbox AND you plop down a MODULE.bazel / WORKSPACE file in the original cwd of the genrule, you'll be using the "query can run in the output base" exception.

I'll still go ahead with the removal because it looks like it's a very special case only affecting who works on Bazel (in fact, it only showed up on our Windows presubmits), but with a little less confidence.

[cameron-martin]

@lberki I've updated the PR with the majority of the heuristics that you mentioned. However, I'm unsure of how to implement point 3, since I get the feeling from surrounding code that filesystem access should be done via skyframe rather than more directly. However I'm not too hot on how skyframe works. Could I have some pointers on how to achieve this?

I'm also unsure about what depth successor the -genfiles symlink is of the execroot, since I've never seen this before (is this just a blaze thing?).

[lberki]

Thanks for updating the pull request! I added a code snippet which I'm pretty sure works after some light editing (I didn't compile it)

[lberki]

Sorry for taking my time; this accidentally fell off my task list.

[cameron-martin]

@lberki I've addressed the latest review comments.

[lberki]

Thanks. Looks good to me now!

[fmeum]

@bazel-io flag

[meteorcloudy]

@bazel-io fork 7.1.0