Skip to content

Commit

Permalink
Exclude convenience symlinks after changing the output base
Browse files Browse the repository at this point in the history
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.

Fixes #10653
  • Loading branch information
cameron-martin committed Feb 15, 2024
1 parent 76b9ee7 commit 82fd2c4
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(

PackageIdentifier packageId = PackageIdentifier.create(repositoryName, rootRelativePath);

if (packageId.getRepository().isMain()
&& fileValue.isSymlink()
&& fileValue
.getUnresolvedLinkTarget()
.startsWith(directories.getExecRootBase().asFragment())) {
// Symlinks back to the execroot are not traversed so that we avoid convenience symlinks.
// Note that it's not enough to just check for the convenience symlinks themselves,
// because if the value of --symlink_prefix changes, the old symlinks are left in place. This
// algorithm also covers more creative use cases where people create convenience symlinks
// somewhere in the directory tree manually.
if (packageId.getRepository().isMain() && isConvenienceSymlink(fileValue, rootRelativePath)) {
return ProcessPackageDirectoryResult.EMPTY_RESULT;
}

Expand Down Expand Up @@ -187,6 +178,35 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps(
/*additionalValuesToAggregate=*/ ImmutableMap.of());
}

// Note that it's not enough to just check for the convenience symlinks themselves,
// because if the value of --symlink_prefix changes, the old symlinks are left in place. It
// is also not sufficient to check whether the symlink points to a directory in the current
// exec root, since this can change between bazel invocations. Therefore we check if the
// suffix of the symlink source suggests it is a convenience symlink, then see if the symlink
// target is in a directory that looks like an execroot. This algorithm also covers more
// creative use cases where people create convenience symlinks somewhere in the directory
// tree manually.
private boolean isConvenienceSymlink(FileValue fileValue, PathFragment rootRelativePath) {
if (!fileValue.isSymlink()) {
return false;
}

PathFragment linkTarget = fileValue.getUnresolvedLinkTarget();

return linkTarget.startsWith(directories.getExecRootBase().asFragment())
|| (rootRelativePath.getBaseName().endsWith("-bin") && isInExecRoot(linkTarget, 4))
|| (rootRelativePath.getBaseName().endsWith("-out") && isInExecRoot(linkTarget, 2))
|| (rootRelativePath.getBaseName().endsWith("-testlogs") && isInExecRoot(linkTarget, 4))
|| (rootRelativePath.getBaseName().endsWith("-" + directories.getWorkingDirectory().getBaseName()) && isInExecRoot(linkTarget, 1));
}

// TODO: Look at DO_NOT_BUILD_HERE file.
private boolean isInExecRoot(PathFragment directory, int depth) {
PathFragment candidateExecRoot = directory.subFragment(0, directory.segmentCount() - depth);

return candidateExecRoot.getBaseName().equals("execroot");
}

private Iterable<SkyKey> getSubdirDeps(
DirectoryListingValue dirListingValue,
RootedPath rootedPath,
Expand Down
14 changes: 14 additions & 0 deletions src/test/py/bazel/query_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import os
import tempfile
from absl.testing import absltest
from src.test.py.bazel import test_base

Expand Down Expand Up @@ -129,6 +131,18 @@ def testBuildFilesForExternalRepos_NoDuplicates(self):
self.assertNotIn(item, result)
result.add(item)

def testQueryWithDifferentOutputBaseAfterBuilding(self):
output_base = tempfile.mkdtemp(dir = os.getenv("TEST_TMPDIR"))

self.ScratchFile('MODULE.bazel')
self.ScratchFile('BUILD', [
'py_binary(name="a", srcs=["a.py"])',
])
self.ScratchFile('a.py')
self.RunBazel(["build", "//..."])
self.RunBazel([f"--output_base={output_base}", "query", "//..."])


def _AssertQueryOutput(self, query_expr, *expected_results):
_, stdout, _ = self.RunBazel(['query', query_expr])

Expand Down

0 comments on commit 82fd2c4

Please sign in to comment.