From 10c363d892f4b290bb95954f8ccf81767904c7d3 Mon Sep 17 00:00:00 2001 From: Michajlo Matijkiw Date: Fri, 9 Oct 2015 22:11:34 +0000 Subject: 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 --- .../build/lib/skyframe/PackageFunction.java | 5 +-- .../lib/skyframe/SkylarkImportLookupFunction.java | 43 +++++++++++++--------- .../skyframe/SkylarkImportUniqueCycleFunction.java | 6 +-- .../skyframe/SkylarkImportUniqueCycleValue.java | 3 +- 4 files changed, 31 insertions(+), 26 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/skyframe') 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 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 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()); } - SkyValue computeInternal(SkyKey skyKey, Environment env, - @Nullable Set visitedKeysForCycle) - throws SkyFunctionException, InterruptedException { - PackageIdentifier arg = (PackageIdentifier) skyKey.argument(); - PathFragment file = arg.getPackageFragment(); + private SkyValue computeWithInlineCallsInternal( + PackageIdentifier pkgId, Environment env, Set visited) + throws SkyFunctionException, InterruptedException { + return computeInternal(pkgId, env, Preconditions.checkNotNull(visited, pkgId)); + } + + SkyValue computeInternal( + PackageIdentifier pkgId, Environment env, @Nullable Set 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 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 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 cycle = - CycleUtils.splitIntoPathAndChain(Predicates.equalTo(skyKey), visitedKeysForCycle) + if (!visited.add(pkgId)) { + ImmutableList 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 { +class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction { private static final SkyValue INSTANCE = new SkyValue() {}; @Override @@ -45,8 +44,7 @@ class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction cycle) { + static SkyKey key(ImmutableList cycle) { return AbstractChainUniquenessValue.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle); } } -- cgit v1.2.3