Ignore symlinks into Bazel output rather than relying on name

[cpsauer]

Hi wonderful Bazelers,

This PR is somewhere between a bug fix and a feature. It generalizes existing functionality that is missing some cases.

It's focused on the walk package's auto-ignoring of symlinks.

Right now, Gazelle will happily traverse symlinks into bazel's output base, so long as they don't start with bazel-*. This is a problem because IDE adapters often create such symlinks. I know Tulsi does, for example, and a user just reported this issue over at a tool I maintain. Since I could see where the issue was, I figured I'd take a quick shot at proposing a fix, despite not currently being a Gazelle or go user. (So please have patience with me :) It seemed worth fixing the default since Gazelle could write a considerable number of files before the user realized what was going on.

This PR also makes progress towards fixing #168, removing the dependence on a bazel-* prefix for the ignoring convenience symlinks, instead ignoring a more general class. However, a full fix to #168 will require more work and wasn't the intent; this is an incremental, separate improvement, just written in such a way as to set the stage for future improvements there. I'll save state into that issue, but note that the output_base resolving code will likely be swapped out then.

I should also note: The deleted comment in repo.go is intentional. The current behavior of --symlink_prefix has changed to also rename bazel-out. I'm also skeptical that the neighboring, similar, symlink resolution code will work on windows; hence the use of Dir in my implementation. In my (python) experience, .. traverses back out of a Windows junction the way it came, as opposed to getting the parent of the symlinks destination, as on posix. But this problematic code will get removed when #168 gets fixed some day.
One final note: The reordering of the default behavior behind the user override is also intentional. It's more consistent with the other automatic defaults and gives the user an escape hatch if they really want it.

Thanks for reading!
Chris
(ex-Googler)

Edit: Now fixes #168

[cpsauer]

Tagging @jayconrod, since it looks like he last reviewed changes to this file. Hope that's ok.

[cpsauer]

@fmeum or @linzhp, could I ask for you to review/approve this one, since it looks like you're the one's who've been active here recently and it's been silent a while (and since @fmeum, we've worked together in the past)

[fmeum]

This introduces a new dependency on the value of --symlink_prefix and also means that symlinks into the output base are still followed if the convenience symlinks aren't created (which is more likely with the new algorithm used in Bazel 6).

@cpsauer @linzhp Do you think we could instead decline to follow any symlinks that point out of the project directory? That is a condition that needs no external information to be evaluated.

[linzhp]

Yeah, I think that's a fair requirement

[jayconrod]

Unfortunately, I think the main use case for following symlinks was stitching together multiple repos into something that looks like a coherent workspace. It is probably necessary to follow symlinks outside the repo and outside the workspace root to avoid a regression.

[jayconrod]

Sorry for the delayed review; I think my GitHub notification settings are messed up.

Some history: symbolic links create problems for anything walking a directory tree, and it's generally best to treat them like regular files and not follow them. Unlike, filepath.Walk Gazelle does not take that advice. I think the reasons were:

• Kubernetes does some shenanigans with their vendor and staging directories. It looks like this is still the case.
• The person who originally contributed this functionality had separate repos for public and private code. They had been separate GOPATH directories. They wanted Gazelle to work, stitching the two repos together with symbolic links.
  • Up to the current maintainers whether you want to maintain this use case or break it. I wish I had said No to that PR since it made everything more complicated for the benefit of one organization.

So the main question is whether it's possible for Gazelle to tell if a symbolic link points inside Bazel's output base without running Bazel. Given --symlink_prefix and changes in new versions of Bazel, it probably can't be done perfectly without running bazel info output_base.

But maybe ignoring symbolic links that point inside a small number of output directories would be good enough? On macOS, bazel info output_base is always in /private/var/tmp/_bazel_$USER. On Linux, it's in $HOME/.cache/bazel/_bazel_$USER.

[jayconrod]

Unfortunately, I think the main use case for following symlinks was stitching together multiple repos into something that looks like a coherent workspace. It is probably necessary to follow symlinks outside the repo and outside the workspace root to avoid a regression.

[linzhp]

Up to the current maintainers whether you want to maintain this use case or break it.

I am OK with breaking it. Stitching two repos together using symlinks sounds more like a hack to me than legit scenario.

The person who originally contributed this functionality had separate repos for public and private code. They had been separate GOPATH directories. They wanted Gazelle to work, stitching the two repos together with symbolic links.

I think they should instead make them two Go modules and have the private module importing the public module.

[cpsauer]

Hey, guys! Thanks for getting back to me.

Just to clarify: Is the decision to get rid of all symlink traversal?
(Vs. just those pointing out of the workspace, also mentioned.)

[I should also note that, after more thought, I'm realizing that even bazel info output_base wouldn't get things perfectly, since, for example, the user could be specifying the output base as a flag instead of in .bazelrc. Using the fixed locations has a similar problem.]

Again, just here trying to help as a non-current user, so please forgive my ignorance--it's unclear how this would interact with, e.g. external repos that symlink things in.

If the decision is to not traverse symlinks at all, and it's obvious to you guys what code to strip, feel free to just close this and go for it--might be faster and save a RTT and it's not that related to the original diff. Might want to preserve the comment change in repo.go, though.

[cpsauer]

(Friendly bump.)

[linzhp]

Sorry for the delay. I am OK with a breaking change to disallow following symlinks all together, or a minor version of not following symlinks outside project directory like @fmeum suggested.

[cpsauer]

np. Thanks for replying to clarify, guys!

How's this for skipping symlinks altogether?

I think this'd also solve #168 in this form, so I've marked it as such.

[cpsauer]

Sweet! Thanks. Glad it looks good to you

[cpsauer]

Before I bounce, friendly reminder about the (separate but likely) problem with FindExternalRepo's handling of Windows junctions, that I'd happened upon while editing. No pressure to fix for me or anything--do with it what you want--but figure I ought to surface things I see.

I'm also skeptical that the neighboring, similar, symlink resolution code will work on windows; hence the use of Dir in my implementation. In my (python) experience, .. traverses back out of a Windows junction the way it came, as opposed to getting the parent of the symlinks destination, as on posix.
^ Fixing would require syscalls, ofc, but that seems likely worth it.

[felberj]

I just found this because I was wondering why "external" is not followed anymore. I am happy that I no longer need to exclude all the directories, but I would still like to follow some symlinks. It seems like the follow directive is broken right now?

I propose we follow a symlink if it is as part of the follow directive, WDYT?

[felberj]

Example: If I put

# gazelle:follow external

I expect gazelle to look into the external directory. This becomes useful with #1573 where we can actually resolve targets in external repositories.