From bfabb6feb06afce3f3cd31d3eb845afaf368e841 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 13 Mar 2026 18:06:59 +0100 Subject: [PATCH] [8.7.0] Refactoring from: Make external repo file checking actually useful Non-functional changes only: remove Pair indirection in ExternalFilesHelper, extract getExternalRepoName() and getExternalDirectory() helpers, move addExternalFilesDependencies into ExternalFilesHelper, modernize switch expression in DirtinessCheckerUtils, formatting fixes. Does not include the functional behavior change of refetching repos on external modifications. (cherry picked from commit 5e3f0c83735c67123c8f3fdca5ddd9be2bffcdd5) --- .../lib/skyframe/DirtinessCheckerUtils.java | 21 ++--- .../lib/skyframe/ExternalFilesHelper.java | 76 ++++++++++++++----- 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java index f321be30b265ed..9c2af9c7286838 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java @@ -112,8 +112,8 @@ static final class ExternalDirtinessChecker extends SkyValueDirtinessChecker { private final UnionDirtinessChecker checker = createBasicFilesystemDirtinessChecker(); - ExternalDirtinessChecker(ExternalFilesHelper externalFilesHelper, - EnumSet fileTypesToCheck) { + ExternalDirtinessChecker( + ExternalFilesHelper externalFilesHelper, EnumSet fileTypesToCheck) { this.externalFilesHelper = externalFilesHelper; this.fileTypesToCheck = fileTypesToCheck; } @@ -141,7 +141,8 @@ public SkyValueDirtinessChecker.DirtyResult check( @Nullable Version oldMtsv, SyscallCache syscallCache, @Nullable TimestampGranularityMonitor tsgm) { - FileType fileType = externalFilesHelper.getAndNoteFileType((RootedPath) skyKey.argument()); + var rootedPath = (RootedPath) skyKey.argument(); + var fileType = externalFilesHelper.getAndNoteFileType(rootedPath); boolean cacheable = isCacheableType(fileType); SkyValue newValue = checker.createNewValue(skyKey, cacheable ? syscallCache : SyscallCache.NO_CACHE, tsgm); @@ -158,16 +159,10 @@ public SkyValueDirtinessChecker.DirtyResult check( } private static boolean isCacheableType(FileType fileType) { - switch (fileType) { - case INTERNAL: - case EXTERNAL: - case BUNDLED: - return true; - case EXTERNAL_REPO: - case OUTPUT: - return false; - } - throw new AssertionError("Unknown type " + fileType); + return switch (fileType) { + case INTERNAL, EXTERNAL, BUNDLED -> true; + case EXTERNAL_REPO, OUTPUT -> false; + }; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index ca8293f9c6d736..4da04656160196 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java @@ -19,20 +19,23 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.BundledFileSystem; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; -import com.google.devtools.build.lib.rules.repository.RepositoryFunction; -import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.lib.util.TestType; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.io.IOException; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import javax.annotation.Nullable; /** Common utilities for dealing with paths outside the package roots. */ public class ExternalFilesHelper { @@ -72,7 +75,7 @@ public static ExternalFilesHelper create( return TestType.isInTest() ? createForTesting(pkgLocator, externalFileAction, directories) : new ExternalFilesHelper( - pkgLocator, externalFileAction, directories, /*maxNumExternalFilesToLog=*/ 100); + pkgLocator, externalFileAction, directories, /* maxNumExternalFilesToLog= */ 100); } public static ExternalFilesHelper createForTesting( @@ -84,10 +87,9 @@ public static ExternalFilesHelper createForTesting( externalFileAction, directories, // These log lines are mostly spam during unit and integration tests. - /*maxNumExternalFilesToLog=*/ 0); + /* maxNumExternalFilesToLog= */ 0); } - /** * The action to take when an external path is encountered. See {@link FileType} for the * definition of "external". @@ -156,11 +158,10 @@ public enum FileType { } /** - * Thrown by {@link #maybeHandleExternalFile} when an applicable path is processed (see - * {@link ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}. + * Thrown by {@link #maybeHandleExternalFile} when an applicable path is processed (see {@link + * ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}. */ - static class NonexistentImmutableExternalFileException extends Exception { - } + static class NonexistentImmutableExternalFileException extends Exception {} static class ExternalFilesKnowledge { final boolean anyOutputFilesSeen; @@ -203,10 +204,6 @@ ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() { } public FileType getAndNoteFileType(RootedPath rootedPath) { - return getFileTypeAndRepository(rootedPath).getFirst(); - } - - private Pair getFileTypeAndRepository(RootedPath rootedPath) { FileType fileType = detectFileType(rootedPath); if (FileType.EXTERNAL == fileType) { if (nonOutputExternalFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) { @@ -221,7 +218,7 @@ private Pair getFileTypeAndRepository(RootedPath roote if (FileType.OUTPUT == fileType) { anyOutputFilesSeen = true; } - return Pair.of(fileType, null); + return fileType; } /** @@ -262,9 +259,7 @@ private FileType detectFileType(RootedPath rootedPath) { @ThreadSafe FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env) throws NonexistentImmutableExternalFileException, IOException, InterruptedException { - Pair pair = getFileTypeAndRepository(rootedPath); - - FileType fileType = Preconditions.checkNotNull(pair.getFirst()); + FileType fileType = Preconditions.checkNotNull(getAndNoteFileType(rootedPath)); switch (fileType) { case BUNDLED: case INTERNAL: @@ -273,7 +268,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) { logger.atInfo().log("Encountered an external path %s", rootedPath); } - // fall through + // fall through case OUTPUT: if (externalFileAction == ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS) { @@ -284,9 +279,52 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment Preconditions.checkState( externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, externalFileAction); - RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env); + addExternalFilesDependencies(rootedPath, env); break; } return fileType; } + + /** + * For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding repo so + * that if the repo definition changes, the File/DirectoryStateValue will be re-evaluated. + * + *

Note that: - We don't add a dependency on the parent directory at the package root boundary, + * so the only transitive dependencies from files inside the package roots to external files are + * through symlinks. So the upwards transitive closure of external files is small. - The only way + * other than external repositories for external source files to get into the skyframe graph in the + * first place is through symlinks outside the package roots, which we neither want to encourage + * nor optimize for since it is not common. So the set of external files is small. + */ + private void addExternalFilesDependencies(RootedPath rootedPath, Environment env) + throws InterruptedException { + var repositoryName = getExternalRepoName(rootedPath); + if (repositoryName != null) { + env.getValue(RepositoryDirectoryValue.key(repositoryName)); + } + } + + /** + * For a file of type {@link FileType#EXTERNAL_REPO}, returns the name of the external repository + * it is in or null if the path is not in a valid external repository. + */ + @Nullable + RepositoryName getExternalRepoName(RootedPath rootedPath) { + PathFragment repositoryPath = rootedPath.asPath().relativeTo(getExternalDirectory()); + if (repositoryPath.isEmpty()) { + // We are the top of the repository path (/external), not in an actual external + // repository path. + return null; + } + try { + return RepositoryName.create(repositoryPath.getSegment(0)); + } catch (LabelSyntaxException ignored) { + // The directory of a repository with an invalid name can never exist. + return null; + } + } + + private Path getExternalDirectory() { + return directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION); + } }