From 62d8862bfb713ffd7353c4db0b0361b82887efd1 Mon Sep 17 00:00:00 2001 From: Cameron Martin Date: Tue, 23 Jan 2024 21:05:52 +0000 Subject: [PATCH 1/2] Exclude convenience symlinks after changing the output base 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 --- .../lib/skyframe/ProcessPackageDirectory.java | 57 +++++++++++++++---- src/test/py/bazel/query_test.py | 14 +++++ 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java index cd434404d964b4..b66ea152fce96e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java @@ -123,19 +123,14 @@ 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, rootedPath, env)) { return ProcessPackageDirectoryResult.EMPTY_RESULT; } + if(env.valuesMissing()) { + return null; + } + SkyKey pkgLookupKey = PackageLookupValue.key(packageId); SkyKey dirListingKey = DirectoryListingValue.key(rootedPath); SkyframeLookupResult pkgLookupAndDirectoryListingDeps = @@ -187,6 +182,48 @@ 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, RootedPath rootedPath, SkyFunction.Environment env) throws InterruptedException { + if (!fileValue.isSymlink()) { + return false; + } + + PathFragment rootRelativePath = rootedPath.getRootRelativePath(); + Root root = rootedPath.getRoot(); + PathFragment linkTarget = fileValue.getUnresolvedLinkTarget(); + + return linkTarget.startsWith(directories.getExecRootBase().asFragment()) + || (rootRelativePath.getBaseName().endsWith("-bin") && isInExecRoot(linkTarget, root, 4, env)) + || (rootRelativePath.getBaseName().endsWith("-genfiles") && isInExecRoot(linkTarget, root, 4, env)) + || (rootRelativePath.getBaseName().endsWith("-out") && isInExecRoot(linkTarget, root, 2, env)) + || (rootRelativePath.getBaseName().endsWith("-testlogs") && isInExecRoot(linkTarget, root, 4, env)) + || (rootRelativePath.getBaseName().endsWith("-" + directories.getWorkingDirectory().getBaseName()) && isInExecRoot(linkTarget, root, 1, env)); + } + + private boolean isInExecRoot(PathFragment path, Root root, int depth, SkyFunction.Environment env) throws InterruptedException { + PathFragment candidateExecRoot = path.subFragment(0, path.segmentCount() - depth); + + if(!candidateExecRoot.getBaseName().equals("execroot")) { + return false; + } + + Root absoluteRoot = Root.absoluteRoot(root.getFileSystem()); + RootedPath doNotBuildPath = RootedPath.toRootedPath(absoluteRoot, candidateExecRoot.getChild("DO_NOT_BUILD_HERE")); + FileValue doNotBuildValue = (FileValue)env.getValue(FileValue.key(doNotBuildPath)); + if (doNotBuildValue == null) { + return false; + } + + return doNotBuildValue.exists(); + } + private Iterable getSubdirDeps( DirectoryListingValue dirListingValue, RootedPath rootedPath, diff --git a/src/test/py/bazel/query_test.py b/src/test/py/bazel/query_test.py index 0af495e1281fb5..fff5b053d4c4ab 100644 --- a/src/test/py/bazel/query_test.py +++ b/src/test/py/bazel/query_test.py @@ -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 @@ -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]) From 3e169aec0048fb09b8afbde16459b7fcd6721c3e Mon Sep 17 00:00:00 2001 From: Cameron Martin Date: Fri, 23 Feb 2024 17:40:24 +0000 Subject: [PATCH 2/2] Address review comments * Split large or statement into multiple ifs. * Check if there is enough path segments in `isInExecRoot`. --- .../lib/skyframe/ProcessPackageDirectory.java | 42 +++++++++++++++---- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java index b66ea152fce96e..7813fad2f5921d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java @@ -195,20 +195,46 @@ private boolean isConvenienceSymlink(FileValue fileValue, RootedPath rootedPath, return false; } + PathFragment linkTarget = fileValue.getUnresolvedLinkTarget(); + + if(linkTarget.startsWith(directories.getExecRootBase().asFragment())) { + return true; + } + PathFragment rootRelativePath = rootedPath.getRootRelativePath(); Root root = rootedPath.getRoot(); - PathFragment linkTarget = fileValue.getUnresolvedLinkTarget(); - return linkTarget.startsWith(directories.getExecRootBase().asFragment()) - || (rootRelativePath.getBaseName().endsWith("-bin") && isInExecRoot(linkTarget, root, 4, env)) - || (rootRelativePath.getBaseName().endsWith("-genfiles") && isInExecRoot(linkTarget, root, 4, env)) - || (rootRelativePath.getBaseName().endsWith("-out") && isInExecRoot(linkTarget, root, 2, env)) - || (rootRelativePath.getBaseName().endsWith("-testlogs") && isInExecRoot(linkTarget, root, 4, env)) - || (rootRelativePath.getBaseName().endsWith("-" + directories.getWorkingDirectory().getBaseName()) && isInExecRoot(linkTarget, root, 1, env)); + if(rootRelativePath.getBaseName().endsWith("-bin") && isInExecRoot(linkTarget, root, 4, env)) { + return true; + } + + if(rootRelativePath.getBaseName().endsWith("-genfiles") && isInExecRoot(linkTarget, root, 4, env)) { + return true; + } + + if(rootRelativePath.getBaseName().endsWith("-out") && isInExecRoot(linkTarget, root, 2, env)) { + return true; + } + + if(rootRelativePath.getBaseName().endsWith("-testlogs") && isInExecRoot(linkTarget, root, 4, env)) { + return true; + } + + if(rootRelativePath.getBaseName().endsWith("-" + directories.getWorkingDirectory().getBaseName()) && isInExecRoot(linkTarget, root, 1, env)) { + return true; + } + + return false; } private boolean isInExecRoot(PathFragment path, Root root, int depth, SkyFunction.Environment env) throws InterruptedException { - PathFragment candidateExecRoot = path.subFragment(0, path.segmentCount() - depth); + int segmentCount = path.segmentCount(); + + if(segmentCount <= depth) { + return false; + } + + PathFragment candidateExecRoot = path.subFragment(0, segmentCount - depth); if(!candidateExecRoot.getBaseName().equals("execroot")) { return false;