Fix up dependency between RepositoryDirectoryValue and FileStateValue
In `RepositoryFunction#addExternalFilesDependencies`, we introduce a dependency from `FileStateValue($outputBase/external/foo/$SOME_PATH)` to `RepositoryDirectoryValue(@foo)`, _except_ when `$SOME_PATH` is empty (i.e. the "foo" directory itself), or when `@foo` is a local_repository and $SOME_PATH is "WORKSPACE".
These two exceptions are a bit strange (why would we expect anyone to mess with the external directory? and why should that result in a re-fetch?) and exist only because there is a dependency in the reverse direction (that is, `RepositoryDirectoryValue(@foo)` depends on `FileStateValue($outputBase/external/foo)`, and additionally on `FileStateValue($outputBase/external/foo/WORKSPACE)` if `@foo` is a local_repository).
This CL removes these exceptions. This means that the dependency is always from FileStateValue to RepositoryDirectoryValue. Meaning that if the repo definition changes in any way, we'll refetch the files in the external directory; but if the files in the external directory somehow change from under us, we don't trigger a refetch.
This change will also make our lives easier in the future with the external deps overhaul.
PiperOrigin-RevId: 374830398
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 da07b4d..b8d9029 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
@@ -22,7 +22,6 @@
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.repository.ExternalPackageHelper;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.TestType;
@@ -42,7 +41,6 @@
private final BlazeDirectories directories;
private final int maxNumExternalFilesToLog;
private final AtomicInteger numExternalFilesLogged = new AtomicInteger(0);
- private final ExternalPackageHelper externalPackageHelper;
// These variables are set to true from multiple threads, but only read in the main thread.
// So volatility or an AtomicBoolean is not needed.
@@ -56,36 +54,27 @@
ExternalFileAction externalFileAction,
BlazeDirectories directories,
int maxNumExternalFilesToLog,
- ManagedDirectoriesKnowledge managedDirectoriesKnowledge,
- ExternalPackageHelper externalPackageHelper) {
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
this.pkgLocator = pkgLocator;
this.externalFileAction = externalFileAction;
this.directories = directories;
this.maxNumExternalFilesToLog = maxNumExternalFilesToLog;
this.managedDirectoriesKnowledge = managedDirectoriesKnowledge;
- this.externalPackageHelper = externalPackageHelper;
}
public static ExternalFilesHelper create(
AtomicReference<PathPackageLocator> pkgLocator,
ExternalFileAction externalFileAction,
BlazeDirectories directories,
- ManagedDirectoriesKnowledge managedDirectoriesKnowledge,
- ExternalPackageHelper externalPackageHelper) {
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
return TestType.isInTest()
- ? createForTesting(
- pkgLocator,
- externalFileAction,
- directories,
- managedDirectoriesKnowledge,
- externalPackageHelper)
+ ? createForTesting(pkgLocator, externalFileAction, directories, managedDirectoriesKnowledge)
: new ExternalFilesHelper(
pkgLocator,
externalFileAction,
directories,
/*maxNumExternalFilesToLog=*/ 100,
- managedDirectoriesKnowledge,
- externalPackageHelper);
+ managedDirectoriesKnowledge);
}
public static ExternalFilesHelper createForTesting(
@@ -96,36 +85,21 @@
pkgLocator,
externalFileAction,
directories,
- BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER);
- }
-
- public static ExternalFilesHelper createForTesting(
- AtomicReference<PathPackageLocator> pkgLocator,
- ExternalFileAction externalFileAction,
- BlazeDirectories directories,
- ExternalPackageHelper externalPackageHelper) {
- return createForTesting(
- pkgLocator,
- externalFileAction,
- directories,
- ManagedDirectoriesKnowledge.NO_MANAGED_DIRECTORIES,
- externalPackageHelper);
+ ManagedDirectoriesKnowledge.NO_MANAGED_DIRECTORIES);
}
private static ExternalFilesHelper createForTesting(
AtomicReference<PathPackageLocator> pkgLocator,
ExternalFileAction externalFileAction,
BlazeDirectories directories,
- ManagedDirectoriesKnowledge managedDirectoriesKnowledge,
- ExternalPackageHelper externalPackageHelper) {
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
return new ExternalFilesHelper(
pkgLocator,
externalFileAction,
directories,
// These log lines are mostly spam during unit and integration tests.
/*maxNumExternalFilesToLog=*/ 0,
- managedDirectoriesKnowledge,
- externalPackageHelper);
+ managedDirectoriesKnowledge);
}
@@ -242,8 +216,7 @@
externalFileAction,
directories,
maxNumExternalFilesToLog,
- managedDirectoriesKnowledge,
- externalPackageHelper);
+ managedDirectoriesKnowledge);
}
public FileType getAndNoteFileType(RootedPath rootedPath) {
@@ -307,8 +280,7 @@
* a {@link NonexistentImmutableExternalFileException} instead.
*/
@ThreadSafe
- FileType maybeHandleExternalFile(
- RootedPath rootedPath, boolean isDirectory, SkyFunction.Environment env)
+ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
throws NonexistentImmutableExternalFileException, IOException, InterruptedException {
Pair<FileType, RepositoryName> pair = getFileTypeAndRepository(rootedPath);
@@ -338,8 +310,7 @@
Preconditions.checkState(
externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
externalFileAction);
- RepositoryFunction.addExternalFilesDependencies(
- rootedPath, isDirectory, directories, env, externalPackageHelper);
+ RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env);
break;
}
return fileType;
