diff options
author | 2015-10-09 22:11:34 +0000 | |
---|---|---|
committer | 2015-10-12 08:36:01 +0000 | |
commit | 10c363d892f4b290bb95954f8ccf81767904c7d3 (patch) | |
tree | ce8a49ed28d7c3191a6c04f523d94cb9a2b37a5e | |
parent | f87a414a6bf50613a2c419e53a96f76154f44ae3 (diff) |
Track skylark import cycles in terms of PackageIdentifiers
We don't have any strong guarantees around the stability of SkyKey#toString(),
which we rely on to canonicalize cycles. We can reasonably expect
PackageIdentifier#toString() to be stable enough though. We also hide away the
cycle detection details from external code in this change.
--
MOS_MIGRATED_REVID=105093095
4 files changed, 31 insertions, 26 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index ddc14accb8..78090e0a83 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -67,7 +67,6 @@ import com.google.devtools.build.skyframe.ValueOrExceptionUtils; import java.io.IOException; import java.util.Collection; import java.util.HashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -641,11 +640,9 @@ public class PackageFunction implements SkyFunction { SkylarkImportFailedException, InconsistentFilesystemException, ASTLookupInputException, BuildFileNotFoundException> lookupResult; - Set<SkyKey> visitedKeysForCycle = new LinkedHashSet<>(); try { SkyValue value = - skylarkImportLookupFunctionForInlining.computeWithInlineCalls(importKey, env, - visitedKeysForCycle); + skylarkImportLookupFunctionForInlining.computeWithInlineCalls(importKey, env); if (value == null) { Preconditions.checkState(env.valuesMissing(), importKey); // Don't give up on computing. This is against the Skyframe contract, but we don't want diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java index abc44906b9..b510f15532 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java @@ -39,6 +39,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; @@ -61,22 +62,28 @@ public class SkylarkImportLookupFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - return computeInternal(skyKey, env, null); + return computeInternal((PackageIdentifier) skyKey.argument(), env, null); } - SkyValue computeWithInlineCalls(SkyKey skyKey, Environment env, Set<SkyKey> visitedKeysForCycle) + SkyValue computeWithInlineCalls(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - return computeInternal(skyKey, env, Preconditions.checkNotNull(visitedKeysForCycle, skyKey)); + return computeWithInlineCallsInternal( + (PackageIdentifier) skyKey.argument(), env, new LinkedHashSet<PackageIdentifier>()); } - SkyValue computeInternal(SkyKey skyKey, Environment env, - @Nullable Set<SkyKey> visitedKeysForCycle) - throws SkyFunctionException, InterruptedException { - PackageIdentifier arg = (PackageIdentifier) skyKey.argument(); - PathFragment file = arg.getPackageFragment(); + private SkyValue computeWithInlineCallsInternal( + PackageIdentifier pkgId, Environment env, Set<PackageIdentifier> visited) + throws SkyFunctionException, InterruptedException { + return computeInternal(pkgId, env, Preconditions.checkNotNull(visited, pkgId)); + } + + SkyValue computeInternal( + PackageIdentifier pkgId, Environment env, @Nullable Set<PackageIdentifier> visited) + throws SkyFunctionException, InterruptedException { + PathFragment file = pkgId.getPackageFragment(); ASTFileLookupValue astLookupValue = null; try { - SkyKey astLookupKey = ASTFileLookupValue.key(arg); + SkyKey astLookupKey = ASTFileLookupValue.key(pkgId); astLookupValue = (ASTFileLookupValue) env.getValueOrThrow(astLookupKey, ErrorReadingSkylarkExtensionException.class, InconsistentFilesystemException.class); } catch (ErrorReadingSkylarkExtensionException e) { @@ -99,7 +106,7 @@ public class SkylarkImportLookupFunction implements SkyFunction { SkylarkImportFailedException.skylarkErrors(file)); } - Label label = pathFragmentToLabel(arg.getRepository(), file, env); + Label label = pathFragmentToLabel(pkgId.getRepository(), file, env); if (label == null) { Preconditions.checkState(env.valuesMissing(), "null label with no missing %s", file); return null; @@ -112,7 +119,7 @@ public class SkylarkImportLookupFunction implements SkyFunction { for (Map.Entry<Location, PathFragment> entry : ast.getImports().entrySet()) { try { skylarkImports.put( - PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, arg), + PackageFunction.getImportKey(entry, ruleClassProvider.getPreludePath(), file, pkgId), entry.getValue()); } catch (ASTLookupInputException e) { throw new SkylarkImportLookupFunctionException(e, Transience.PERSISTENT); @@ -121,15 +128,15 @@ public class SkylarkImportLookupFunction implements SkyFunction { Map<SkyKey, SkyValue> skylarkImportMap; boolean valuesMissing = false; - if (visitedKeysForCycle == null) { + if (visited == null) { // Not inlining. skylarkImportMap = env.getValues(skylarkImports.keySet()); valuesMissing = env.valuesMissing(); } else { // inlining calls to SkylarkImportLookupFunction. - if (!visitedKeysForCycle.add(skyKey)) { - ImmutableList<SkyKey> cycle = - CycleUtils.splitIntoPathAndChain(Predicates.equalTo(skyKey), visitedKeysForCycle) + if (!visited.add(pkgId)) { + ImmutableList<PackageIdentifier> cycle = + CycleUtils.splitIntoPathAndChain(Predicates.equalTo(pkgId), visited) .second; if (env.getValue(SkylarkImportUniqueCycleValue.key(cycle)) == null) { return null; @@ -139,7 +146,9 @@ public class SkylarkImportLookupFunction implements SkyFunction { } skylarkImportMap = Maps.newHashMapWithExpectedSize(astImports.size()); for (SkyKey skylarkImport : skylarkImports.keySet()) { - SkyValue skyValue = this.computeWithInlineCalls(skylarkImport, env, visitedKeysForCycle); + SkyValue skyValue = + this.computeWithInlineCallsInternal( + (PackageIdentifier) skylarkImport.argument(), env, visited); if (skyValue == null) { Preconditions.checkState( env.valuesMissing(), "no skylark import value for %s", skylarkImport); @@ -151,7 +160,7 @@ public class SkylarkImportLookupFunction implements SkyFunction { } } // All imports traversed, this key can no longer be part of a cycle. - visitedKeysForCycle.remove(skyKey); + visited.remove(pkgId); } if (valuesMissing) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java index e7c4decae7..a16ad9ab0a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java @@ -14,14 +14,13 @@ package com.google.devtools.build.lib.skyframe; import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; /** * Emits an error message exactly once when a Skylark import cycle is found when running inlined * {@link SkylarkImportLookupFunction}s. */ -class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<SkyKey> { +class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<PackageIdentifier> { private static final SkyValue INSTANCE = new SkyValue() {}; @Override @@ -45,8 +44,7 @@ class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<S } @Override - protected String elementToString(SkyKey elt) { - PackageIdentifier pkgId = (PackageIdentifier) elt.argument(); + protected String elementToString(PackageIdentifier pkgId) { return pkgId.getPathFragment().getPathString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java index 242fc25467..0b9df89721 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -22,7 +23,7 @@ import com.google.devtools.build.skyframe.SkyValue; * once. */ public class SkylarkImportUniqueCycleValue implements SkyValue { - static SkyKey key(ImmutableList<SkyKey> cycle) { + static SkyKey key(ImmutableList<PackageIdentifier> cycle) { return AbstractChainUniquenessValue.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle); } } |