Set the version of a computed node to the max of its child versions rather than the graph version when that is feasible. * It's not feasible when the computation accesses outside state, i.e. is non-hermetic, so see below. * It's also more complicated (and not worth the trouble) when the computation is taking place just for the error status. Have SkyFunctionName declare whether the function it corresponds to is hermetic or non-hermetic. Only non-hermetically-generated SkyValues can be directly marked changed, and non-hermetic SkyFunctions have their values saved at the graph version, not the max of the child versions. All SkyFunctions are hermetic except for the ones that can be explicitly dirtied. A marked-hermetic SkyFunction that has a transient error due to filesystem access can be re-evaluated and get the correct version: if it throws an IOException at version 1 and then, when re-evaluated at version 2 with unchanged dependencies, has a value, the version will be version 1. All Skyframe unit tests that were doing non-hermetic things to nodes need to declare that those nodes are non-hermetic. I tried to make the minimal set of changes there, so that we had good incidental coverage of hermetic+non-hermetic nodes. Also did some drive-by clean-ups around that code. Artifacts are a weird case, since they're doing untracked filesystem access (for source directories). Using max(child versions) for them gives rise to the following correctness bug: 1. do a build at v1 that creates a FileStateValue for dir/ at v1. Then at v2, add a file to dir/ and do a build that consumes dir/ as a source artifact. Now the artifact for dir/ will (incorrectly) have v1. Then at v1, do that build again. We'll consume the "artifact from the future". However, this can only have an effect when using the local action cache, since the incorrect value of the artifact (the mtime) is only consumed by the action cache. Bazel is already broken in this way (incremental builds don't invalidate directories), so this change doesn't make things worse. PiperOrigin-RevId: 204210719
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java index b456de9..d287d77 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionLookupData.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.skyframe.FunctionHermeticity; import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -28,8 +29,10 @@ public class ActionLookupData implements SkyKey { private static final Interner<ActionLookupData> INTERNER = BlazeInterners.newWeakInterner(); // Test actions are not shareable. + // Action execution writes to disk and can be invalidated by disk state, so is non-hermetic. public static final SkyFunctionName NAME = - SkyFunctionName.create("ACTION_EXECUTION", ShareabilityOfValue.SOMETIMES); + SkyFunctionName.create( + "ACTION_EXECUTION", ShareabilityOfValue.SOMETIMES, FunctionHermeticity.NONHERMETIC); private final ActionLookupKey actionLookupKey; private final int actionIndex;
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index a27756e..f21d579a 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -127,7 +127,19 @@ } }; - public static final SkyFunctionName ARTIFACT = SkyFunctionName.create("ARTIFACT"); + /** + * {@link com.google.devtools.build.lib.skyframe.ArtifactFunction} does direct filesystem access + * without declaring Skyframe dependencies if the artifact is a source directory. However, that + * filesystem access is not invalidated on incremental builds, and we have no plans to fix it, + * since general consumption of source directories in this way is unsound. Therefore no new bugs + * are created by declaring {@link com.google.devtools.build.lib.skyframe.ArtifactFunction} to be + * hermetic. + * + * <p>TODO(janakr): Avoid this issue entirely by giving {@link SourceArtifact} its own {@code + * SkyFunction}. Then we can just declare that function to be non-hermetic. That will also save + * memory since we can make mandatory source artifacts their own SkyKeys! + */ + public static final SkyFunctionName ARTIFACT = SkyFunctionName.createHermetic("ARTIFACT"); @Override public int compareTo(Object o) {
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java index bc4b51a..1a5cceb 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
@@ -56,7 +56,7 @@ */ @VisibleForTesting public abstract class FileStateValue implements SkyValue { - public static final SkyFunctionName FILE_STATE = SkyFunctionName.create("FILE_STATE"); + public static final SkyFunctionName FILE_STATE = SkyFunctionName.createNonHermetic("FILE_STATE"); @AutoCodec public static final DirectoryFileStateValue DIRECTORY_FILE_STATE_NODE =
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java index d862881..89450d2 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java
@@ -49,7 +49,8 @@ @Immutable @ThreadSafe public abstract class FileValue implements SkyValue { - public static final SkyFunctionName FILE = SkyFunctionName.create("FILE"); + // Depends non-hermetically on package path. + public static final SkyFunctionName FILE = SkyFunctionName.createNonHermetic("FILE"); /** * Exists to accommodate the control flow of {@link ActionMetadataHandler#getMetadata}.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenServerFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenServerFunction.java index 6e0a8b0..24c24ba 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenServerFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenServerFunction.java
@@ -53,7 +53,8 @@ * Implementation of maven_repository. */ public class MavenServerFunction implements SkyFunction { - public static final SkyFunctionName NAME = SkyFunctionName.create("MAVEN_SERVER_FUNCTION"); + public static final SkyFunctionName NAME = + SkyFunctionName.createHermetic("MAVEN_SERVER_FUNCTION"); private static final String USER_KEY = "user"; private static final String SYSTEM_KEY = "system";
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index 831c408..43d9846 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -82,7 +82,7 @@ public static final PathFragment EXTERNAL_PATH_PREFIX = PathFragment.create("external"); public static final SkyFunctionName TRANSITIVE_TRAVERSAL = - SkyFunctionName.create("TRANSITIVE_TRAVERSAL"); + SkyFunctionName.createHermetic("TRANSITIVE_TRAVERSAL"); private static final Interner<Label> LABEL_INTERNER = BlazeInterners.newWeakInterner();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java index 5d2192b..b6d0d9e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
@@ -36,7 +36,7 @@ @AutoCodec @Immutable public class FdoSupportValue implements SkyValue { - public static final SkyFunctionName SKYFUNCTION = SkyFunctionName.create("FDO_SUPPORT"); + public static final SkyFunctionName SKYFUNCTION = SkyFunctionName.createHermetic("FDO_SUPPORT"); /** {@link SkyKey} for {@link FdoSupportValue}. */ @Immutable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index c998980..b501f3a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.Differencer; +import com.google.devtools.build.skyframe.FunctionHermeticity; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -506,6 +507,10 @@ if (!checker.applies(key)) { continue; } + Preconditions.checkState( + key.functionName().getHermeticity() == FunctionHermeticity.NONHERMETIC, + "Only non-hermetic keys can be dirty roots: %s", + key); executor.execute( wrapper.wrap( () -> {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index aed4d5d..be40e06 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -15,6 +15,7 @@ import com.google.common.base.Predicate; import com.google.devtools.build.lib.actions.ActionLookupData; +import com.google.devtools.build.skyframe.FunctionHermeticity; import com.google.devtools.build.skyframe.ShareabilityOfValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -23,98 +24,116 @@ * Value types in Skyframe. */ public final class SkyFunctions { - public static final SkyFunctionName PRECOMPUTED = SkyFunctionName.create("PRECOMPUTED"); + public static final SkyFunctionName PRECOMPUTED = + SkyFunctionName.createNonHermetic("PRECOMPUTED"); public static final SkyFunctionName CLIENT_ENVIRONMENT_VARIABLE = - SkyFunctionName.create("CLIENT_ENVIRONMENT_VARIABLE"); + SkyFunctionName.createNonHermetic("CLIENT_ENVIRONMENT_VARIABLE"); public static final SkyFunctionName ACTION_ENVIRONMENT_VARIABLE = - SkyFunctionName.create("ACTION_ENVIRONMENT_VARIABLE"); + SkyFunctionName.createHermetic("ACTION_ENVIRONMENT_VARIABLE"); public static final SkyFunctionName DIRECTORY_LISTING_STATE = - SkyFunctionName.create("DIRECTORY_LISTING_STATE"); + SkyFunctionName.createNonHermetic("DIRECTORY_LISTING_STATE"); public static final SkyFunctionName FILE_SYMLINK_CYCLE_UNIQUENESS = - SkyFunctionName.create("FILE_SYMLINK_CYCLE_UNIQUENESS"); + SkyFunctionName.createHermetic("FILE_SYMLINK_CYCLE_UNIQUENESS"); public static final SkyFunctionName FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS = - SkyFunctionName.create("FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS"); + SkyFunctionName.createHermetic("FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS"); public static final SkyFunctionName DIRECTORY_LISTING = - SkyFunctionName.create("DIRECTORY_LISTING"); - public static final SkyFunctionName PACKAGE_LOOKUP = SkyFunctionName.create("PACKAGE_LOOKUP"); + SkyFunctionName.createHermetic("DIRECTORY_LISTING"); + // Non-hermetic because unfortunately package lookups secretly access the set of deleted packages. + public static final SkyFunctionName PACKAGE_LOOKUP = + SkyFunctionName.createNonHermetic("PACKAGE_LOOKUP"); public static final SkyFunctionName CONTAINING_PACKAGE_LOOKUP = - SkyFunctionName.create("CONTAINING_PACKAGE_LOOKUP"); - public static final SkyFunctionName AST_FILE_LOOKUP = SkyFunctionName.create("AST_FILE_LOOKUP"); + SkyFunctionName.createHermetic("CONTAINING_PACKAGE_LOOKUP"); + // Non-hermetic because accesses the package locator. Also does disk access. + public static final SkyFunctionName AST_FILE_LOOKUP = + SkyFunctionName.createNonHermetic("AST_FILE_LOOKUP"); public static final SkyFunctionName SKYLARK_IMPORTS_LOOKUP = - SkyFunctionName.create("SKYLARK_IMPORTS_LOOKUP"); - public static final SkyFunctionName GLOB = SkyFunctionName.create("GLOB"); - public static final SkyFunctionName PACKAGE = SkyFunctionName.create("PACKAGE"); - public static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.create("PACKAGE_ERROR"); - public static final SkyFunctionName PACKAGE_ERROR_MESSAGE = - SkyFunctionName.create("PACKAGE_ERROR_MESSAGE"); - public static final SkyFunctionName TARGET_MARKER = SkyFunctionName.create("TARGET_MARKER"); - public static final SkyFunctionName TARGET_PATTERN = SkyFunctionName.create("TARGET_PATTERN"); - public static final SkyFunctionName TARGET_PATTERN_ERROR = - SkyFunctionName.create("TARGET_PATTERN_ERROR"); + SkyFunctionName.createHermetic("SKYLARK_IMPORTS_LOOKUP"); + public static final SkyFunctionName GLOB = SkyFunctionName.createHermetic("GLOB"); + public static final SkyFunctionName PACKAGE = SkyFunctionName.createHermetic("PACKAGE"); + static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.createHermetic("PACKAGE_ERROR"); + static final SkyFunctionName PACKAGE_ERROR_MESSAGE = + SkyFunctionName.createHermetic("PACKAGE_ERROR_MESSAGE"); + public static final SkyFunctionName TARGET_MARKER = + SkyFunctionName.createHermetic("TARGET_MARKER"); + // Non-hermetic because accesses package locator + public static final SkyFunctionName TARGET_PATTERN = + SkyFunctionName.createNonHermetic("TARGET_PATTERN"); + static final SkyFunctionName TARGET_PATTERN_ERROR = + SkyFunctionName.createHermetic("TARGET_PATTERN_ERROR"); public static final SkyFunctionName PREPARE_DEPS_OF_PATTERNS = - SkyFunctionName.create("PREPARE_DEPS_OF_PATTERNS"); + SkyFunctionName.createHermetic("PREPARE_DEPS_OF_PATTERNS"); + // Non-hermetic because accesses package locator public static final SkyFunctionName PREPARE_DEPS_OF_PATTERN = - SkyFunctionName.create("PREPARE_DEPS_OF_PATTERN"); + SkyFunctionName.createNonHermetic("PREPARE_DEPS_OF_PATTERN"); public static final SkyFunctionName PREPARE_DEPS_OF_TARGETS_UNDER_DIRECTORY = - SkyFunctionName.create("PREPARE_DEPS_OF_TARGETS_UNDER_DIRECTORY"); + SkyFunctionName.createHermetic("PREPARE_DEPS_OF_TARGETS_UNDER_DIRECTORY"); public static final SkyFunctionName COLLECT_TARGETS_IN_PACKAGE = - SkyFunctionName.create("COLLECT_TARGETS_IN_PACKAGE"); + SkyFunctionName.createHermetic("COLLECT_TARGETS_IN_PACKAGE"); public static final SkyFunctionName COLLECT_PACKAGES_UNDER_DIRECTORY = - SkyFunctionName.create("COLLECT_PACKAGES_UNDER_DIRECTORY"); + SkyFunctionName.createHermetic("COLLECT_PACKAGES_UNDER_DIRECTORY"); public static final SkyFunctionName BLACKLISTED_PACKAGE_PREFIXES = - SkyFunctionName.create("BLACKLISTED_PACKAGE_PREFIXES"); - public static final SkyFunctionName TEST_SUITE_EXPANSION = - SkyFunctionName.create("TEST_SUITE_EXPANSION"); - public static final SkyFunctionName TESTS_IN_SUITE = SkyFunctionName.create("TESTS_IN_SUITE"); - public static final SkyFunctionName TARGET_PATTERN_PHASE = - SkyFunctionName.create("TARGET_PATTERN_PHASE"); - public static final SkyFunctionName RECURSIVE_PKG = SkyFunctionName.create("RECURSIVE_PKG"); - public static final SkyFunctionName TRANSITIVE_TARGET = - SkyFunctionName.create("TRANSITIVE_TARGET"); + SkyFunctionName.createHermetic("BLACKLISTED_PACKAGE_PREFIXES"); + static final SkyFunctionName TEST_SUITE_EXPANSION = + SkyFunctionName.createHermetic("TEST_SUITE_EXPANSION"); + static final SkyFunctionName TESTS_IN_SUITE = SkyFunctionName.createHermetic("TESTS_IN_SUITE"); + // Non-hermetic because accesses package locator + static final SkyFunctionName TARGET_PATTERN_PHASE = + SkyFunctionName.createNonHermetic("TARGET_PATTERN_PHASE"); + static final SkyFunctionName RECURSIVE_PKG = SkyFunctionName.createHermetic("RECURSIVE_PKG"); + static final SkyFunctionName TRANSITIVE_TARGET = + SkyFunctionName.createHermetic("TRANSITIVE_TARGET"); public static final SkyFunctionName CONFIGURED_TARGET = - SkyFunctionName.create("CONFIGURED_TARGET"); + SkyFunctionName.createHermetic("CONFIGURED_TARGET"); public static final SkyFunctionName POST_CONFIGURED_TARGET = - SkyFunctionName.create("POST_CONFIGURED_TARGET"); - public static final SkyFunctionName ASPECT = SkyFunctionName.create("ASPECT"); - public static final SkyFunctionName LOAD_SKYLARK_ASPECT = - SkyFunctionName.create("LOAD_SKYLARK_ASPECT"); + SkyFunctionName.createHermetic("POST_CONFIGURED_TARGET"); + public static final SkyFunctionName ASPECT = SkyFunctionName.createHermetic("ASPECT"); + static final SkyFunctionName LOAD_SKYLARK_ASPECT = + SkyFunctionName.createHermetic("LOAD_SKYLARK_ASPECT"); public static final SkyFunctionName TARGET_COMPLETION = - SkyFunctionName.create("TARGET_COMPLETION"); + SkyFunctionName.createHermetic("TARGET_COMPLETION"); public static final SkyFunctionName ASPECT_COMPLETION = - SkyFunctionName.create("ASPECT_COMPLETION"); - public static final SkyFunctionName TEST_COMPLETION = - SkyFunctionName.create("TEST_COMPLETION", ShareabilityOfValue.NEVER); - public static final SkyFunctionName BUILD_CONFIGURATION = - SkyFunctionName.create("BUILD_CONFIGURATION"); + SkyFunctionName.createHermetic("ASPECT_COMPLETION"); + static final SkyFunctionName TEST_COMPLETION = + SkyFunctionName.create( + "TEST_COMPLETION", ShareabilityOfValue.NEVER, FunctionHermeticity.HERMETIC); + static final SkyFunctionName BUILD_CONFIGURATION = + SkyFunctionName.createHermetic("BUILD_CONFIGURATION"); public static final SkyFunctionName CONFIGURATION_FRAGMENT = - SkyFunctionName.create("CONFIGURATION_FRAGMENT"); + SkyFunctionName.createHermetic("CONFIGURATION_FRAGMENT"); public static final SkyFunctionName ACTION_EXECUTION = ActionLookupData.NAME; - public static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL = - SkyFunctionName.create("RECURSIVE_DIRECTORY_TRAVERSAL"); - public static final SkyFunctionName FILESET_ENTRY = SkyFunctionName.create("FILESET_ENTRY"); - public static final SkyFunctionName BUILD_INFO_COLLECTION = - SkyFunctionName.create("BUILD_INFO_COLLECTION"); - public static final SkyFunctionName BUILD_INFO = SkyFunctionName.create("BUILD_INFO"); - public static final SkyFunctionName WORKSPACE_NAME = SkyFunctionName.create("WORKSPACE_NAME"); - public static final SkyFunctionName WORKSPACE_FILE = SkyFunctionName.create("WORKSPACE_FILE"); - public static final SkyFunctionName COVERAGE_REPORT = SkyFunctionName.create("COVERAGE_REPORT"); - public static final SkyFunctionName REPOSITORY = SkyFunctionName.create("REPOSITORY"); + static final SkyFunctionName RECURSIVE_FILESYSTEM_TRAVERSAL = + SkyFunctionName.createHermetic("RECURSIVE_DIRECTORY_TRAVERSAL"); + public static final SkyFunctionName FILESET_ENTRY = + SkyFunctionName.createHermetic("FILESET_ENTRY"); + static final SkyFunctionName BUILD_INFO_COLLECTION = + SkyFunctionName.createHermetic("BUILD_INFO_COLLECTION"); + public static final SkyFunctionName BUILD_INFO = SkyFunctionName.createHermetic("BUILD_INFO"); + public static final SkyFunctionName WORKSPACE_NAME = + SkyFunctionName.createHermetic("WORKSPACE_NAME"); + // Non-hermetic because accesses package locator + public static final SkyFunctionName WORKSPACE_FILE = + SkyFunctionName.createNonHermetic("WORKSPACE_FILE"); + static final SkyFunctionName COVERAGE_REPORT = SkyFunctionName.createHermetic("COVERAGE_REPORT"); + public static final SkyFunctionName REPOSITORY = SkyFunctionName.createHermetic("REPOSITORY"); public static final SkyFunctionName REPOSITORY_DIRECTORY = - SkyFunctionName.create("REPOSITORY_DIRECTORY"); - public static final SkyFunctionName WORKSPACE_AST = SkyFunctionName.create("WORKSPACE_AST"); - public static final SkyFunctionName EXTERNAL_PACKAGE = SkyFunctionName.create("EXTERNAL_PACKAGE"); - public static final SkyFunctionName ACTION_TEMPLATE_EXPANSION = - SkyFunctionName.create("ACTION_TEMPLATE_EXPANSION"); + SkyFunctionName.createNonHermetic("REPOSITORY_DIRECTORY"); + public static final SkyFunctionName WORKSPACE_AST = + SkyFunctionName.createHermetic("WORKSPACE_AST"); + // Non-hermetic because accesses package locator + public static final SkyFunctionName EXTERNAL_PACKAGE = + SkyFunctionName.createNonHermetic("EXTERNAL_PACKAGE"); + static final SkyFunctionName ACTION_TEMPLATE_EXPANSION = + SkyFunctionName.createHermetic("ACTION_TEMPLATE_EXPANSION"); public static final SkyFunctionName LOCAL_REPOSITORY_LOOKUP = - SkyFunctionName.create("LOCAL_REPOSITORY_LOOKUP"); - public static final SkyFunctionName REGISTERED_EXECUTION_PLATFORMS = - SkyFunctionName.create("REGISTERED_EXECUTION_PLATFORMS"); - public static final SkyFunctionName REGISTERED_TOOLCHAINS = - SkyFunctionName.create("REGISTERED_TOOLCHAINS"); - public static final SkyFunctionName TOOLCHAIN_RESOLUTION = - SkyFunctionName.create("TOOLCHAIN_RESOLUTION"); + SkyFunctionName.createHermetic("LOCAL_REPOSITORY_LOOKUP"); + static final SkyFunctionName REGISTERED_EXECUTION_PLATFORMS = + SkyFunctionName.createHermetic("REGISTERED_EXECUTION_PLATFORMS"); + static final SkyFunctionName REGISTERED_TOOLCHAINS = + SkyFunctionName.createHermetic("REGISTERED_TOOLCHAINS"); + static final SkyFunctionName TOOLCHAIN_RESOLUTION = + SkyFunctionName.createHermetic("TOOLCHAIN_RESOLUTION"); public static final SkyFunctionName REPOSITORY_MAPPING = - SkyFunctionName.create("REPOSITORY_MAPPING"); + SkyFunctionName.createHermetic("REPOSITORY_MAPPING"); public static Predicate<SkyKey> isSkyFunction(final SkyFunctionName functionName) { return new Predicate<SkyKey>() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyValueDirtinessChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyValueDirtinessChecker.java index 2a41a47..ad852d6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyValueDirtinessChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyValueDirtinessChecker.java
@@ -24,8 +24,10 @@ * up to date. */ public abstract class SkyValueDirtinessChecker { - - /** Returns {@code true} iff the checker can handle {@code key}. */ + /** + * Returns {@code true} iff the checker can handle {@code key}. Can only be true if {@code + * key.functionName().getHermeticity() == FunctionHermeticity.NONHERMETIC}. + */ public abstract boolean applies(SkyKey key); /**
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java index a82d115..af658f3 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java
@@ -114,7 +114,8 @@ DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { super( graph, graphVersion, @@ -127,7 +128,8 @@ progressReceiver, graphInconsistencyReceiver, forkJoinPool, - cycleDetector); + cycleDetector, + evaluationVersionBehavior); } private void informProgressReceiverThatValueIsDone(SkyKey key, NodeEntry entry) @@ -469,6 +471,7 @@ maybeMarkRebuilding(parentEntry); // Fall through to REBUILDING. case REBUILDING: + case FORCED_REBUILDING: break; default: throw new AssertionError(parent + " not in valid dirty state: " + parentEntry);
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java index c2f8561..04fcdea 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java
@@ -104,7 +104,8 @@ DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { this.graph = graph; this.cycleDetector = cycleDetector; evaluatorContext = @@ -120,14 +121,17 @@ errorInfoManager, Evaluate::new, graphInconsistencyReceiver, - Preconditions.checkNotNull(forkJoinPool)); + Preconditions.checkNotNull(forkJoinPool), + evaluationVersionBehavior); } /** * If the entry is dirty and not already rebuilding, puts it in a state so that it can rebuild. */ static void maybeMarkRebuilding(NodeEntry entry) { - if (entry.isDirty() && entry.getDirtyState() != DirtyState.REBUILDING) { + if (entry.isDirty() + && entry.getDirtyState() != DirtyState.REBUILDING + && entry.getDirtyState() != DirtyState.FORCED_REBUILDING) { entry.markRebuilding(); } } @@ -310,6 +314,7 @@ maybeMarkRebuilding(state); // Fall through to REBUILDING case. case REBUILDING: + case FORCED_REBUILDING: return DirtyOutcome.NEEDS_EVALUATION; default: throw new IllegalStateException("key: " + skyKey + ", entry: " + state);
diff --git a/src/main/java/com/google/devtools/build/skyframe/BUILD b/src/main/java/com/google/devtools/build/skyframe/BUILD index 8a57fd0..c75206d 100644 --- a/src/main/java/com/google/devtools/build/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/skyframe/BUILD
@@ -6,6 +6,7 @@ SKYFRAME_OBJECT_SRCS = [ "AbstractSkyKey.java", + "FunctionHermeticity.java", "ShareabilityOfValue.java", "SkyFunctionName.java", "SkyKey.java", @@ -38,7 +39,6 @@ "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", - "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/common/options", "//third_party:error_prone", "//third_party:guava",
diff --git a/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java b/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java index 59a2e13..2732fc4 100644 --- a/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java +++ b/src/main/java/com/google/devtools/build/skyframe/DirtyBuildingState.java
@@ -93,16 +93,20 @@ final void forceChanged() { Preconditions.checkState(dirtyState == DirtyState.CHECK_DEPENDENCIES, this); Preconditions.checkState(getNumOfGroupsInLastBuildDirectDeps() == dirtyDirectDepIndex, this); - dirtyState = DirtyState.REBUILDING; + dirtyState = DirtyState.FORCED_REBUILDING; } final boolean isChanged() { - return dirtyState == DirtyState.NEEDS_REBUILDING || dirtyState == DirtyState.REBUILDING; + return dirtyState == DirtyState.NEEDS_REBUILDING + || dirtyState == DirtyState.REBUILDING + || dirtyState == DirtyState.FORCED_REBUILDING; } private void checkFinishedBuildingWhenAboutToSetValue() { Preconditions.checkState( - dirtyState == DirtyState.VERIFIED_CLEAN || dirtyState == DirtyState.REBUILDING, + dirtyState == DirtyState.VERIFIED_CLEAN + || dirtyState == DirtyState.REBUILDING + || dirtyState == DirtyState.FORCED_REBUILDING, "not done building %s", this); } @@ -194,7 +198,8 @@ } final void resetForRestartFromScratch() { - Preconditions.checkState(dirtyState == DirtyState.REBUILDING, this); + Preconditions.checkState( + dirtyState == DirtyState.REBUILDING || dirtyState == DirtyState.FORCED_REBUILDING, this); dirtyDirectDepIndex = 0; }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java index babf902..bd9ce9c 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java +++ b/src/main/java/com/google/devtools/build/skyframe/ErrorTransienceValue.java
@@ -22,7 +22,8 @@ * failure. Is not equal to anything, including itself, in order to force re-evaluation. */ public final class ErrorTransienceValue implements SkyValue { - public static final SkyFunctionName FUNCTION_NAME = SkyFunctionName.create("ERROR_TRANSIENCE"); + public static final SkyFunctionName FUNCTION_NAME = + SkyFunctionName.createNonHermetic("ERROR_TRANSIENCE"); @AutoCodec public static final SkyKey KEY = () -> FUNCTION_NAME; @AutoCodec public static final ErrorTransienceValue INSTANCE = new ErrorTransienceValue();
diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationVersionBehavior.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationVersionBehavior.java new file mode 100644 index 0000000..a4cc371 --- /dev/null +++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationVersionBehavior.java
@@ -0,0 +1,29 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.skyframe; + +/** + * What version to give an evaluated node: the max of its child versions or the graph version. Even + * for {@link #MAX_CHILD_VERSIONS} the version may still be the graph version depending on + * properties of the {@link SkyFunction} (if it is {@link FunctionHermeticity#NONHERMETIC}) or the + * error state of the node. + * + * <p>Should be set to {@link #MAX_CHILD_VERSIONS} unless the evaluation framework is being very + * sneaky. + */ +public enum EvaluationVersionBehavior { + MAX_CHILD_VERSIONS, + GRAPH_VERSION +}
diff --git a/src/main/java/com/google/devtools/build/skyframe/FunctionHermeticity.java b/src/main/java/com/google/devtools/build/skyframe/FunctionHermeticity.java new file mode 100644 index 0000000..fd19b75 --- /dev/null +++ b/src/main/java/com/google/devtools/build/skyframe/FunctionHermeticity.java
@@ -0,0 +1,31 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.skyframe; + +/** + * Hermeticity of a {@link SkyFunction}, meaning whether it accesses external state untracked by + * Skyframe during its evaluation. A classic example is a {@link SkyFunction} that consumes a file + * on a filesystem: that state is untracked by Skyframe. Skyframe must be more conservative when + * using values generated by a non-hermetic function: for instance, a non-hermetic function may need + * to be re-run even if all its Skyframe dependencies are unchanged: such a node may be explicitly + * dirtied due to outside changes. + * + * <p>Note that Skyframe does <i>not</i> explicitly re-evaluate non-hermetic functions on every + * build: it just relaxes some of its graph-pruning logic to be more conservative with such nodes. + */ +public enum FunctionHermeticity { + HERMETIC, + NONHERMETIC +}
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java index 859f531..12e3a23 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java
@@ -307,8 +307,16 @@ // value, because preserving == equality is even better than .equals() equality. this.value = getDirtyBuildingState().getLastBuildValue(); } else { + boolean forcedRebuild = + isDirty() && getDirtyBuildingState().getDirtyState() == DirtyState.FORCED_REBUILDING; // If this is a new value, or it has changed since the last build, set the version to the // current graph version. + Preconditions.checkState( + forcedRebuild || !this.lastChangedVersion.equals(version), + "Changed value but with the same version? %s %s %s", + this.lastChangedVersion, + version, + this); this.lastChangedVersion = version; this.value = value; } @@ -550,8 +558,9 @@ public synchronized Set<SkyKey> getAllRemainingDirtyDirectDeps() throws InterruptedException { Preconditions.checkState(isEvaluating(), "Not evaluating for remaining dirty? %s", this); if (isDirty()) { + DirtyState dirtyState = getDirtyBuildingState().getDirtyState(); Preconditions.checkState( - getDirtyBuildingState().getDirtyState() == DirtyState.REBUILDING, this); + dirtyState == DirtyState.REBUILDING || dirtyState == DirtyState.FORCED_REBUILDING, this); return getDirtyBuildingState().getAllRemainingDirtyDirectDeps(/*preservePosition=*/ true); } else { return ImmutableSet.of();
diff --git a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java index 9d2d811..168ad23 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java +++ b/src/main/java/com/google/devtools/build/skyframe/InvalidatingNodeVisitor.java
@@ -425,6 +425,9 @@ ArrayList<SkyKey> keysToGet = new ArrayList<>(size); for (SkyKey key : keys) { if (setToCheck.add(key)) { + Preconditions.checkState( + !isChanged || key.functionName().getHermeticity() == FunctionHermeticity.NONHERMETIC, + key); keysToGet.add(key); } }
diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java index 1e61694..e74fb1e 100644 --- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java
@@ -72,7 +72,13 @@ */ NEEDS_REBUILDING, /** A rebuilding is in progress. */ - REBUILDING + REBUILDING, + /** + * A forced rebuilding is in progress, likely because of a transient error on the previous + * build. The distinction between this and {@link #REBUILDING} is only needed for internal + * sanity checks. + */ + FORCED_REBUILDING } /**
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java index afd0279..5b847c5 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java
@@ -70,7 +70,8 @@ DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { super( graph, graphVersion, @@ -83,7 +84,8 @@ progressReceiver, graphInconsistencyReceiver, forkJoinPool, - cycleDetector); + cycleDetector, + evaluationVersionBehavior); } @Override
diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java index 14306d4..60987ac 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java
@@ -53,6 +53,7 @@ private final EventFilter storedEventFilter; private final ErrorInfoManager errorInfoManager; private final GraphInconsistencyReceiver graphInconsistencyReceiver; + private final EvaluationVersionBehavior evaluationVersionBehavior; /** * The visitor managing the thread pool. Used to enqueue parents when an entry is finished, and, @@ -86,7 +87,8 @@ storedEventFilter, errorInfoManager, graphInconsistencyReceiver, - () -> new NodeEntryVisitor(threadCount, progressReceiver, runnableMaker)); + () -> new NodeEntryVisitor(threadCount, progressReceiver, runnableMaker), + EvaluationVersionBehavior.MAX_CHILD_VERSIONS); } ParallelEvaluatorContext( @@ -101,7 +103,8 @@ ErrorInfoManager errorInfoManager, final Function<SkyKey, Runnable> runnableMaker, GraphInconsistencyReceiver graphInconsistencyReceiver, - final ForkJoinPool forkJoinPool) { + final ForkJoinPool forkJoinPool, + EvaluationVersionBehavior evaluationVersionBehavior) { this( graph, graphVersion, @@ -113,7 +116,8 @@ storedEventFilter, errorInfoManager, graphInconsistencyReceiver, - () -> new NodeEntryVisitor(forkJoinPool, progressReceiver, runnableMaker)); + () -> new NodeEntryVisitor(forkJoinPool, progressReceiver, runnableMaker), + evaluationVersionBehavior); } private ParallelEvaluatorContext( @@ -127,12 +131,14 @@ EventFilter storedEventFilter, ErrorInfoManager errorInfoManager, GraphInconsistencyReceiver graphInconsistencyReceiver, - Supplier<NodeEntryVisitor> visitorSupplier) { + Supplier<NodeEntryVisitor> visitorSupplier, + EvaluationVersionBehavior evaluationVersionBehavior) { this.graph = graph; this.graphVersion = graphVersion; this.skyFunctions = skyFunctions; this.reporter = reporter; this.graphInconsistencyReceiver = graphInconsistencyReceiver; + this.evaluationVersionBehavior = evaluationVersionBehavior; this.replayingNestedSetEventVisitor = new NestedSetVisitor<>(new NestedSetEventReceiver(reporter), emittedEventState.eventState); this.replayingNestedSetPostableVisitor = @@ -236,6 +242,10 @@ return errorInfoManager; } + EvaluationVersionBehavior getEvaluationVersionBehavior() { + return evaluationVersionBehavior; + } + /** Receives the events from the NestedSet and delegates to the reporter. */ private static class NestedSetEventReceiver implements NestedSetVisitor.Receiver<TaggedEvents> {
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index 4b2fc51..fc6e3c8 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -71,6 +71,9 @@ private SkyValue value = null; private ErrorInfo errorInfo = null; + private final FunctionHermeticity hermeticity; + @Nullable private Version maxChildVersion = null; + /** * This is not {@code null} only during cycle detection and error bubbling. The nullness of this * field is used to detect whether evaluation is in one of those special states. @@ -156,6 +159,7 @@ this.oldDeps = oldDeps; this.evaluatorContext = evaluatorContext; this.bubbleErrorInfo = null; + this.hermeticity = skyKey.functionName().getHermeticity(); this.previouslyRequestedDepsValues = batchPrefetch(skyKey, directDeps, oldDeps, /*assertDone=*/ true, skyKey); Preconditions.checkState( @@ -176,6 +180,7 @@ this.oldDeps = oldDeps; this.evaluatorContext = evaluatorContext; this.bubbleErrorInfo = Preconditions.checkNotNull(bubbleErrorInfo); + this.hermeticity = skyKey.functionName().getHermeticity(); try { this.previouslyRequestedDepsValues = batchPrefetch(skyKey, directDeps, oldDeps, /*assertDone=*/ false, skyKey); @@ -227,7 +232,8 @@ ImmutableMap.builderWithExpectedSize(batchMap.size()); for (Entry<SkyKey, ? extends NodeEntry> entry : batchMap.entrySet()) { SkyValue valueMaybeWithMetadata = entry.getValue().getValueMaybeWithMetadata(); - if (assertDone && valueMaybeWithMetadata == null) { + boolean depDone = valueMaybeWithMetadata != null; + if (assertDone && !depDone) { // A previously requested dep may have transitioned from done to dirty between when the node // was read during a previous attempt to build this node and now. Notify the graph // inconsistency receiver so that we can crash if that's unexpected. @@ -237,8 +243,10 @@ skyKey, entry.getKey(), Inconsistency.CHILD_UNDONE_FOR_BUILDING_NODE); throw new UndonePreviouslyRequestedDep(entry.getKey()); } - depValuesBuilder.put( - entry.getKey(), valueMaybeWithMetadata == null ? NULL_MARKER : valueMaybeWithMetadata); + depValuesBuilder.put(entry.getKey(), !depDone ? NULL_MARKER : valueMaybeWithMetadata); + if (depDone) { + maybeUpdateMaxChildVersion(entry.getValue()); + } } return depValuesBuilder.build(); } @@ -323,6 +331,7 @@ triState == DependencyState.DONE, "%s %s %s", skyKey, triState, errorInfo); state.addTemporaryDirectDeps(GroupedListHelper.create(ErrorTransienceValue.KEY)); state.signalDep(); + maxChildVersion = evaluatorContext.getGraphVersion(); } this.errorInfo = Preconditions.checkNotNull(errorInfo, skyKey); @@ -367,9 +376,13 @@ Map<SkyKey, ? extends NodeEntry> missingEntries = evaluatorContext.getBatchValues(skyKey, Reason.DEP_REQUESTED, missingKeys); for (SkyKey key : missingKeys) { - SkyValue valueOrNullMarker = getValueOrNullMarker(missingEntries.get(key)); + NodeEntry depEntry = missingEntries.get(key); + SkyValue valueOrNullMarker = getValueOrNullMarker(depEntry); result.put(key, valueOrNullMarker); newlyRequestedDepsValues.put(key, valueOrNullMarker); + if (valueOrNullMarker != NULL_MARKER) { + maybeUpdateMaxChildVersion(depEntry); + } } return result; } @@ -425,7 +438,8 @@ Map<SkyKey, ? extends NodeEntry> missingEntries = evaluatorContext.getBatchValues(skyKey, Reason.DEP_REQUESTED, missingKeys); for (SkyKey key : missingKeys) { - SkyValue valueOrNullMarker = getValueOrNullMarker(missingEntries.get(key)); + NodeEntry depEntry = missingEntries.get(key); + SkyValue valueOrNullMarker = getValueOrNullMarker(depEntry); newlyRequestedDepsValues.put(key, valueOrNullMarker); if (valueOrNullMarker == NULL_MARKER) { // TODO(mschaller): handle registered deps that transitioned from done to dirty during eval @@ -435,6 +449,7 @@ Preconditions.checkState(!assertDone, "%s had not done: %s", skyKey, key); continue; } + maybeUpdateMaxChildVersion(depEntry); result.add(valueOrNullMarker); } return result; @@ -686,7 +701,6 @@ NestedSet<Postable> posts = buildPosts(primaryEntry, /*expectDoneDeps=*/ true); NestedSet<TaggedEvents> events = buildEvents(primaryEntry, /*expectDoneDeps=*/ true); - Version valueVersion; SkyValue valueWithMetadata; if (value == null) { Preconditions.checkNotNull(errorInfo, "%s %s", skyKey, primaryEntry); @@ -697,42 +711,54 @@ enqueueParents == EnqueueParentBehavior.ENQUEUE, "%s %s", skyKey, primaryEntry); valueWithMetadata = ValueWithMetadata.normal(value, errorInfo, events, posts); } + GroupedList<SkyKey> temporaryDirectDeps = primaryEntry.getTemporaryDirectDeps(); if (!oldDeps.isEmpty()) { // Remove the rdep on this entry for each of its old deps that is no longer a direct dep. - Set<SkyKey> depsToRemove = - Sets.difference(oldDeps, primaryEntry.getTemporaryDirectDeps().toSet()); + Set<SkyKey> depsToRemove = Sets.difference(oldDeps, temporaryDirectDeps.toSet()); Collection<? extends NodeEntry> oldDepEntries = evaluatorContext.getGraph().getBatch(skyKey, Reason.RDEP_REMOVAL, depsToRemove).values(); for (NodeEntry oldDepEntry : oldDepEntries) { oldDepEntry.removeReverseDep(skyKey); } } + Version evaluationVersion = maxChildVersion; + if (evaluatorContext.getEvaluationVersionBehavior() == EvaluationVersionBehavior.GRAPH_VERSION + || hermeticity == FunctionHermeticity.NONHERMETIC) { + evaluationVersion = evaluatorContext.getGraphVersion(); + } else if (bubbleErrorInfo != null) { + // Cycles can lead to a state where the versions of done children don't accurately reflect the + // state that led to this node's value. Be conservative then. + evaluationVersion = evaluatorContext.getGraphVersion(); + } else if (evaluationVersion == null) { + Preconditions.checkState( + temporaryDirectDeps.isEmpty(), + "No max child version found, but have direct deps: %s %s", + skyKey, + primaryEntry); + evaluationVersion = evaluatorContext.getGraphVersion(); + } + Version previousVersion = primaryEntry.getVersion(); // If this entry is dirty, setValue may not actually change it, if it determines that // the data being written now is the same as the data already present in the entry. - // We could consider using max(childVersions) here instead of graphVersion. When full - // versioning is implemented, this would allow evaluation at a version between - // max(childVersions) and graphVersion to re-use this result. - Set<SkyKey> reverseDeps = - primaryEntry.setValue(valueWithMetadata, evaluatorContext.getGraphVersion()); - // Note that if this update didn't actually change the value entry, this version may not - // be the graph version. - valueVersion = primaryEntry.getVersion(); + Set<SkyKey> reverseDeps = primaryEntry.setValue(valueWithMetadata, evaluationVersion); + // Note that if this update didn't actually change the entry, this version may not be + // evaluationVersion. + Version currentVersion = primaryEntry.getVersion(); Preconditions.checkState( - valueVersion.atMost(evaluatorContext.getGraphVersion()), - "%s should be at most %s in the version partial ordering", - valueVersion, + currentVersion.atMost(evaluationVersion), + "%s should be at most %s in the version partial ordering (graph version %s)", + currentVersion, + evaluationVersion, evaluatorContext.getGraphVersion()); - // Tell the receiver that this value was built. If valueVersion.equals(graphVersion), it was - // evaluated this run, and so was changed. Otherwise, it is less than graphVersion, by the - // Preconditions check above, and was not actually changed this run -- when it was written + // Tell the receiver that this value was built. If currentVersion.equals(evaluationVersion), it + // was evaluated this run, and so was changed. Otherwise, it is less than evaluationVersion, by + // the Preconditions check above, and was not actually changed this run -- when it was written // above, its version stayed below this update's version, so its value remains the same. // We use a SkyValueSupplier here because it keeps a reference to the entry, allowing for // the receiver to be confident that the entry is readily accessible in memory. EvaluationState evaluationState = - valueVersion.equals(evaluatorContext.getGraphVersion()) - ? EvaluationState.BUILT - : EvaluationState.CLEAN; + currentVersion.equals(previousVersion) ? EvaluationState.CLEAN : EvaluationState.BUILT; evaluatorContext .getProgressReceiver() .evaluated( @@ -742,7 +768,7 @@ evaluationState); evaluatorContext.signalValuesAndEnqueueIfReady( - skyKey, reverseDeps, valueVersion, enqueueParents); + skyKey, reverseDeps, currentVersion, enqueueParents); evaluatorContext.getReplayingNestedSetPostableVisitor().visit(posts); evaluatorContext.getReplayingNestedSetEventVisitor().visit(events); @@ -777,11 +803,25 @@ if (!previouslyRequestedDepsValues.containsKey(key)) { newlyRequestedDeps.add(key); newlyRegisteredDeps.add(key); + // Be conservative with these value-not-retrieved deps: assume they have the highest + // possible version. + maxChildVersion = evaluatorContext.getGraphVersion(); } } newlyRequestedDeps.endGroup(); } + private void maybeUpdateMaxChildVersion(NodeEntry depEntry) { + if (hermeticity == FunctionHermeticity.HERMETIC + && evaluatorContext.getEvaluationVersionBehavior() + == EvaluationVersionBehavior.MAX_CHILD_VERSIONS) { + Version depVersion = depEntry.getVersion(); + if (maxChildVersion == null || maxChildVersion.atMost(depVersion)) { + maxChildVersion = depVersion; + } + } + } + /** Thrown during environment construction if a previously requested dep is no longer done. */ static class UndonePreviouslyRequestedDep extends Exception { private final SkyKey depKey;
diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java index 3d6ece1..107181e 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionName.java
@@ -31,15 +31,29 @@ * argument. */ // Needs to be after the cache is initialized. - public static final SkyFunctionName FOR_TESTING = SkyFunctionName.create("FOR_TESTING"); + public static final SkyFunctionName FOR_TESTING = SkyFunctionName.createHermetic("FOR_TESTING"); - /** Create a SkyFunctionName identified by {@code name}. */ - public static SkyFunctionName create(String name) { - return create(name, ShareabilityOfValue.ALWAYS); + /** + * Create a SkyFunctionName identified by {@code name} whose evaluation is non-hermetic (its value + * may not be a pure function of its dependencies. Only use this if the evaluation explicitly + * consumes data outside of Skyframe, or if the node can be directly invalidated (as opposed to + * transitively invalidated). + */ + public static SkyFunctionName createNonHermetic(String name) { + return create(name, ShareabilityOfValue.ALWAYS, FunctionHermeticity.NONHERMETIC); } - public static SkyFunctionName create(String name, ShareabilityOfValue shareabilityOfValue) { - SkyFunctionName result = new SkyFunctionName(name, shareabilityOfValue); + /** + * Creates a SkyFunctionName identified by {@code name} whose evaluation is hermetic (guaranteed + * to be a deterministic function of its dependencies, not doing any external operations). + */ + public static SkyFunctionName createHermetic(String name) { + return create(name, ShareabilityOfValue.ALWAYS, FunctionHermeticity.HERMETIC); + } + + public static SkyFunctionName create( + String name, ShareabilityOfValue shareabilityOfValue, FunctionHermeticity hermeticity) { + SkyFunctionName result = new SkyFunctionName(name, shareabilityOfValue, hermeticity); SkyFunctionName cached; try { cached = interner.get(new NameOnlyWrapper(result), () -> result); @@ -56,10 +70,13 @@ private final String name; private final ShareabilityOfValue shareabilityOfValue; + private final FunctionHermeticity hermeticity; - private SkyFunctionName(String name, ShareabilityOfValue shareabilityOfValue) { + private SkyFunctionName( + String name, ShareabilityOfValue shareabilityOfValue, FunctionHermeticity hermeticity) { this.name = name; this.shareabilityOfValue = shareabilityOfValue; + this.hermeticity = hermeticity; } public String getName() { @@ -70,6 +87,10 @@ return shareabilityOfValue; } + public FunctionHermeticity getHermeticity() { + return hermeticity; + } + @Override public String toString() { return name
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java b/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java new file mode 100644 index 0000000..221f44f --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/actions/util/InjectedActionLookupKey.java
@@ -0,0 +1,54 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.actions.util; + +import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; +import com.google.devtools.build.skyframe.SkyFunctionName; + +/** + * An {@link ActionLookupKey} with a non-hermetic {@link SkyFunctionName} so that its value can be + * directly injected during tests. + */ +public class InjectedActionLookupKey extends ActionLookupKey { + public static final SkyFunctionName INJECTED_ACTION_LOOKUP = + SkyFunctionName.createNonHermetic("INJECTED_ACTION_LOOKUP"); + + private final String name; + + public InjectedActionLookupKey(String name) { + this.name = name; + } + + @Override + public SkyFunctionName functionName() { + return INJECTED_ACTION_LOOKUP; + } + + @Override + public int hashCode() { + return name.hashCode(); + } + + @Override + public boolean equals(Object obj) { + return obj instanceof InjectedActionLookupKey + && ((InjectedActionLookupKey) obj).name.equals(name); + } + + @Override + public String toString() { + return "InjectedActionLookupKey:" + name; + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageUtilTest.java b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageUtilTest.java index 709d9dc..f8d4685 100644 --- a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageUtilTest.java +++ b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageUtilTest.java
@@ -229,7 +229,7 @@ } private static final SkyFunctionName GET_RULE_BY_NAME_FUNCTION = - SkyFunctionName.create("GET_RULE_BY_NAME"); + SkyFunctionName.createHermetic("GET_RULE_BY_NAME"); @AutoValue abstract static class GetRuleByNameValue implements SkyValue { @@ -277,7 +277,7 @@ } private static final SkyFunctionName GET_REGISTERED_TOOLCHAINS_FUNCTION = - SkyFunctionName.create("GET_REGISTERED_TOOLCHAINS"); + SkyFunctionName.createHermetic("GET_REGISTERED_TOOLCHAINS"); @AutoValue abstract static class GetRegisteredToolchainsValue implements SkyValue { @@ -324,7 +324,7 @@ } private static final SkyFunctionName GET_REGISTERED_EXECUTION_PLATFORMS_FUNCTION = - SkyFunctionName.create("GET_REGISTERED_EXECUTION_PLATFORMS_FUNCTION"); + SkyFunctionName.createHermetic("GET_REGISTERED_EXECUTION_PLATFORMS_FUNCTION"); @AutoValue abstract static class GetRegisteredExecutionPlatformsValue implements SkyValue {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index c8f9da1..786fe20 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.ActionTemplate; import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; @@ -36,11 +37,11 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate; import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate.OutputPathMapper; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.packages.Package; @@ -192,8 +193,7 @@ } } - private static final ConfiguredTargetKey CTKEY = - ConfiguredTargetKey.of(Label.parseAbsoluteUnchecked("//foo:foo"), null); + private static final ActionLookupValue.ActionLookupKey CTKEY = new InjectedActionLookupKey("key"); private List<Action> evaluate(SpawnActionTemplate spawnActionTemplate) throws Exception { ConfiguredTargetValue ctValue = createConfiguredTargetValue(spawnActionTemplate);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java index 5a03f3e..b0deb69 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -52,7 +53,7 @@ import org.junit.Before; abstract class ArtifactFunctionTestCase { - static final ActionLookupKey ALL_OWNER = new SingletonActionLookupKey(); + static final ActionLookupKey ALL_OWNER = new InjectedActionLookupKey("all_owner"); protected LinkedHashSet<ActionAnalysisMetadata> actions; protected boolean fastDigest = false; @@ -159,13 +160,6 @@ } } - private static class SingletonActionLookupKey extends ActionLookupKey { - @Override - public SkyFunctionName functionName() { - return SkyFunctions.CONFIGURED_TARGET; - } - } - /** InMemoryFileSystem that can pretend to do a fast digest. */ protected class CustomInMemoryFs extends InMemoryFileSystem { @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java index 3109126..a813512 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
@@ -95,7 +95,7 @@ */ private static class ComputeDependenciesFunction implements SkyFunction { static final SkyFunctionName SKYFUNCTION_NAME = - SkyFunctionName.create("CONFIGURED_TARGET_FUNCTION_COMPUTE_DEPENDENCIES"); + SkyFunctionName.createHermetic("CONFIGURED_TARGET_FUNCTION_COMPUTE_DEPENDENCIES"); private final LateBoundStateProvider stateProvider; private final Supplier<BuildOptions> buildOptionsSupplier;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index 3d9971d..5750221 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -23,7 +23,6 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.FileStateValue; -import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.Label; @@ -202,13 +201,11 @@ // has a child directory "baz". fs.stubStat(bazDir, null); RootedPath barDirRootedPath = RootedPath.toRootedPath(pkgRoot, barDir); - FileStateValue barDirFileStateValue = FileStateValue.create(barDirRootedPath, tsgm); - FileValue barDirFileValue = FileValue.value(barDirRootedPath, barDirFileStateValue, - barDirRootedPath, barDirFileStateValue); - DirectoryListingValue barDirListing = DirectoryListingValue.value(barDirRootedPath, - barDirFileValue, DirectoryListingStateValue.create(ImmutableList.of( - new Dirent("baz", Dirent.Type.DIRECTORY)))); - differencer.inject(ImmutableMap.of(DirectoryListingValue.key(barDirRootedPath), barDirListing)); + differencer.inject( + ImmutableMap.of( + DirectoryListingStateValue.key(barDirRootedPath), + DirectoryListingStateValue.create( + ImmutableList.of(new Dirent("baz", Dirent.Type.DIRECTORY))))); SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); String expectedMessage = "/workspace/foo/bar/baz is no longer an existing directory"; EvaluationResult<PackageValue> result = SkyframeExecutorTestUtils.evaluate(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index b75009d..4315e34 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -53,6 +53,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; +import com.google.devtools.build.skyframe.AbstractSkyKey; import com.google.devtools.build.skyframe.ErrorInfo; import com.google.devtools.build.skyframe.EvaluationProgressReceiver; import com.google.devtools.build.skyframe.EvaluationResult; @@ -91,12 +92,10 @@ private SequentialBuildDriver driver; private RecordingDifferencer differencer; private AtomicReference<PathPackageLocator> pkgLocator; - private ArtifactFakeFunction artifactFakeFunction; @Before - public final void setUp() throws Exception { + public final void setUp() { AnalysisMock analysisMock = AnalysisMock.get(); - artifactFakeFunction = new ArtifactFakeFunction(); pkgLocator = new AtomicReference<>( new PathPackageLocator( @@ -154,7 +153,11 @@ new FileSymlinkInfiniteExpansionUniquenessFunction()); skyFunctions.put( SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction()); - skyFunctions.put(Artifact.ARTIFACT, artifactFakeFunction); + // We use a non-hermetic key to allow us to invalidate the proper artifacts on rebuilds. We + // could have the artifact depend on the corresponding FileValue, but that would not cover the + // case of a generated directory, which we have test coverage for. + skyFunctions.put(Artifact.ARTIFACT, new ArtifactFakeFunction()); + skyFunctions.put(NONHERMETIC_ARTIFACT, new NonHermeticArtifactFakeFunction()); progressReceiver = new RecordingEvaluationProgressReceiver(); differencer = new SequencedRecordingDifferencer(); @@ -307,9 +310,10 @@ } private void appendToFile(Artifact file, String content) throws Exception { - SkyKey key = file.isSourceArtifact() - ? FileStateValue.key(rootedPath(file)) - : ArtifactSkyKey.key(file, true); + SkyKey key = + file.isSourceArtifact() + ? FileStateValue.key(rootedPath(file)) + : new NonHermeticArtifactSkyKey(ArtifactSkyKey.key(file, true)); appendToFile(rootedPath(file), key, content); } @@ -319,7 +323,8 @@ private void invalidateOutputArtifact(Artifact output) { assertThat(output.isSourceArtifact()).isFalse(); - differencer.invalidate(ImmutableList.of(ArtifactSkyKey.key(output, true))); + differencer.invalidate( + ImmutableList.of(new NonHermeticArtifactSkyKey(ArtifactSkyKey.key(output, true)))); } private void invalidateDirectory(Artifact directoryArtifact) { @@ -540,6 +545,9 @@ assertTraversalOfDirectory(sourceArtifact("dir")); } + // Note that in actual Bazel derived artifact directories are not checked for modifications on + // incremental builds, so this test is testing a feature that Bazel does not have. It's included + // aspirationally. @Test public void testTraversalOfGeneratedDirectory() throws Exception { assertTraversalOfDirectory(derivedArtifact("dir")); @@ -891,7 +899,7 @@ assertThat(error.getException()).hasMessageThat().contains("Symlink cycle"); } - private static class ArtifactFakeFunction implements SkyFunction { + private static class NonHermeticArtifactFakeFunction implements SkyFunction { @Nullable @Override public SkyValue compute(SkyKey skyKey, Environment env) @@ -911,4 +919,33 @@ return null; } } + + private static class ArtifactFakeFunction implements SkyFunction { + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws SkyFunctionException, InterruptedException { + return env.getValue(new NonHermeticArtifactSkyKey((ArtifactSkyKey) skyKey)); + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + } + + private static class NonHermeticArtifactSkyKey extends AbstractSkyKey<ArtifactSkyKey> { + private NonHermeticArtifactSkyKey(ArtifactSkyKey arg) { + super(arg); + } + + @Override + public SkyFunctionName functionName() { + return NONHERMETIC_ARTIFACT; + } + } + + private static final SkyFunctionName NONHERMETIC_ARTIFACT = + SkyFunctionName.createNonHermetic("NONHERMETIC_ARTIFACT"); }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 5633ab5..c0e3317 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -55,6 +55,7 @@ import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; import com.google.devtools.build.lib.actions.util.DummyExecutor; +import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -118,7 +119,7 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { @AutoCodec protected static final ActionLookupValue.ActionLookupKey ACTION_LOOKUP_KEY = - new SingletonActionLookupKey(); + new InjectedActionLookupKey("action_lookup_key"); protected static final Predicate<Action> ALWAYS_EXECUTE_FILTER = Predicates.alwaysTrue(); protected static final String CYCLE_MSG = "Yarrrr, there be a cycle up in here"; @@ -514,13 +515,6 @@ } } - static class SingletonActionLookupKey extends ActionLookupValue.ActionLookupKey { - @Override - public SkyFunctionName functionName() { - return SkyFunctions.CONFIGURED_TARGET; - } - } - private class DelegatingActionTemplateExpansionFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java index a4b727e..5e7f349 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
@@ -23,26 +23,37 @@ import com.google.common.collect.ImmutableMap; import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.actions.Actions.GeneratingActions; +import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.platform.ToolchainTestCase; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.skyframe.EvaluationResult; import com.google.devtools.build.skyframe.SkyKey; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mockito; /** Tests for {@link ToolchainResolutionValue} and {@link ToolchainResolutionFunction}. */ @RunWith(JUnit4.class) public class ToolchainResolutionFunctionTest extends ToolchainTestCase { - private static final ConfiguredTargetKey LINUX_CTKEY = - ConfiguredTargetKey.of(Label.parseAbsoluteUnchecked("//linux:key"), null, false); - private static final ConfiguredTargetKey MAC_CTKEY = - ConfiguredTargetKey.of(Label.parseAbsoluteUnchecked("//mac:key"), null, false); + @AutoCodec @AutoCodec.VisibleForSerialization + static final ConfiguredTargetKey LINUX_CTKEY = Mockito.mock(ConfiguredTargetKey.class); + + @AutoCodec @AutoCodec.VisibleForSerialization + static final ConfiguredTargetKey MAC_CTKEY = Mockito.mock(ConfiguredTargetKey.class); + + static { + Mockito.when(LINUX_CTKEY.functionName()) + .thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP); + Mockito.when(MAC_CTKEY.functionName()) + .thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP); + } private static ConfiguredTargetValue createConfiguredTargetValue( ConfiguredTarget configuredTarget) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java index 4a78f59..b431612 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java
@@ -420,7 +420,7 @@ // Calls ToolchainUtil.createToolchainContext. private static final SkyFunctionName CREATE_TOOLCHAIN_CONTEXT_FUNCTION = - SkyFunctionName.create("CREATE_TOOLCHAIN_CONTEXT_FUNCTION"); + SkyFunctionName.createHermetic("CREATE_TOOLCHAIN_CONTEXT_FUNCTION"); @AutoValue abstract static class CreateToolchainContextKey implements SkyKey {
diff --git a/src/test/java/com/google/devtools/build/skyframe/BUILD b/src/test/java/com/google/devtools/build/skyframe/BUILD index 712dc50..b3808d2 100644 --- a/src/test/java/com/google/devtools/build/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/skyframe/BUILD
@@ -58,6 +58,7 @@ "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/test/java/com/google/devtools/build/lib:testutil", + "//third_party:auto_value", "//third_party:guava", "//third_party:guava-testlib", "//third_party:jsr305",
diff --git a/src/test/java/com/google/devtools/build/skyframe/CyclesReporterTest.java b/src/test/java/com/google/devtools/build/skyframe/CyclesReporterTest.java index 7cac95f..866fdd7 100644 --- a/src/test/java/com/google/devtools/build/skyframe/CyclesReporterTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/CyclesReporterTest.java
@@ -27,7 +27,7 @@ @RunWith(JUnit4.class) public class CyclesReporterTest { - private static final SkyKey DUMMY_KEY = () -> SkyFunctionName.create("func"); + private static final SkyKey DUMMY_KEY = () -> SkyFunctionName.createHermetic("func"); @Test public void nullEventHandler() {
diff --git a/src/test/java/com/google/devtools/build/skyframe/DeterministicHelper.java b/src/test/java/com/google/devtools/build/skyframe/DeterministicHelper.java index 632f23d..524ea9b 100644 --- a/src/test/java/com/google/devtools/build/skyframe/DeterministicHelper.java +++ b/src/test/java/com/google/devtools/build/skyframe/DeterministicHelper.java
@@ -54,13 +54,10 @@ } } + /** Compare using SkyKey argument first, so that tests can easily order keys. */ private static final Comparator<SkyKey> ALPHABETICAL_SKYKEY_COMPARATOR = - new Comparator<SkyKey>() { - @Override - public int compare(SkyKey o1, SkyKey o2) { - return o1.toString().compareTo(o2.toString()); - } - }; + Comparator.<SkyKey, String>comparing(key -> key.argument().toString()) + .thenComparing(key -> key.functionName().toString()); DeterministicHelper(Listener listener) { super(listener);
diff --git a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java index d9ebca5..223b14b 100644 --- a/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/EagerInvalidatorTest.java
@@ -185,19 +185,20 @@ } }); graph = new InMemoryGraphImpl(); - set("a", "a"); - set("b", "b"); - tester.getOrCreate("ab").addDependency("a").addDependency("b") - .setComputedValue(CONCATENATE); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + SkyKey bKey = GraphTester.nonHermeticKey("b"); + tester.set(aKey, new StringValue("a")); + tester.set(bKey, new StringValue("b")); + tester.getOrCreate("ab").addDependency(aKey).addDependency(bKey).setComputedValue(CONCATENATE); assertValueValue("ab", "ab"); - set("a", "c"); - invalidateWithoutError(receiver, skyKey("a")); - assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab")); + tester.set(aKey, new StringValue("c")); + invalidateWithoutError(receiver, aKey); + assertThat(invalidated).containsExactly(aKey, skyKey("ab")); assertValueValue("ab", "cb"); - set("b", "d"); - invalidateWithoutError(receiver, skyKey("b")); - assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab"), skyKey("b")); + tester.set(bKey, new StringValue("d")); + invalidateWithoutError(receiver, bKey); + assertThat(invalidated).containsExactly(aKey, skyKey("ab"), bKey); } @Test @@ -215,15 +216,16 @@ // Given a graph consisting of two nodes, "a" and "ab" such that "ab" depends on "a", // And given "ab" is in error, graph = new InMemoryGraphImpl(); - set("a", "a"); - tester.getOrCreate("ab").addDependency("a").setHasError(true); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + tester.set(aKey, new StringValue("a")); + tester.getOrCreate("ab").addDependency(aKey).setHasError(true); eval(false, skyKey("ab")); // When "a" is invalidated, - invalidateWithoutError(receiver, skyKey("a")); + invalidateWithoutError(receiver, aKey); // Then the invalidation receiver is notified of both "a" and "ab"'s invalidations. - assertThat(invalidated).containsExactly(skyKey("a"), skyKey("ab")); + assertThat(invalidated).containsExactly(aKey, skyKey("ab")); // Note that this behavior isn't strictly required for correctness. This test is // meant to document current behavior and protect against programming error. @@ -241,20 +243,22 @@ } }); graph = new InMemoryGraphImpl(); - invalidateWithoutError(receiver, skyKey("a")); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + invalidateWithoutError(receiver, aKey); assertThat(invalidated).isEmpty(); - set("a", "a"); - assertValueValue("a", "a"); - invalidateWithoutError(receiver, skyKey("b")); + tester.set(aKey, new StringValue("a")); + StringValue value = (StringValue) eval(false, aKey); + assertThat(value.getValue()).isEqualTo("a"); + invalidateWithoutError(receiver, GraphTester.nonHermeticKey("b")); assertThat(invalidated).isEmpty(); } @Test public void invalidatedValuesAreGCedAsExpected() throws Exception { - SkyKey key = GraphTester.skyKey("a"); + SkyKey key = GraphTester.nonHermeticKey("a"); HeavyValue heavyValue = new HeavyValue(); WeakReference<HeavyValue> weakRef = new WeakReference<>(heavyValue); - tester.set("a", heavyValue); + tester.set(key, heavyValue); graph = new InMemoryGraphImpl(); eval(false, key); @@ -278,30 +282,34 @@ set("a", "a"); set("b", "b"); set("c", "c"); - tester.getOrCreate("ab").addDependency("a").addDependency("b").setComputedValue(CONCATENATE); + SkyKey abKey = GraphTester.nonHermeticKey("ab"); + tester.getOrCreate(abKey).addDependency("a").addDependency("b").setComputedValue(CONCATENATE); tester.getOrCreate("bc").addDependency("b").addDependency("c").setComputedValue(CONCATENATE); - tester.getOrCreate("ab_c").addDependency("ab").addDependency("c") + tester + .getOrCreate("ab_c") + .addDependency(abKey) + .addDependency("c") .setComputedValue(CONCATENATE); eval(false, skyKey("ab_c"), skyKey("bc")); assertThat(graph.get(null, Reason.OTHER, skyKey("a")).getReverseDepsForDoneEntry()) - .containsExactly(skyKey("ab")); + .containsExactly(abKey); assertThat(graph.get(null, Reason.OTHER, skyKey("b")).getReverseDepsForDoneEntry()) - .containsExactly(skyKey("ab"), skyKey("bc")); + .containsExactly(abKey, skyKey("bc")); assertThat(graph.get(null, Reason.OTHER, skyKey("c")).getReverseDepsForDoneEntry()) .containsExactly(skyKey("ab_c"), skyKey("bc")); - invalidateWithoutError(new DirtyTrackingProgressReceiver(null), skyKey("ab")); + invalidateWithoutError(new DirtyTrackingProgressReceiver(null), abKey); eval(false); // The graph values should be gone. - assertThat(isInvalidated(skyKey("ab"))).isTrue(); + assertThat(isInvalidated(abKey)).isTrue(); assertThat(isInvalidated(skyKey("abc"))).isTrue(); // The reverse deps to ab and ab_c should have been removed if reverse deps are cleared. Set<SkyKey> reverseDeps = new HashSet<>(); if (reverseDepsPresent()) { - reverseDeps.add(skyKey("ab")); + reverseDeps.add(abKey); } assertThat(graph.get(null, Reason.OTHER, skyKey("a")).getReverseDepsForDoneEntry()) .containsExactlyElementsIn(reverseDeps); @@ -321,9 +329,9 @@ public void interruptChild() throws Exception { graph = new InMemoryGraphImpl(); int numValues = 50; // More values than the invalidator has threads. - final SkyKey[] family = new SkyKey[numValues]; - final SkyKey child = GraphTester.skyKey("child"); - final StringValue childValue = new StringValue("child"); + SkyKey[] family = new SkyKey[numValues]; + SkyKey child = GraphTester.nonHermeticKey("child"); + StringValue childValue = new StringValue("child"); tester.set(child, childValue); family[0] = child; for (int i = 1; i < numValues; i++) { @@ -400,11 +408,12 @@ SkyKey[] values = new SkyKey[size]; for (int i = 0; i < size; i++) { String iString = Integer.toString(i); - SkyKey iKey = GraphTester.toSkyKey(iString); + SkyKey iKey = GraphTester.nonHermeticKey(iString); + tester.set(iKey, new StringValue(iString)); set(iString, iString); for (int j = 0; j < i; j++) { if (random.nextInt(3) == 0) { - tester.getOrCreate(iKey).addDependency(Integer.toString(j)); + tester.getOrCreate(iKey).addDependency(GraphTester.nonHermeticKey(Integer.toString(j))); } } values[i] = iKey; @@ -488,11 +497,12 @@ protected void setupInvalidatableGraph() throws Exception { graph = new InMemoryGraphImpl(); - set("a", "a"); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + tester.set(aKey, new StringValue("a")); set("b", "b"); - tester.getOrCreate("ab").addDependency("a").addDependency("b").setComputedValue(CONCATENATE); + tester.getOrCreate("ab").addDependency(aKey).addDependency("b").setComputedValue(CONCATENATE); assertValueValue("ab", "ab"); - set("a", "c"); + tester.set(aKey, new StringValue("c")); } private static class HeavyValue implements SkyValue { @@ -549,20 +559,15 @@ new EvaluationProgressReceiver.NullEvaluationProgressReceiver()); // Dirty the node, and ensure that the tracker is aware of it: - Iterable<SkyKey> diff1 = ImmutableList.of(skyKey("a")); + ImmutableList<SkyKey> diff = ImmutableList.of(GraphTester.nonHermeticKey("a")); InvalidationState state1 = new DirtyingInvalidationState(); Preconditions.checkNotNull( EagerInvalidator.createInvalidatingVisitorIfNeeded( - graph, - diff1, - receiver, - state1, - AbstractQueueVisitor.EXECUTOR_FACTORY)) + graph, diff, receiver, state1, AbstractQueueVisitor.EXECUTOR_FACTORY)) .run(); - assertThat(receiver.getUnenqueuedDirtyKeys()).containsExactly(skyKey("a"), skyKey("ab")); + assertThat(receiver.getUnenqueuedDirtyKeys()).containsExactly(diff.get(0), skyKey("ab")); // Delete the node, and ensure that the tracker is no longer tracking it: - Iterable<SkyKey> diff = ImmutableList.of(skyKey("a")); Preconditions.checkNotNull(EagerInvalidator.createDeletingVisitorIfNeeded(graph, diff, receiver, state, true)).run(); assertThat(receiver.getUnenqueuedDirtyKeys()).isEmpty(); @@ -624,7 +629,7 @@ new EvaluationProgressReceiver.NullEvaluationProgressReceiver()); // Dirty the node, and ensure that the tracker is aware of it: - invalidate(graph, receiver, skyKey("a")); + invalidate(graph, receiver, GraphTester.nonHermeticKey("a")); assertThat(receiver.getUnenqueuedDirtyKeys()).hasSize(2); } }
diff --git a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java index 9b129df..b688880 100644 --- a/src/test/java/com/google/devtools/build/skyframe/GraphTester.java +++ b/src/test/java/com/google/devtools/build/skyframe/GraphTester.java
@@ -57,6 +57,7 @@ public GraphTester() { functionMap.put(NODE_TYPE, new DelegatingFunction()); + functionMap.put(FOR_TESTING_NONHERMETIC, new DelegatingFunction()); } public TestFunction getOrCreate(String name) { @@ -170,6 +171,10 @@ return Key.create(key); } + public static NonHermeticKey nonHermeticKey(String key) { + return NonHermeticKey.create(key); + } + /** A value in the testing graph that is constructed in the tester. */ public static class TestFunction { // TODO(bazel-team): We could use a multiset here to simulate multi-pass dependency discovery. @@ -421,11 +426,12 @@ static class Key extends AbstractSkyKey<String> { private static final Interner<Key> interner = BlazeInterners.newWeakInterner(); - @AutoCodec.VisibleForSerialization - Key(String arg) { + private Key(String arg) { super(arg); } + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator static Key create(String arg) { return interner.intern(new Key(arg)); } @@ -435,4 +441,28 @@ return SkyFunctionName.FOR_TESTING; } } + + @AutoCodec.VisibleForSerialization + @AutoCodec + static class NonHermeticKey extends AbstractSkyKey<String> { + private static final Interner<NonHermeticKey> interner = BlazeInterners.newWeakInterner(); + + private NonHermeticKey(String arg) { + super(arg); + } + + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static NonHermeticKey create(String arg) { + return interner.intern(new NonHermeticKey(arg)); + } + + @Override + public SkyFunctionName functionName() { + return FOR_TESTING_NONHERMETIC; + } + } + + private static final SkyFunctionName FOR_TESTING_NONHERMETIC = + SkyFunctionName.createNonHermetic("FOR_TESTING_NONHERMETIC"); }
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java index 3112cff..d10a243 100644 --- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -24,12 +24,14 @@ import static com.google.devtools.build.skyframe.GraphTester.skyKey; import static org.junit.Assert.fail; +import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.common.eventbus.EventBus; import com.google.common.testing.GcFinalization; import com.google.common.util.concurrent.Uninterruptibles; @@ -41,6 +43,7 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.testutil.TestThread; import com.google.devtools.build.lib.testutil.TestUtils; +import com.google.devtools.build.skyframe.GraphInconsistencyReceiver.Inconsistency; import com.google.devtools.build.skyframe.GraphTester.NotComparableStringValue; import com.google.devtools.build.skyframe.GraphTester.StringValue; import com.google.devtools.build.skyframe.GraphTester.TestFunction; @@ -152,6 +155,25 @@ return GraphTester.toSkyKey(name); } + /** + * Equips {@link #tester} with a {@link GraphInconsistencyReceiver} that tolerates and tracks + * inconsistencies. + * + * <p>Returns a concurrent {@link Set} containing {@link InconsistencyData}s discovered during + * evaluation. Callers should assert the desired properties on the returned set. + */ + protected Set<InconsistencyData> setupGraphInconsistencyReceiver() { + Set<InconsistencyData> inconsistencies = Sets.newConcurrentHashSet(); + tester.setGraphInconsistencyReceiver( + (key, otherKey, inconsistency) -> + Preconditions.checkState( + inconsistencies.add(InconsistencyData.create(key, otherKey, inconsistency)))); + // #initialize must be called after setting the GraphInconsistencyReceiver for the receiver to + // be registered with the test's memoizing evaluator. + tester.initialize(/*keepEdges=*/ true); + return inconsistencies; + } + @Test public void smoke() throws Exception { tester.set("x", new StringValue("y")); @@ -414,18 +436,23 @@ @Test public void deleteOldNodesTest() throws Exception { - tester.getOrCreate("top").setComputedValue(CONCATENATE).addDependency("d1").addDependency("d2"); + SkyKey d2Key = GraphTester.nonHermeticKey("d2"); + tester + .getOrCreate("top") + .setComputedValue(CONCATENATE) + .addDependency("d1") + .addDependency(d2Key); tester.set("d1", new StringValue("one")); - tester.set("d2", new StringValue("two")); + tester.set(d2Key, new StringValue("two")); tester.eval(true, "top"); - tester.set("d2", new StringValue("three")); + tester.set(d2Key, new StringValue("three")); tester.invalidate(); - tester.eval(true, "d2"); + tester.eval(true, d2Key); // The graph now contains the three above nodes (and ERROR_TRANSIENCE). assertThat(tester.evaluator.getValues().keySet()) - .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY); + .containsExactly(skyKey("top"), skyKey("d1"), d2Key, ErrorTransienceValue.KEY); String[] noKeys = {}; tester.evaluator.deleteDirty(2); @@ -433,19 +460,19 @@ // The top node's value is dirty, but less than two generations old, so it wasn't deleted. assertThat(tester.evaluator.getValues().keySet()) - .containsExactly(skyKey("top"), skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY); + .containsExactly(skyKey("top"), skyKey("d1"), d2Key, ErrorTransienceValue.KEY); tester.evaluator.deleteDirty(2); tester.eval(true, noKeys); // The top node's value was dirty, and was two generations old, so it was deleted. assertThat(tester.evaluator.getValues().keySet()) - .containsExactly(skyKey("d1"), skyKey("d2"), ErrorTransienceValue.KEY); + .containsExactly(skyKey("d1"), d2Key, ErrorTransienceValue.KEY); } @Test public void deleteDirtyCleanedValue() throws Exception { - SkyKey leafKey = GraphTester.skyKey("leafKey"); + SkyKey leafKey = GraphTester.nonHermeticKey("leafKey"); tester.getOrCreate(leafKey).setConstantValue(new StringValue("value")); SkyKey topKey = GraphTester.skyKey("topKey"); tester.getOrCreate(topKey).addDependency(leafKey).setComputedValue(CONCATENATE); @@ -623,13 +650,12 @@ @Test public void invalidationWithChangeAndThenNothingChanged() throws Exception { - tester.getOrCreate("a") - .addDependency("b") - .setComputedValue(COPY); - tester.set("b", new StringValue("y")); + SkyKey bKey = GraphTester.nonHermeticKey("b"); + tester.getOrCreate("a").addDependency(bKey).setComputedValue(COPY); + tester.set(bKey, new StringValue("y")); StringValue original = (StringValue) tester.evalAndGet("a"); assertThat(original.getValue()).isEqualTo("y"); - tester.set("b", new StringValue("z")); + tester.set(bKey, new StringValue("z")); tester.invalidate(); StringValue old = (StringValue) tester.evalAndGet("a"); assertThat(old.getValue()).isEqualTo("z"); @@ -640,7 +666,7 @@ @Test public void noKeepGoingErrorAfterKeepGoingError() throws Exception { - SkyKey topKey = GraphTester.skyKey("top"); + SkyKey topKey = GraphTester.nonHermeticKey("top"); SkyKey errorKey = GraphTester.skyKey("error"); tester.getOrCreate(errorKey).setHasError(true); tester.getOrCreate(topKey).addDependency(errorKey).setComputedValue(CONCATENATE); @@ -685,7 +711,7 @@ @Test public void transientPruning() throws Exception { - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); tester.getOrCreate("top").setHasTransientError(true).addDependency(leaf); tester.set(leaf, new StringValue("leafy")); tester.evalAndGetError(/*keepGoing=*/ true, "top"); @@ -706,13 +732,12 @@ @Test public void incrementalSimpleDependency() throws Exception { - tester.getOrCreate("ab") - .addDependency("a") - .setComputedValue(COPY); - tester.set("a", new StringValue("me")); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + tester.getOrCreate("ab").addDependency(aKey).setComputedValue(COPY); + tester.set(aKey, new StringValue("me")); tester.evalAndGet("ab"); - tester.set("a", new StringValue("other")); + tester.set(aKey, new StringValue("other")); tester.invalidate(); StringValue value = (StringValue) tester.evalAndGet("ab"); assertThat(value.getValue()).isEqualTo("other"); @@ -720,35 +745,33 @@ @Test public void diamondDependency() throws Exception { - setupDiamondDependency(); - tester.set("d", new StringValue("me")); + SkyKey diamondBase = setupDiamondDependency(); + tester.set(diamondBase, new StringValue("me")); StringValue value = (StringValue) tester.evalAndGet("a"); assertThat(value.getValue()).isEqualTo("meme"); } @Test public void incrementalDiamondDependency() throws Exception { - setupDiamondDependency(); - tester.set("d", new StringValue("me")); + SkyKey diamondBase = setupDiamondDependency(); + tester.set(diamondBase, new StringValue("me")); tester.evalAndGet("a"); - tester.set("d", new StringValue("other")); + tester.set(diamondBase, new StringValue("other")); tester.invalidate(); StringValue value = (StringValue) tester.evalAndGet("a"); assertThat(value.getValue()).isEqualTo("otherother"); } - private void setupDiamondDependency() { + private SkyKey setupDiamondDependency() { + SkyKey diamondBase = GraphTester.nonHermeticKey("d"); tester.getOrCreate("a") .addDependency("b") .addDependency("c") .setComputedValue(CONCATENATE); - tester.getOrCreate("b") - .addDependency("d") - .setComputedValue(COPY); - tester.getOrCreate("c") - .addDependency("d") - .setComputedValue(COPY); + tester.getOrCreate("b").addDependency(diamondBase).setComputedValue(COPY); + tester.getOrCreate("c").addDependency(diamondBase).setComputedValue(COPY); + return diamondBase; } // ParallelEvaluator notifies ValueProgressReceiver of already-built top-level values in error: we @@ -793,7 +816,7 @@ @Test public void receiverToldOfVerifiedValueDependingOnCycle() throws Exception { - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey cycle = GraphTester.toSkyKey("cycle"); SkyKey top = GraphTester.toSkyKey("top"); tester.set(leaf, new StringValue("leaf")); @@ -812,31 +835,29 @@ @Test public void incrementalAddedDependency() throws Exception { - tester.getOrCreate("a") - .addDependency("b") - .setComputedValue(CONCATENATE); - tester.set("b", new StringValue("first")); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + SkyKey bKey = GraphTester.nonHermeticKey("b"); + tester.getOrCreate(aKey).addDependency(bKey).setComputedValue(CONCATENATE); + tester.set(bKey, new StringValue("first")); tester.set("c", new StringValue("second")); - tester.evalAndGet("a"); + tester.evalAndGet(/*keepGoing=*/ false, aKey); - tester.getOrCreate("a").addDependency("c"); - tester.set("b", new StringValue("now")); + tester.getOrCreate(aKey).addDependency("c"); + tester.set(bKey, new StringValue("now")); tester.invalidate(); - StringValue value = (StringValue) tester.evalAndGet("a"); + StringValue value = (StringValue) tester.evalAndGet(/*keepGoing=*/ false, aKey); assertThat(value.getValue()).isEqualTo("nowsecond"); } @Test public void manyValuesDependOnSingleValue() throws Exception { - initializeTester(); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); String[] values = new String[TEST_NODE_COUNT]; for (int i = 0; i < values.length; i++) { values[i] = Integer.toString(i); - tester.getOrCreate(values[i]) - .addDependency("leaf") - .setComputedValue(COPY); + tester.getOrCreate(values[i]).addDependency(leaf).setComputedValue(COPY); } - tester.set("leaf", new StringValue("leaf")); + tester.set(leaf, new StringValue("leaf")); EvaluationResult<StringValue> result = tester.eval(/* keepGoing= */ false, values); for (int i = 0; i < values.length; i++) { @@ -845,7 +866,7 @@ } for (int j = 0; j < TESTED_NODES; j++) { - tester.set("leaf", new StringValue("other" + j)); + tester.set(leaf, new StringValue("other" + j)); tester.invalidate(); result = tester.eval(/* keepGoing= */ false, values); for (int i = 0; i < values.length; i++) { @@ -859,13 +880,13 @@ @Test public void singleValueDependsOnManyValues() throws Exception { - initializeTester(); - String[] values = new String[TEST_NODE_COUNT]; + SkyKey[] values = new SkyKey[TEST_NODE_COUNT]; StringBuilder expected = new StringBuilder(); for (int i = 0; i < values.length; i++) { - values[i] = Integer.toString(i); - tester.set(values[i], new StringValue(values[i])); - expected.append(values[i]); + String iString = Integer.toString(i); + values[i] = GraphTester.nonHermeticKey(iString); + tester.set(values[i], new StringValue(iString)); + expected.append(iString); } SkyKey rootKey = toSkyKey("root"); TestFunction value = tester.getOrCreate(rootKey) @@ -893,19 +914,15 @@ @Test public void twoRailLeftRightDependencies() throws Exception { - initializeTester(); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); String[] leftValues = new String[TEST_NODE_COUNT]; String[] rightValues = new String[TEST_NODE_COUNT]; for (int i = 0; i < leftValues.length; i++) { leftValues[i] = "left-" + i; rightValues[i] = "right-" + i; if (i == 0) { - tester.getOrCreate(leftValues[i]) - .addDependency("leaf") - .setComputedValue(COPY); - tester.getOrCreate(rightValues[i]) - .addDependency("leaf") - .setComputedValue(COPY); + tester.getOrCreate(leftValues[i]).addDependency(leaf).setComputedValue(COPY); + tester.getOrCreate(rightValues[i]).addDependency(leaf).setComputedValue(COPY); } else { tester.getOrCreate(leftValues[i]) .addDependency(leftValues[i - 1]) @@ -917,7 +934,7 @@ .setComputedValue(new PassThroughSelected(toSkyKey(rightValues[i - 1]))); } } - tester.set("leaf", new StringValue("leaf")); + tester.set(leaf, new StringValue("leaf")); String lastLeft = "left-" + (TEST_NODE_COUNT - 1); String lastRight = "right-" + (TEST_NODE_COUNT - 1); @@ -928,7 +945,7 @@ for (int j = 0; j < TESTED_NODES; j++) { String value = "other" + j; - tester.set("leaf", new StringValue(value)); + tester.set(leaf, new StringValue(value)); tester.invalidate(); result = tester.eval(/* keepGoing= */ false, lastLeft, lastRight); assertThat(result.get(toSkyKey(lastLeft))).isEqualTo(new StringValue(value)); @@ -994,9 +1011,8 @@ } private void changeCycle(boolean keepGoing) throws Exception { - initializeTester(); SkyKey aKey = GraphTester.toSkyKey("a"); - SkyKey bKey = GraphTester.toSkyKey("b"); + SkyKey bKey = GraphTester.nonHermeticKey("b"); SkyKey topKey = GraphTester.toSkyKey("top"); SkyKey midKey = GraphTester.toSkyKey("mid"); tester.getOrCreate(topKey).addDependency(midKey).setComputedValue(COPY); @@ -1035,9 +1051,9 @@ public void cycleAboveIndependentCycle() throws Exception { makeGraphDeterministic(); SkyKey aKey = GraphTester.toSkyKey("a"); - final SkyKey bKey = GraphTester.toSkyKey("b"); - SkyKey cKey = GraphTester.toSkyKey("c"); - final SkyKey leafKey = GraphTester.toSkyKey("leaf"); + SkyKey bKey = GraphTester.toSkyKey("b"); + SkyKey cKey = GraphTester.nonHermeticKey("c"); + final SkyKey leafKey = GraphTester.nonHermeticKey("leaf"); // When aKey depends on leafKey and bKey, tester .getOrCreate(aKey) @@ -1114,7 +1130,7 @@ // The cycle detection algorithm non-deterministically traverses into children nodes, so // use explicit determinism. makeGraphDeterministic(); - SkyKey cycleKey1 = GraphTester.toSkyKey("ZcycleKey1"); + SkyKey cycleKey1 = GraphTester.nonHermeticKey("ZcycleKey1"); SkyKey cycleKey2 = GraphTester.toSkyKey("AcycleKey2"); tester.getOrCreate(cycleKey1).addDependency(cycleKey2).addDependency(cycleKey1) .setComputedValue(CONCATENATE); @@ -1189,8 +1205,7 @@ /** Regression test: "crash in cycle checker with dirty values". */ @Test public void cycleWithDirtyValue() throws Exception { - initializeTester(); - SkyKey cycleKey1 = GraphTester.toSkyKey("cycleKey1"); + SkyKey cycleKey1 = GraphTester.nonHermeticKey("cycleKey1"); SkyKey cycleKey2 = GraphTester.toSkyKey("cycleKey2"); tester.getOrCreate(cycleKey1).addDependency(cycleKey2).setComputedValue(COPY); tester.getOrCreate(cycleKey2).addDependency(cycleKey1).setComputedValue(COPY); @@ -1346,7 +1361,7 @@ } }, /*deterministic=*/ true); - final SkyKey dep1 = GraphTester.toSkyKey("dep1"); + SkyKey dep1 = GraphTester.nonHermeticKey("dep1"); tester.set(dep1, new StringValue("dep1")); final SkyKey dep2 = GraphTester.toSkyKey("dep2"); tester.set(dep2, new StringValue("dep2")); @@ -1411,9 +1426,8 @@ @Test public void breakCycle() throws Exception { - initializeTester(); - SkyKey aKey = GraphTester.toSkyKey("a"); - SkyKey bKey = GraphTester.toSkyKey("b"); + SkyKey aKey = GraphTester.nonHermeticKey("a"); + SkyKey bKey = GraphTester.nonHermeticKey("b"); // When aKey and bKey depend on each other, tester.getOrCreate(aKey).addDependency(bKey); tester.getOrCreate(bKey).addDependency(aKey); @@ -1458,8 +1472,8 @@ public void nodeInvalidatedThenDoubleCycle() throws InterruptedException { makeGraphDeterministic(); // When topKey depends on depKey, and both are top-level nodes in the graph, - final SkyKey topKey = skyKey("bKey"); - final SkyKey depKey = skyKey("aKey"); + SkyKey topKey = GraphTester.nonHermeticKey("bKey"); + SkyKey depKey = GraphTester.nonHermeticKey("aKey"); tester.getOrCreate(topKey).addDependency(depKey).setConstantValue(new StringValue("a")); tester.getOrCreate(depKey).setConstantValue(new StringValue("b")); // Then evaluation is as expected. @@ -1569,9 +1583,9 @@ @Test public void nodeIsChangedWithoutBeingEvaluated() throws Exception { - SkyKey buildFile = GraphTester.skyKey("buildfile"); + SkyKey buildFile = GraphTester.nonHermeticKey("buildfile"); SkyKey top = GraphTester.skyKey("top"); - SkyKey dep = GraphTester.skyKey("dep"); + SkyKey dep = GraphTester.nonHermeticKey("dep"); tester.set(buildFile, new StringValue("depend on dep")); StringValue depVal = new StringValue("this is dep"); tester.set(dep, depVal); @@ -1623,10 +1637,9 @@ */ @Test public void incompleteDirectDepsAreClearedBeforeInvalidation() throws Exception { - initializeTester(); CountDownLatch slowStart = new CountDownLatch(1); CountDownLatch errorFinish = new CountDownLatch(1); - SkyKey errorKey = GraphTester.toSkyKey("error"); + SkyKey errorKey = GraphTester.nonHermeticKey("error"); tester.getOrCreate(errorKey).setBuilder( new ChainedFunction(/*notifyStart=*/null, /*waitToFinish=*/slowStart, /*notifyFinish=*/errorFinish, /*waitForException=*/false, /*value=*/null, @@ -1738,8 +1751,7 @@ @Test public void incompleteDirectDepsForDirtyValue() throws Exception { - initializeTester(); - SkyKey topKey = GraphTester.toSkyKey("top"); + SkyKey topKey = GraphTester.nonHermeticKey("top"); tester.set(topKey, new StringValue("initial")); // Put topKey into graph so it will be dirtied on next run. assertThat(tester.evalAndGet(/*keepGoing=*/ false, topKey)) @@ -1781,17 +1793,20 @@ @Test public void continueWithErrorDep() throws Exception { - initializeTester(); + SkyKey afterKey = GraphTester.nonHermeticKey("after"); SkyKey errorKey = GraphTester.toSkyKey("my_error_value"); tester.getOrCreate(errorKey).setHasError(true); - tester.set("after", new StringValue("after")); + tester.set(afterKey, new StringValue("after")); SkyKey parentKey = GraphTester.toSkyKey("parent"); - tester.getOrCreate(parentKey).addErrorDependency(errorKey, new StringValue("recovered")) - .setComputedValue(CONCATENATE).addDependency("after"); + tester + .getOrCreate(parentKey) + .addErrorDependency(errorKey, new StringValue("recovered")) + .setComputedValue(CONCATENATE) + .addDependency(afterKey); EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/true, parentKey); assertThat(result.errorMap()).isEmpty(); assertThat(result.get(parentKey).getValue()).isEqualTo("recoveredafter"); - tester.set("after", new StringValue("before")); + tester.set(afterKey, new StringValue("before")); tester.invalidate(); result = tester.eval(/*keepGoing=*/true, parentKey); assertThat(result.errorMap()).isEmpty(); @@ -1800,7 +1815,7 @@ @Test public void continueWithErrorDepTurnedGood() throws Exception { - SkyKey errorKey = GraphTester.toSkyKey("my_error_value"); + SkyKey errorKey = GraphTester.nonHermeticKey("my_error_value"); tester.getOrCreate(errorKey).setHasError(true); tester.set("after", new StringValue("after")); SkyKey parentKey = GraphTester.toSkyKey("parent"); @@ -1818,8 +1833,7 @@ @Test public void errorDepAlreadyThereThenTurnedGood() throws Exception { - initializeTester(); - SkyKey errorKey = GraphTester.toSkyKey("my_error_value"); + SkyKey errorKey = GraphTester.nonHermeticKey("my_error_value"); tester.getOrCreate(errorKey).setHasError(true); SkyKey parentKey = GraphTester.toSkyKey("parent"); tester.getOrCreate(parentKey).addErrorDependency(errorKey, new StringValue("recovered")) @@ -1858,8 +1872,7 @@ */ @Test public void doubleDepOnErrorTransienceValue() throws Exception { - initializeTester(); - SkyKey leafKey = GraphTester.toSkyKey("leaf"); + SkyKey leafKey = GraphTester.nonHermeticKey("leaf"); tester.set(leafKey, new StringValue("leaf")); // Prime the graph by putting leaf in beforehand. assertThat(tester.evalAndGet(/*keepGoing=*/ false, leafKey)).isEqualTo(new StringValue("leaf")); @@ -1878,9 +1891,8 @@ /** Regression test for crash bug. */ @Test public void errorTransienceDepCleared() throws Exception { - initializeTester(); - final SkyKey top = GraphTester.toSkyKey("top"); - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey top = GraphTester.toSkyKey("top"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); tester.set(leaf, new StringValue("leaf")); tester.getOrCreate(top).addDependency(leaf).setHasTransientError(true); EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/false, top); @@ -1923,11 +1935,10 @@ */ @Test public void errorInDependencyGroup() throws Exception { - initializeTester(); SkyKey topKey = GraphTester.toSkyKey("top"); CountDownLatch slowStart = new CountDownLatch(1); CountDownLatch errorFinish = new CountDownLatch(1); - final SkyKey errorKey = GraphTester.toSkyKey("error"); + SkyKey errorKey = GraphTester.nonHermeticKey("error"); tester.getOrCreate(errorKey).setBuilder( new ChainedFunction(/*notifyStart=*/null, /*waitToFinish=*/slowStart, /*notifyFinish=*/errorFinish, /*waitForException=*/false, @@ -1979,12 +1990,11 @@ */ @Test public void valueInErrorWithGroups() throws Exception { - initializeTester(); SkyKey topKey = GraphTester.toSkyKey("top"); - final SkyKey groupDepA = GraphTester.toSkyKey("groupDepA"); + final SkyKey groupDepA = GraphTester.nonHermeticKey("groupDepA"); final SkyKey groupDepB = GraphTester.toSkyKey("groupDepB"); - SkyKey depC = GraphTester.toSkyKey("depC"); - tester.set(groupDepA, new StringValue("depC")); + SkyKey depC = GraphTester.nonHermeticKey("depC"); + tester.set(groupDepA, new SkyKeyValue(depC)); tester.set(groupDepB, new StringValue("")); tester.getOrCreate(depC).setHasError(true); tester @@ -1994,15 +2004,14 @@ @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - StringValue val = - ((StringValue) + SkyKeyValue val = + ((SkyKeyValue) env.getValues(ImmutableList.of(groupDepA, groupDepB)).get(groupDepA)); if (env.valuesMissing()) { return null; } - String nextDep = val.getValue(); try { - env.getValueOrThrow(GraphTester.toSkyKey(nextDep), SomeErrorException.class); + env.getValueOrThrow(val.key, SomeErrorException.class); } catch (SomeErrorException e) { throw new GenericFunctionException(e, Transience.PERSISTENT); } @@ -2010,21 +2019,28 @@ } }); - EvaluationResult<StringValue> evaluationResult = tester.eval( - /*keepGoing=*/true, groupDepA, depC); + EvaluationResult<SkyValue> evaluationResult = tester.eval(/*keepGoing=*/ true, groupDepA, depC); assertThat(evaluationResult.hasError()).isTrue(); - assertThat(evaluationResult.get(groupDepA).getValue()).isEqualTo("depC"); + assertThat(((SkyKeyValue) evaluationResult.get(groupDepA)).key).isEqualTo(depC); assertThat(evaluationResult.getError(depC).getRootCauses()).containsExactly(depC).inOrder(); evaluationResult = tester.eval(/*keepGoing=*/false, topKey); assertThat(evaluationResult.hasError()).isTrue(); assertThat(evaluationResult.getError(topKey).getRootCauses()).containsExactly(topKey).inOrder(); - tester.set(groupDepA, new StringValue("groupDepB")); + tester.set(groupDepA, new SkyKeyValue(groupDepB)); tester.getOrCreate(depC, /*markAsModified=*/true); tester.invalidate(); evaluationResult = tester.eval(/*keepGoing=*/false, topKey); assertWithMessage(evaluationResult.toString()).that(evaluationResult.hasError()).isFalse(); - assertThat(evaluationResult.get(topKey).getValue()).isEqualTo("top"); + assertThat(evaluationResult.get(topKey)).isEqualTo(new StringValue("top")); + } + + private static class SkyKeyValue implements SkyValue { + private final SkyKey key; + + private SkyKeyValue(SkyKey key) { + this.key = key; + } } @Test @@ -2091,7 +2107,7 @@ // which will wait for the signal before it enqueues its next dep. We prevent the thread from // finishing by having the listener to which it reports its warning block until top's builder // starts. - final SkyKey firstKey = GraphTester.skyKey("first"); + SkyKey firstKey = GraphTester.nonHermeticKey("first"); tester.set(firstKey, new StringValue("biding")); tester.set(slowAddingDep, new StringValue("dep")); final AtomicInteger numTopInvocations = new AtomicInteger(0); @@ -2177,8 +2193,8 @@ @Test public void removeReverseDepFromRebuildingNode() throws Exception { SkyKey topKey = GraphTester.skyKey("top"); - final SkyKey midKey = GraphTester.skyKey("mid"); - final SkyKey changedKey = GraphTester.skyKey("changed"); + final SkyKey midKey = GraphTester.nonHermeticKey("mid"); + final SkyKey changedKey = GraphTester.nonHermeticKey("changed"); tester.getOrCreate(changedKey).setConstantValue(new StringValue("first")); // When top depends on mid, tester.getOrCreate(topKey).addDependency(midKey).setComputedValue(CONCATENATE); @@ -2242,9 +2258,8 @@ @Test public void dirtyThenDeleted() throws Exception { - initializeTester(); - SkyKey topKey = GraphTester.skyKey("top"); - SkyKey leafKey = GraphTester.skyKey("leaf"); + SkyKey topKey = GraphTester.nonHermeticKey("top"); + SkyKey leafKey = GraphTester.nonHermeticKey("leaf"); tester.getOrCreate(topKey).addDependency(leafKey).setComputedValue(CONCATENATE); tester.set(leafKey, new StringValue("leafy")); assertThat(tester.evalAndGet(/*keepGoing=*/false, topKey)).isEqualTo(new StringValue("leafy")); @@ -2261,15 +2276,13 @@ @Test public void resetNodeOnRequest_smoke() throws Exception { - SkyKey restartingKey = GraphTester.skyKey("restart"); + SkyKey restartingKey = GraphTester.nonHermeticKey("restart"); StringValue expectedValue = new StringValue("done"); AtomicInteger numInconsistencyCalls = new AtomicInteger(0); tester.setGraphInconsistencyReceiver( (key, otherKey, inconsistency) -> { Preconditions.checkState(otherKey == null, otherKey); - Preconditions.checkState( - inconsistency == GraphInconsistencyReceiver.Inconsistency.RESET_REQUESTED, - inconsistency); + Preconditions.checkState(inconsistency == Inconsistency.RESET_REQUESTED, inconsistency); Preconditions.checkState(restartingKey.equals(key), key); numInconsistencyCalls.incrementAndGet(); }); @@ -2327,9 +2340,7 @@ tester.setGraphInconsistencyReceiver( (key, otherKey, inconsistency) -> { Preconditions.checkState(otherKey == null, otherKey); - Preconditions.checkState( - inconsistency == GraphInconsistencyReceiver.Inconsistency.RESET_REQUESTED, - inconsistency); + Preconditions.checkState(inconsistency == Inconsistency.RESET_REQUESTED, inconsistency); Preconditions.checkState(restartingKey.equals(key), key); numInconsistencyCalls.incrementAndGet(); }); @@ -2432,15 +2443,13 @@ (key, otherKey, inconsistency) -> { Preconditions.checkState(missingChild.equals(otherKey), otherKey); Preconditions.checkState( - inconsistency - == GraphInconsistencyReceiver.Inconsistency.CHILD_MISSING_FOR_DIRTY_NODE, - inconsistency); + inconsistency == Inconsistency.CHILD_MISSING_FOR_DIRTY_NODE, inconsistency); Preconditions.checkState(topKey.equals(key), key); numInconsistencyCalls.incrementAndGet(); }); tester.initialize(/*keepEdges=*/ true); tester.getOrCreate(missingChild).setConstantValue(new StringValue("will go missing")); - SkyKey presentChild = GraphTester.skyKey("present"); + SkyKey presentChild = GraphTester.nonHermeticKey("present"); tester.getOrCreate(presentChild).setConstantValue(new StringValue("present")); StringValue topValue = new StringValue("top"); tester @@ -2528,7 +2537,8 @@ // value "leaf5". final List<SkyKey> leaves = new ArrayList<>(); for (int i = 0; i <= 2; i++) { - SkyKey leaf = GraphTester.toSkyKey("leaf" + i); + SkyKey leaf = + i == 2 ? GraphTester.nonHermeticKey("leaf" + i) : GraphTester.toSkyKey("leaf" + i); leaves.add(leaf); tester.set(leaf, new StringValue("leaf" + (i + 2))); } @@ -2574,9 +2584,8 @@ @Test public void dirtyAndChanged() throws Exception { - initializeTester(); - SkyKey leaf = GraphTester.toSkyKey("leaf"); - SkyKey mid = GraphTester.toSkyKey("mid"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); + SkyKey mid = GraphTester.nonHermeticKey("mid"); SkyKey top = GraphTester.toSkyKey("top"); tester.getOrCreate(top).addDependency(mid).setComputedValue(COPY); tester.getOrCreate(mid).addDependency(leaf).setComputedValue(COPY); @@ -2606,7 +2615,7 @@ */ @Test public void dirtyAndChangedValueIsChanged() throws Exception { - final SkyKey parent = GraphTester.toSkyKey("parent"); + SkyKey parent = GraphTester.nonHermeticKey("parent"); final AtomicBoolean blockingEnabled = new AtomicBoolean(false); final CountDownLatch waitForChanged = new CountDownLatch(1); // changed thread checks value entry once (to see if it is changed). dirty thread checks twice, @@ -2645,7 +2654,7 @@ } }, /*deterministic=*/ false); - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); tester.set(leaf, new StringValue("leaf")); tester.getOrCreate(parent).addDependency(leaf).setComputedValue(CONCATENATE); EvaluationResult<StringValue> result; @@ -2667,13 +2676,83 @@ } @Test + public void childVersionRespectedForChangePruning() throws Exception { + SkyKey leaf = skyKey("leaf"); + SkyKey mid = skyKey("mid"); + SkyKey top = skyKey("top"); + SkyKey invalidator = GraphTester.nonHermeticKey("invalidator"); + StringValue value = new StringValue("value"); + Set<InconsistencyData> inconsistencyData = setupGraphInconsistencyReceiver(); + AtomicInteger topEvalCount = new AtomicInteger(0); + tester + .getOrCreate(top) + .setBuilder( + (TaglessSkyFunction) + (skykey, env) -> { + assertThat(skykey).isEqualTo(top); + Map<SkyKey, SkyValue> values = env.getValues(ImmutableList.of(mid, invalidator)); + if (env.valuesMissing()) { + return null; + } + topEvalCount.incrementAndGet(); + return Preconditions.checkNotNull(values.get(mid)); + }); + // When top depends on mid depends on leaf, and also depends on invalidator, + tester.getOrCreate(mid).addDependency(leaf).setComputedValue(CONCATENATE); + tester.getOrCreate(leaf).setConstantValue(value); + tester.getOrCreate(invalidator).setConstantValue(value); + // And top is evaluated at the first version, + assertThat(tester.evalAndGet(/*keepGoing=*/ true, top)).isEqualTo(value); + assertThat(topEvalCount.get()).isEqualTo(1); + // And mid is deleted from the graph, + deleteKeyFromGraph(mid); + assertThat(inconsistencyData).isEmpty(); + // And top is invalidated (by invalidator) but not actually changed, + tester.getOrCreate(invalidator, /*markAsModified=*/ true); + tester.invalidate(); + // Then we re-evaluate successfully, + assertThat(tester.evalAndGet(/*keepGoing=*/ true, top)).isEqualTo(value); + // And we noticed the missing dep, + assertThat(inconsistencyData) + .containsExactly( + InconsistencyData.create(top, mid, Inconsistency.CHILD_MISSING_FOR_DIRTY_NODE)); + // And top was not actually re-evaluated on the incremental build (it was change-pruned). + assertWithMessage("Top should have been change-pruned").that(topEvalCount.get()).isEqualTo(1); + } + + @Test + public void hermeticSkyFunctionCanThrowTransientErrorThenRecover() throws Exception { + SkyKey leaf = skyKey("leaf"); + SkyKey top = skyKey("top"); + // When top depends on leaf, but throws a transient error, + tester + .getOrCreate(top) + .addDependency(leaf) + .setHasTransientError(true) + .setComputedValue(CONCATENATE); + StringValue value = new StringValue("value"); + tester.getOrCreate(leaf).setConstantValue(value); + // And the first build throws a transient error (as expected), + EvaluationResult<StringValue> result = tester.eval(/*keepGoing=*/ true, top); + assertThatEvaluationResult(result).hasError(); + assertThatEvaluationResult(result).hasErrorEntryForKeyThat(top).hasExceptionThat().isNotNull(); + // And then top's transient error is removed, + tester.getOrCreate(top, /*markAsModified=*/ false).setHasTransientError(false); + tester.invalidateTransientErrors(); + // Then top evaluates successfully, even though it was hermetic and didn't give the same result + // on successive evaluations with the same inputs. + result = tester.eval(/*keepGoing=*/ true, top); + assertThatEvaluationResult(result).hasNoError(); + assertThatEvaluationResult(result).hasEntryThat(top).isEqualTo(value); + } + + @Test public void singleValueDependsOnManyDirtyValues() throws Exception { - initializeTester(); SkyKey[] values = new SkyKey[TEST_NODE_COUNT]; StringBuilder expected = new StringBuilder(); for (int i = 0; i < values.length; i++) { String valueName = Integer.toString(i); - values[i] = GraphTester.toSkyKey(valueName); + values[i] = GraphTester.nonHermeticKey(valueName); tester.set(values[i], new StringValue(valueName)); expected.append(valueName); } @@ -2709,14 +2788,13 @@ */ private void dirtyValueChildrenProperlyRemovedOnEarlyBuildAbort( boolean reevaluateMissingValue, boolean removeError) throws Exception { - initializeTester(); - SkyKey errorKey = GraphTester.toSkyKey("error"); + SkyKey errorKey = GraphTester.nonHermeticKey("error"); tester.set(errorKey, new StringValue("biding time")); - SkyKey slowKey = GraphTester.toSkyKey("slow"); + SkyKey slowKey = GraphTester.nonHermeticKey("slow"); tester.set(slowKey, new StringValue("slow")); SkyKey midKey = GraphTester.toSkyKey("mid"); tester.getOrCreate(midKey).addDependency(slowKey).setComputedValue(COPY); - SkyKey lastKey = GraphTester.toSkyKey("last"); + SkyKey lastKey = GraphTester.nonHermeticKey("last"); tester.set(lastKey, new StringValue("last")); SkyKey motherKey = GraphTester.toSkyKey("mother"); tester.getOrCreate(motherKey).addDependency(errorKey) @@ -2817,9 +2895,9 @@ * to them. */ private void manyDirtyValuesClearChildrenOnFail(boolean interrupt) throws Exception { - SkyKey leafKey = GraphTester.toSkyKey("leaf"); + SkyKey leafKey = GraphTester.nonHermeticKey("leaf"); tester.set(leafKey, new StringValue("leafy")); - SkyKey lastKey = GraphTester.toSkyKey("last"); + SkyKey lastKey = GraphTester.nonHermeticKey("last"); tester.set(lastKey, new StringValue("last")); final List<SkyKey> tops = new ArrayList<>(); // Request far more top-level values than there are threads, so some of them will block until @@ -2929,13 +3007,16 @@ super.invalidated(skyKey, state); } }); + SkyKey node0 = GraphTester.nonHermeticKey("node0"); SkyKey key = null; // Create a long chain of nodes. Most of them will not actually be dirtied, but the last one to // be dirtied will enqueue its parent for dirtying, so it will be in the queue for the next run. for (int i = 0; i < TEST_NODE_COUNT; i++) { - key = GraphTester.toSkyKey("node" + i); - if (i > 0) { + key = i == 0 ? node0 : GraphTester.toSkyKey("node" + i); + if (i > 1) { tester.getOrCreate(key).addDependency("node" + (i - 1)).setComputedValue(COPY); + } else if (i == 1) { + tester.getOrCreate(key).addDependency(node0).setComputedValue(COPY); } else { tester.set(key, new StringValue("node0")); } @@ -2944,7 +3025,7 @@ assertThat(((StringValue) tester.evalAndGet(/*keepGoing=*/ false, key)).getValue()) .isEqualTo("node0"); // Start the dirtying process. - tester.set("node0", new StringValue("new")); + tester.set(node0, new StringValue("new")); tester.invalidate(); interruptInvalidation.set(true); try { @@ -2963,7 +3044,7 @@ @Test public void changePruning() throws Exception { - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey mid = GraphTester.toSkyKey("mid"); SkyKey top = GraphTester.toSkyKey("top"); tester.getOrCreate(top).addDependency(mid).setComputedValue(COPY); @@ -2987,8 +3068,7 @@ @Test public void changePruningWithDoneValue() throws Exception { - initializeTester(); - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey mid = GraphTester.toSkyKey("mid"); SkyKey top = GraphTester.toSkyKey("top"); SkyKey suffix = GraphTester.toSkyKey("suffix"); @@ -3006,7 +3086,7 @@ // and its dirty child will evaluate to the same element. tester.getOrCreate(mid, /*markAsModified=*/false).setHasError(true); tester.invalidate(); - value = (StringValue) tester.evalAndGet("leaf"); + value = (StringValue) tester.evalAndGet(/*keepGoing=*/ false, leaf); assertThat(value.getValue()).isEqualTo("leafy"); assertThat(tester.getDirtyKeys()).containsExactly(mid, top); assertThat(tester.getDeletedKeys()).isEmpty(); @@ -3020,7 +3100,7 @@ @Test public void changePruningAfterParentPrunes() throws Exception { - final SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey top = GraphTester.toSkyKey("top"); tester.set(leaf, new StringValue("leafy")); // When top depends on leaf, but always returns the same value, @@ -3072,9 +3152,8 @@ @Test public void changePruningFromOtherNodeAfterParentPrunes() throws Exception { - initializeTester(); - final SkyKey leaf = GraphTester.toSkyKey("leaf"); - final SkyKey other = GraphTester.toSkyKey("other"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); + SkyKey other = GraphTester.nonHermeticKey("other"); SkyKey top = GraphTester.toSkyKey("top"); tester.set(leaf, new StringValue("leafy")); tester.set(other, new StringValue("other")); @@ -3130,8 +3209,7 @@ @Test public void changedChildChangesDepOfParent() throws Exception { - initializeTester(); - final SkyKey buildFile = GraphTester.toSkyKey("buildFile"); + SkyKey buildFile = GraphTester.nonHermeticKey("buildFile"); ValueComputer authorDrink = new ValueComputer() { @Override @@ -3167,7 +3245,7 @@ assertThat(topValue.getValue()).isEqualTo("hemingway drank absinthe"); tester.set(buildFile, new StringValue("joyce")); // Don't evaluate absinthe successfully anymore. - tester.getOrCreate(absinthe).setHasError(true); + tester.getOrCreate(absinthe, /*markAsModified=*/ false).setHasError(true); tester.invalidate(); topValue = (StringValue) tester.evalAndGet("top"); assertThat(topValue.getValue()).isEqualTo("joyce drank whiskey"); @@ -3179,8 +3257,7 @@ @Test public void dirtyDepIgnoresChildren() throws Exception { - initializeTester(); - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey mid = GraphTester.toSkyKey("mid"); SkyKey top = GraphTester.toSkyKey("top"); tester.set(mid, new StringValue("ignore")); @@ -3248,8 +3325,7 @@ */ @Test public void dirtyBuildAfterFailedBuild() throws Exception { - initializeTester(); - final SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey top = GraphTester.toSkyKey("top"); tester.getOrCreate(top).addDependency(leaf).setComputedValue(COPY); tester.set(leaf, new StringValue("leafy")); @@ -3273,12 +3349,11 @@ */ @Test public void changedBuildAfterFailedThenSuccessfulBuild() throws Exception { - initializeTester(); - final SkyKey leaf = GraphTester.toSkyKey("leaf"); - SkyKey top = GraphTester.toSkyKey("top"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); + SkyKey top = GraphTester.nonHermeticKey("top"); tester.getOrCreate(top).addDependency(leaf).setComputedValue(COPY); tester.set(leaf, new StringValue("leafy")); - StringValue topValue = (StringValue) tester.evalAndGet("top"); + StringValue topValue = (StringValue) tester.evalAndGet(/*keepGoing=*/ false, top); assertThat(topValue.getValue()).isEqualTo("leafy"); assertThat(tester.getDirtyKeys()).isEmpty(); assertThat(tester.getDeletedKeys()).isEmpty(); @@ -3290,7 +3365,7 @@ // top value is evaluated unconditionally. tester.getOrCreate(top, /*markAsModified=*/true); tester.invalidate(); - topValue = (StringValue) tester.evalAndGet("top"); + topValue = (StringValue) tester.evalAndGet(/*keepGoing=*/ false, top); assertThat(topValue.getValue()).isEqualTo("crunchy"); } @@ -3309,7 +3384,7 @@ */ @Test public void manyDirtyValuesClearChildrenOnSecondFail() throws Exception { - final SkyKey leafKey = GraphTester.toSkyKey("leaf"); + SkyKey leafKey = GraphTester.nonHermeticKey("leaf"); tester.set(leafKey, new StringValue("leafy")); SkyKey lastKey = GraphTester.toSkyKey("last"); tester.set(lastKey, new StringValue("last")); @@ -3342,8 +3417,7 @@ @Test public void failedDirtyBuild() throws Exception { - initializeTester(); - SkyKey leaf = GraphTester.toSkyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); SkyKey top = GraphTester.toSkyKey("top"); tester.getOrCreate(top).addErrorDependency(leaf, new StringValue("recover")) .setComputedValue(COPY); @@ -3365,9 +3439,8 @@ @Test public void failedDirtyBuildInBuilder() throws Exception { - initializeTester(); - SkyKey leaf = GraphTester.toSkyKey("leaf"); - SkyKey secondError = GraphTester.toSkyKey("secondError"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); + SkyKey secondError = GraphTester.nonHermeticKey("secondError"); SkyKey top = GraphTester.toSkyKey("top"); tester.getOrCreate(top).addDependency(leaf) .addErrorDependency(secondError, new StringValue("recover")).setComputedValue(CONCATENATE); @@ -3406,8 +3479,7 @@ @Test public void dirtyDependsOnErrorTurningGood() throws Exception { - initializeTester(); - SkyKey error = GraphTester.toSkyKey("error"); + SkyKey error = GraphTester.nonHermeticKey("error"); tester.getOrCreate(error).setHasError(true); SkyKey topKey = GraphTester.toSkyKey("top"); tester.getOrCreate(topKey).addDependency(error).setComputedValue(COPY); @@ -3425,8 +3497,7 @@ /** Regression test for crash bug. */ @Test public void dirtyWithOwnErrorDependsOnTransientErrorTurningGood() throws Exception { - initializeTester(); - final SkyKey error = GraphTester.toSkyKey("error"); + SkyKey error = GraphTester.nonHermeticKey("error"); tester.getOrCreate(error).setHasTransientError(true); SkyKey topKey = GraphTester.toSkyKey("top"); SkyFunction errorFunction = new SkyFunction() { @@ -3452,7 +3523,11 @@ tester.getOrCreate(error).setHasTransientError(false); StringValue reformed = new StringValue("reformed"); tester.set(error, reformed); - tester.getOrCreate(topKey).setBuilder(null).addDependency(error).setComputedValue(COPY); + tester + .getOrCreate(topKey, /*markAsModified=*/ false) + .setBuilder(null) + .addDependency(error) + .setComputedValue(COPY); tester.invalidate(); tester.invalidateTransientErrors(); result = tester.eval(/*keepGoing=*/false, topKey); @@ -3481,9 +3556,9 @@ public void errorOnlyBubblesToRequestingParents() throws Exception { // We need control over the order of reverse deps, so use a deterministic graph. makeGraphDeterministic(); - SkyKey errorKey = GraphTester.toSkyKey("error"); + SkyKey errorKey = GraphTester.nonHermeticKey("error"); tester.set(errorKey, new StringValue("biding time")); - SkyKey slowKey = GraphTester.toSkyKey("slow"); + SkyKey slowKey = GraphTester.nonHermeticKey("slow"); tester.set(slowKey, new StringValue("slow")); SkyKey midKey = GraphTester.toSkyKey("mid"); tester.getOrCreate(midKey).addDependency(slowKey).setComputedValue(COPY); @@ -3520,8 +3595,7 @@ @Test public void dirtyWithRecoveryErrorDependsOnErrorTurningGood() throws Exception { - initializeTester(); - final SkyKey error = GraphTester.toSkyKey("error"); + final SkyKey error = GraphTester.nonHermeticKey("error"); tester.getOrCreate(error).setHasError(true); SkyKey topKey = GraphTester.toSkyKey("top"); SkyFunction recoveryErrorFunction = new SkyFunction() { @@ -3798,8 +3872,7 @@ @Test public void absentParent() throws Exception { - initializeTester(); - SkyKey errorKey = GraphTester.toSkyKey("my_error_value"); + SkyKey errorKey = GraphTester.nonHermeticKey("my_error_value"); tester.set(errorKey, new StringValue("biding time")); SkyKey absentParentKey = GraphTester.toSkyKey("absentParent"); tester.getOrCreate(absentParentKey).addDependency(errorKey).setComputedValue(CONCATENATE); @@ -3825,9 +3898,8 @@ } private void checkNotComparableNotPruned(boolean hasEvent) throws Exception { - initializeTester(); SkyKey parent = GraphTester.toSkyKey("parent"); - SkyKey child = GraphTester.toSkyKey("child"); + SkyKey child = GraphTester.nonHermeticKey("child"); NotComparableStringValue notComparableString = new NotComparableStringValue("not comparable"); if (hasEvent) { tester.getOrCreate(child).setConstantValue(notComparableString).setWarning("shmoop"); @@ -3874,9 +3946,8 @@ @Test public void changePruningWithEvent() throws Exception { - initializeTester(); SkyKey parent = GraphTester.toSkyKey("parent"); - SkyKey child = GraphTester.toSkyKey("child"); + SkyKey child = GraphTester.nonHermeticKey("child"); tester.getOrCreate(child).setConstantValue(new StringValue("child")).setWarning("bloop"); // Restart once because child isn't ready. CountDownLatch parentEvaluated = new CountDownLatch(3); @@ -3942,9 +4013,8 @@ @Test public void deleteInvalidatedValue() throws Exception { - initializeTester(); SkyKey top = GraphTester.toSkyKey("top"); - SkyKey toDelete = GraphTester.toSkyKey("toDelete"); + SkyKey toDelete = GraphTester.nonHermeticKey("toDelete"); // Must be a concatenation -- COPY doesn't actually copy. tester.getOrCreate(top).addDependency(toDelete).setComputedValue(CONCATENATE); tester.set(toDelete, new StringValue("toDelete")); @@ -3972,7 +4042,7 @@ SkyKey[] leftValues = new SkyKey[TEST_NODE_COUNT]; SkyKey[] rightValues = new SkyKey[TEST_NODE_COUNT]; for (int i = 0; i < TEST_NODE_COUNT; i++) { - leftValues[i] = GraphTester.toSkyKey("left-" + i); + leftValues[i] = GraphTester.nonHermeticKey("left-" + i); rightValues[i] = GraphTester.toSkyKey("right-" + i); if (i == 0) { tester.getOrCreate(leftValues[i]) @@ -3994,8 +4064,8 @@ } tester.set("leaf", new StringValue("leaf")); - String lastLeft = "left-" + (TEST_NODE_COUNT - 1); - String lastRight = "right-" + (TEST_NODE_COUNT - 1); + SkyKey lastLeft = GraphTester.nonHermeticKey("left-" + (TEST_NODE_COUNT - 1)); + SkyKey lastRight = GraphTester.skyKey("right-" + (TEST_NODE_COUNT - 1)); for (int i = 0; i < TESTED_NODES; i++) { try { @@ -4011,8 +4081,8 @@ tester.getOrCreate(leftValues[i], /*markAsModified=*/true).setHasError(false); tester.invalidate(); result = tester.eval(/* keepGoing= */ false, lastLeft, lastRight); - assertThat(result.get(toSkyKey(lastLeft))).isEqualTo(new StringValue("leaf")); - assertThat(result.get(toSkyKey(lastRight))).isEqualTo(new StringValue("leaf")); + assertThat(result.get(lastLeft)).isEqualTo(new StringValue("leaf")); + assertThat(result.get(lastRight)).isEqualTo(new StringValue("leaf")); } catch (Exception e) { System.err.println("twoRailLeftRightDependenciesWithFailure exception on run " + i); throw e; @@ -4022,26 +4092,26 @@ @Test public void valueInjection() throws Exception { - SkyKey key = GraphTester.toSkyKey("new_value"); + SkyKey key = GraphTester.nonHermeticKey("new_value"); SkyValue val = new StringValue("val"); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("new_value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingEntry() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate(key).setConstantValue(new StringValue("old_val")); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingDirtyEntry() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate(key).setConstantValue(new StringValue("old_val")); @@ -4053,89 +4123,90 @@ tester.differencer.inject(ImmutableMap.of(key, val)); tester.eval(/*keepGoing=*/false, new SkyKey[0]); // Inject again. - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingEntryMarkedForInvalidation() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate(key).setConstantValue(new StringValue("old_val")); tester.differencer.invalidate(ImmutableList.of(key)); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingEntryMarkedForDeletion() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate(key).setConstantValue(new StringValue("old_val")); tester.evaluator.delete(Predicates.<SkyKey>alwaysTrue()); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingEqualEntryMarkedForInvalidation() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); tester.differencer.invalidate(ImmutableList.of(key)); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverExistingEqualEntryMarkedForDeletion() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); tester.evaluator.delete(Predicates.<SkyKey>alwaysTrue()); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverValueWithDeps() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); + SkyKey otherKey = GraphTester.nonHermeticKey("other"); SkyValue val = new StringValue("val"); StringValue prevVal = new StringValue("foo"); - tester.getOrCreate("other").setConstantValue(prevVal); - tester.getOrCreate(key).addDependency("other").setComputedValue(COPY); - assertThat(tester.evalAndGet("value")).isEqualTo(prevVal); + tester.getOrCreate(otherKey).setConstantValue(prevVal); + tester.getOrCreate(key).addDependency(otherKey).setComputedValue(COPY); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(prevVal); tester.differencer.inject(ImmutableMap.of(key, val)); StringValue depVal = new StringValue("newfoo"); - tester.getOrCreate("other").setConstantValue(depVal); - tester.differencer.invalidate(ImmutableList.of(GraphTester.toSkyKey("other"))); + tester.getOrCreate(otherKey).setConstantValue(depVal); + tester.differencer.invalidate(ImmutableList.of(otherKey)); // Injected value is ignored for value with deps. - assertThat(tester.evalAndGet("value")).isEqualTo(depVal); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(depVal); } @Test public void valueInjectionOverEqualValueWithDeps() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate("other").setConstantValue(val); tester.getOrCreate(key).addDependency("other").setComputedValue(COPY); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); tester.differencer.inject(ImmutableMap.of(key, val)); - assertThat(tester.evalAndGet("value")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, key)).isEqualTo(val); } @Test public void valueInjectionOverValueWithErrors() throws Exception { - SkyKey key = GraphTester.toSkyKey("value"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.getOrCreate(key).setHasError(true); @@ -4147,12 +4218,12 @@ @Test public void valueInjectionInvalidatesReverseDeps() throws Exception { - SkyKey childKey = GraphTester.toSkyKey("child"); + SkyKey childKey = GraphTester.nonHermeticKey("child"); SkyKey parentKey = GraphTester.toSkyKey("parent"); StringValue oldVal = new StringValue("old_val"); tester.getOrCreate(childKey).setConstantValue(oldVal); - tester.getOrCreate(parentKey).addDependency("child").setComputedValue(COPY); + tester.getOrCreate(parentKey).addDependency(childKey).setComputedValue(COPY); EvaluationResult<SkyValue> result = tester.eval(false, parentKey); assertThat(result.hasError()).isFalse(); @@ -4160,46 +4231,46 @@ SkyValue val = new StringValue("val"); tester.differencer.inject(ImmutableMap.of(childKey, val)); - assertThat(tester.evalAndGet("child")).isEqualTo(val); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, childKey)).isEqualTo(val); // Injecting a new child should have invalidated the parent. assertThat(tester.getExistingValue("parent")).isNull(); tester.eval(false, childKey); - assertThat(tester.getExistingValue("child")).isEqualTo(val); + assertThat(tester.getExistingValue(childKey)).isEqualTo(val); assertThat(tester.getExistingValue("parent")).isNull(); assertThat(tester.evalAndGet("parent")).isEqualTo(val); } @Test public void valueInjectionOverExistingEqualEntryDoesNotInvalidate() throws Exception { - SkyKey childKey = GraphTester.toSkyKey("child"); + SkyKey childKey = GraphTester.nonHermeticKey("child"); SkyKey parentKey = GraphTester.toSkyKey("parent"); SkyValue val = new StringValue("same_val"); - tester.getOrCreate(parentKey).addDependency("child").setComputedValue(COPY); + tester.getOrCreate(parentKey).addDependency(childKey).setComputedValue(COPY); tester.getOrCreate(childKey).setConstantValue(new StringValue("same_val")); assertThat(tester.evalAndGet("parent")).isEqualTo(val); tester.differencer.inject(ImmutableMap.of(childKey, val)); - assertThat(tester.getExistingValue("child")).isEqualTo(val); + assertThat(tester.getExistingValue(childKey)).isEqualTo(val); // Since we are injecting an equal value, the parent should not have been invalidated. assertThat(tester.getExistingValue("parent")).isEqualTo(val); } @Test public void valueInjectionInterrupt() throws Exception { - SkyKey key = GraphTester.toSkyKey("key"); + SkyKey key = GraphTester.nonHermeticKey("key"); SkyValue val = new StringValue("val"); tester.differencer.inject(ImmutableMap.of(key, val)); Thread.currentThread().interrupt(); try { - tester.evalAndGet("key"); + tester.evalAndGet(/*keepGoing=*/ false, key); fail(); } catch (InterruptedException expected) { // Expected. } - SkyValue newVal = tester.evalAndGet("key"); + SkyValue newVal = tester.evalAndGet(/*keepGoing=*/ false, key); assertThat(newVal).isEqualTo(val); } @@ -4264,7 +4335,7 @@ // errorKey will be invalidated due to its dependence on invalidatedKey, but later revalidated // since invalidatedKey re-evaluates to the same value on a subsequent build. final SkyKey errorKey = GraphTester.toSkyKey("error"); - SkyKey invalidatedKey = GraphTester.toSkyKey("invalidated-leaf"); + SkyKey invalidatedKey = GraphTester.nonHermeticKey("invalidated-leaf"); tester.set(invalidatedKey, new StringValue("invalidated-leaf-value")); tester.getOrCreate(errorKey).addDependency(invalidatedKey).setHasError(true); // Names are alphabetized in reverse deps of errorKey. @@ -4346,8 +4417,8 @@ // errorKey will be invalidated due to its dependence on invalidatedKey, but later revalidated // since invalidatedKey re-evaluates to the same value on a subsequent build. SkyKey errorKey = GraphTester.toSkyKey("error"); - SkyKey invalidatedKey = GraphTester.toSkyKey("invalidated-leaf"); - SkyKey changedKey = GraphTester.toSkyKey("changed-leaf"); + SkyKey invalidatedKey = GraphTester.nonHermeticKey("invalidated-leaf"); + SkyKey changedKey = GraphTester.nonHermeticKey("changed-leaf"); tester.set(invalidatedKey, new StringValue("invalidated-leaf-value")); tester.set(changedKey, new StringValue("changed-leaf-value")); // Names are alphabetized in reverse deps of errorKey. @@ -4483,7 +4554,7 @@ } }); final SkyKey errorKey = GraphTester.toSkyKey("error"); - SkyKey invalidatedKey = GraphTester.toSkyKey("invalidated"); + SkyKey invalidatedKey = GraphTester.nonHermeticKey("invalidated"); final SkyKey topKey = GraphTester.toSkyKey("top"); tester.getOrCreate(errorKey).addDependency(invalidatedKey).setHasError(true); tester.getOrCreate(invalidatedKey).setConstantValue(new StringValue("constant")); @@ -4563,7 +4634,7 @@ public void cachedChildErrorDepWithSiblingDepOnNoKeepGoingEval() throws Exception { SkyKey parent1Key = GraphTester.toSkyKey("parent1"); SkyKey parent2Key = GraphTester.toSkyKey("parent2"); - final SkyKey errorKey = GraphTester.toSkyKey("error"); + SkyKey errorKey = GraphTester.nonHermeticKey("error"); final SkyKey otherKey = GraphTester.toSkyKey("other"); SkyFunction parentBuilder = new SkyFunction() { @@ -4617,9 +4688,9 @@ } private void removedNodeComesBack() throws Exception { - SkyKey top = GraphTester.skyKey("top"); + SkyKey top = GraphTester.nonHermeticKey("top"); SkyKey mid = GraphTester.skyKey("mid"); - SkyKey leaf = GraphTester.skyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); // When top depends on mid, which depends on leaf, tester.getOrCreate(top).addDependency(mid).setComputedValue(CONCATENATE); tester.getOrCreate(mid).addDependency(leaf).setComputedValue(CONCATENATE); @@ -4673,7 +4744,8 @@ tester.getOrCreate(GraphTester.skyKey("leaf"), /*markAsModified=*/ true); // Then when top is evaluated, its value is as expected. tester.invalidate(); - assertThat(tester.evalAndGet(/*keepGoing=*/ true, "top")).isEqualTo(new StringValue("leaf")); + assertThat(tester.evalAndGet(/*keepGoing=*/ true, GraphTester.nonHermeticKey("top"))) + .isEqualTo(new StringValue("leaf")); } // Tests that a removed and then reinstated node behaves properly when its parent disappears and @@ -4681,9 +4753,9 @@ @Test public void removedNodeComesBackAndOtherInvalidates() throws Exception { removedNodeComesBack(); - SkyKey top = GraphTester.skyKey("top"); + SkyKey top = GraphTester.nonHermeticKey("top"); SkyKey mid = GraphTester.skyKey("mid"); - SkyKey leaf = GraphTester.skyKey("leaf"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); // When top is invalidated again, tester.getOrCreate(top, /*markAsModified=*/ true).removeDependency(leaf).addDependency(mid); // Then when top is evaluated, its value is as expected. @@ -4694,8 +4766,8 @@ // Tests that a removed and then reinstated node doesn't have a reverse dep on a former parent. @Test public void removedInvalidatedNodeComesBackAndOtherInvalidates() throws Exception { - SkyKey top = GraphTester.skyKey("top"); - SkyKey leaf = GraphTester.skyKey("leaf"); + SkyKey top = GraphTester.nonHermeticKey("top"); + SkyKey leaf = GraphTester.nonHermeticKey("leaf"); // When top depends on leaf, tester.getOrCreate(top).addDependency(leaf).setComputedValue(CONCATENATE); StringValue leafValue = new StringValue("leaf"); @@ -4734,10 +4806,10 @@ @Test public void cleanReverseDepFromDirtyNodeNotInBuild() throws Exception { - final SkyKey topKey = GraphTester.skyKey("top"); - SkyKey inactiveKey = GraphTester.skyKey("inactive"); - final Thread mainThread = Thread.currentThread(); - final AtomicBoolean shouldInterrupt = new AtomicBoolean(false); + SkyKey topKey = GraphTester.nonHermeticKey("top"); + SkyKey inactiveKey = GraphTester.nonHermeticKey("inactive"); + Thread mainThread = Thread.currentThread(); + AtomicBoolean shouldInterrupt = new AtomicBoolean(false); injectGraphListenerForTesting( new Listener() { @Override @@ -4795,7 +4867,7 @@ @Test public void errorChanged() throws Exception { - SkyKey error = GraphTester.skyKey("error"); + SkyKey error = GraphTester.nonHermeticKey("error"); tester.getOrCreate(error).setHasError(true); assertThatErrorInfo(tester.evalAndGetError(/*keepGoing=*/ true, error)) .hasExceptionThat().isNotNull(); @@ -4871,6 +4943,22 @@ assertThat(tester.evalAndGetError(keepGoing, parentKey).getException()).isSameAs(parentExn); } + /** Data encapsulating a graph inconsistency found during evaluation. */ + @AutoValue + public abstract static class InconsistencyData { + public abstract SkyKey key(); + + @Nullable + public abstract SkyKey otherKey(); + + public abstract Inconsistency inconsistency(); + + public static InconsistencyData create( + SkyKey key, @Nullable SkyKey otherKey, Inconsistency inconsistency) { + return new AutoValue_MemoizingEvaluatorTest_InconsistencyData(key, otherKey, inconsistency); + } + } + /** A graph tester that is specific to the memoizing evaluator, with some convenience methods. */ protected class MemoizingEvaluatorTester extends GraphTester { private RecordingDifferencer differencer; @@ -4997,4 +5085,13 @@ return getExistingValue(toSkyKey(key)); } } + + /** {@link SkyFunction} with no tag extraction for easier lambda-izing. */ + protected interface TaglessSkyFunction extends SkyFunction { + @Nullable + @Override + default String extractTag(SkyKey skyKey) { + return null; + } + } }
diff --git a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java index 6c1e866..89433c6 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java
@@ -67,8 +67,8 @@ */ @RunWith(JUnit4.class) public class ParallelEvaluatorTest { - private static final SkyFunctionName CHILD_TYPE = SkyFunctionName.create("child"); - private static final SkyFunctionName PARENT_TYPE = SkyFunctionName.create("parent"); + private static final SkyFunctionName CHILD_TYPE = SkyFunctionName.createHermetic("child"); + private static final SkyFunctionName PARENT_TYPE = SkyFunctionName.createHermetic("parent"); protected ProcessableGraph graph; protected IntVersion graphVersion = IntVersion.of(0);