Fix SkyQuery bug where we weren't respecting the package blacklist. We do this by changing both the relevant Skyframe and the SkyQuery code to propagate (minimal!) blacklist information in the SkyKeys themselves.
There are other approaches to solving this problem, but I like how this solution doesn't involve duplication of logic. Also, it has the following nice benefit: previously, RecursiveDirectoryTraversalFunction would declare a dep on the blacklist for every directory traversed which adds an edge for each directory traversed.
--
MOS_MIGRATED_REVID=120049635
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index 9caa115..7524bee 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -79,12 +79,24 @@
Preconditions.checkState(patternKey.getPolicy().equals(FilteringPolicies.NO_FILTER),
patternKey.getPolicy());
+ TargetPattern parsedPattern = patternKey.getParsedPattern();
+
+ BlacklistedPackagePrefixesValue blacklist =
+ (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key());
+ if (blacklist == null) {
+ return null;
+ }
+ ImmutableSet<PathFragment> subdirectoriesToExclude =
+ patternKey.getAllSubdirectoriesToExclude(blacklist.getPatterns());
+
+ DepsOfPatternPreparer preparer = new DepsOfPatternPreparer(env, pkgPath.get());
+
try {
- TargetPattern parsedPattern = patternKey.getParsedPattern();
- DepsOfPatternPreparer preparer = new DepsOfPatternPreparer(env, pkgPath.get());
- ImmutableSet<PathFragment> excludedSubdirectories = patternKey.getExcludedSubdirectories();
parsedPattern.eval(
- preparer, excludedSubdirectories, NullCallback.<Void>instance(), RuntimeException.class);
+ preparer,
+ subdirectoriesToExclude,
+ NullCallback.<Void>instance(),
+ RuntimeException.class);
} catch (TargetParsingException e) {
throw new PrepareDepsOfPatternFunctionException(e);
} catch (MissingDepException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index 5b5ee60..72d07b8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -45,7 +44,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.Set;
/**
* RecursiveDirectoryTraversalFunction traverses the subdirectories of a directory, looking for
@@ -139,13 +137,7 @@
*/
TReturn visitDirectory(RecursivePkgKey recursivePkgKey, Environment env) {
RootedPath rootedPath = recursivePkgKey.getRootedPath();
- BlacklistedPackagePrefixesValue blacklist =
- (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key());
- if (blacklist == null) {
- return null;
- }
- Set<PathFragment> excludedPaths =
- Sets.union(recursivePkgKey.getExcludedPaths(), blacklist.getPatterns());
+ ImmutableSet<PathFragment> excludedPaths = recursivePkgKey.getExcludedPaths();
Path root = rootedPath.getRoot();
PathFragment rootRelativePath = rootedPath.getRelativePath();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index e3ddb1e..6f6b48c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -112,7 +112,8 @@
Preprocessor.Factory.Supplier preprocessorFactorySupplier,
ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
- Iterable<SkyValueDirtinessChecker> customDirtinessCheckers) {
+ Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
+ PathFragment blacklistedPackagePrefixesFile) {
super(
evaluatorSupplier,
pkgFactory,
@@ -124,7 +125,8 @@
preprocessorFactorySupplier,
extraSkyFunctions,
extraPrecomputedValues,
- false);
+ false,
+ blacklistedPackagePrefixesFile);
this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories);
this.customDirtinessCheckers = customDirtinessCheckers;
}
@@ -141,6 +143,34 @@
ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
Iterable<SkyValueDirtinessChecker> customDirtinessCheckers) {
+ return create(
+ pkgFactory,
+ directories,
+ binTools,
+ workspaceStatusActionFactory,
+ buildInfoFactories,
+ diffAwarenessFactories,
+ allowedMissingInputs,
+ preprocessorFactorySupplier,
+ extraSkyFunctions,
+ extraPrecomputedValues,
+ customDirtinessCheckers,
+ /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT);
+ }
+
+ private static SequencedSkyframeExecutor create(
+ PackageFactory pkgFactory,
+ BlazeDirectories directories,
+ BinTools binTools,
+ Factory workspaceStatusActionFactory,
+ ImmutableList<BuildInfoFactory> buildInfoFactories,
+ Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories,
+ Predicate<PathFragment> allowedMissingInputs,
+ Preprocessor.Factory.Supplier preprocessorFactorySupplier,
+ ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
+ ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
+ Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
+ PathFragment blacklistedPackagePrefixesFile) {
SequencedSkyframeExecutor skyframeExecutor =
new SequencedSkyframeExecutor(
InMemoryMemoizingEvaluator.SUPPLIER,
@@ -154,7 +184,8 @@
preprocessorFactorySupplier,
extraSkyFunctions,
extraPrecomputedValues,
- customDirtinessCheckers);
+ customDirtinessCheckers,
+ blacklistedPackagePrefixesFile);
skyframeExecutor.init();
return skyframeExecutor;
}
@@ -164,7 +195,8 @@
BlazeDirectories directories, BinTools binTools,
WorkspaceStatusAction.Factory workspaceStatusActionFactory,
ImmutableList<BuildInfoFactory> buildInfoFactories,
- Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories) {
+ Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories,
+ PathFragment blacklistedPackagePrefixesFile) {
return create(
pkgFactory,
directories,
@@ -176,7 +208,8 @@
Preprocessor.Factory.Supplier.NullSupplier.INSTANCE,
ImmutableMap.<SkyFunctionName, SkyFunction>of(),
ImmutableList.<PrecomputedValue.Injected>of(),
- ImmutableList.<SkyValueDirtinessChecker>of());
+ ImmutableList.<SkyValueDirtinessChecker>of(),
+ blacklistedPackagePrefixesFile);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 6049fd4..60904c1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -265,6 +265,8 @@
private MutableSupplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments =
new MutableSupplier<>();
+ private final PathFragment blacklistedPackagePrefixesFile;
+
private static final Logger LOG = Logger.getLogger(SkyframeExecutor.class.getName());
protected SkyframeExecutor(
@@ -278,7 +280,8 @@
Preprocessor.Factory.Supplier preprocessorFactorySupplier,
ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
- boolean errorOnExternalFiles) {
+ boolean errorOnExternalFiles,
+ PathFragment blacklistedPackagePrefixesFile) {
// Strictly speaking, these arguments are not required for initialization, but all current
// callsites have them at hand, so we might as well set them during construction.
this.evaluatorSupplier = evaluatorSupplier;
@@ -298,6 +301,7 @@
this.extraSkyFunctions = extraSkyFunctions;
this.extraPrecomputedValues = extraPrecomputedValues;
this.errorOnExternalFiles = errorOnExternalFiles;
+ this.blacklistedPackagePrefixesFile = blacklistedPackagePrefixesFile;
this.binTools = binTools;
this.skyframeBuildView = new SkyframeBuildView(
@@ -503,8 +507,9 @@
}
}
- protected PathFragment getBlacklistedPackagePrefixesFile() {
- return PathFragment.EMPTY_FRAGMENT;
+ @VisibleForTesting
+ public PathFragment getBlacklistedPackagePrefixesFile() {
+ return blacklistedPackagePrefixesFile;
}
class BuildViewProvider {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternValue.java
index f247bde..c19a72b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternValue.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
import com.google.devtools.build.lib.cmdline.ResolvedTargets.Builder;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
@@ -205,6 +206,21 @@
return excludedSubdirectories;
}
+ public ImmutableSet<PathFragment> getAllSubdirectoriesToExclude(
+ Iterable<PathFragment> blacklistedPackagePrefixes) {
+ ImmutableSet.Builder<PathFragment> excludedPathsBuilder = ImmutableSet.builder();
+ excludedPathsBuilder.addAll(getExcludedSubdirectories());
+ for (PathFragment blacklistedPackagePrefix : blacklistedPackagePrefixes) {
+ PackageIdentifier pkgIdForBlacklistedDirectorPrefix = PackageIdentifier.create(
+ parsedPattern.getDirectory().getRepository(),
+ blacklistedPackagePrefix);
+ if (parsedPattern.containsBelowDirectory(pkgIdForBlacklistedDirectorPrefix)) {
+ excludedPathsBuilder.add(blacklistedPackagePrefix);
+ }
+ }
+ return excludedPathsBuilder.build();
+ }
+
@Override
public String toString() {
return (isNegative ? "-" : "") + parsedPattern.getOriginalPattern();
