diff options
Diffstat (limited to 'src')
41 files changed, 900 insertions, 475 deletions
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 b456de9be5..d287d77a55 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.actions.ActionLookupValue.ActionLookupKey; 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 @@ import com.google.devtools.build.skyframe.SkyKey; 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 a27756e259..f21d579aed 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 class Artifact } }; - 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 bc4b51ad71..1a5cceb1d0 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 @@ import javax.annotation.Nullable; */ @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 d862881b2c..89450d2f62 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 @@ import javax.annotation.Nullable; @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 6e0a8b0fd8..24c24ba356 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 @@ import org.apache.maven.settings.building.SettingsBuildingResult; * 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 831c408e2d..43d984628b 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 final class Label 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 5d2192b889..b6d0d9e576 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 @@ import java.util.Objects; @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 c99898028a..b501f3abce 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.ModifiedFileSet; 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 @@ public class FilesystemValueChecker { 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 aed4d5dbc1..be40e0619f 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 @@ package com.google.devtools.build.lib.skyframe; 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 @@ import com.google.devtools.build.skyframe.SkyKey; * 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 2a41a4740f..ad852d6268 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 @@ import javax.annotation.Nullable; * 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 a82d1158df..af658f32d5 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 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception> DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { super( graph, graphVersion, @@ -127,7 +128,8 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception> progressReceiver, graphInconsistencyReceiver, forkJoinPool, - cycleDetector); + cycleDetector, + evaluationVersionBehavior); } private void informProgressReceiverThatValueIsDone(SkyKey key, NodeEntry entry) @@ -469,6 +471,7 @@ public abstract class AbstractExceptionalParallelEvaluator<E extends Exception> 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 c2f8561c32..04fcdeae91 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 @@ public abstract class AbstractParallelEvaluator { DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { this.graph = graph; this.cycleDetector = cycleDetector; evaluatorContext = @@ -120,14 +121,17 @@ public abstract class AbstractParallelEvaluator { 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 @@ public abstract class AbstractParallelEvaluator { 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 8a57fd0d35..c75206db4d 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 @@ package( SKYFRAME_OBJECT_SRCS = [ "AbstractSkyKey.java", + "FunctionHermeticity.java", "ShareabilityOfValue.java", "SkyFunctionName.java", "SkyKey.java", @@ -38,7 +39,6 @@ java_library( "//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 59a2e13db3..2732fc4fe5 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 @@ public abstract class DirtyBuildingState { 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 @@ public abstract class DirtyBuildingState { } 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 babf90208c..bd9ce9c746 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 @@ import java.io.ObjectOutputStream; * 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 0000000000..a4cc37165e --- /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 0000000000..fd19b757b7 --- /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 859f531666..12e3a2357f 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 @@ public class InMemoryNodeEntry implements NodeEntry { // 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 class InMemoryNodeEntry implements NodeEntry { 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 9d2d811594..168ad23b95 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 @@ public abstract class InvalidatingNodeVisitor<TGraph extends QueryableGraph> { 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 1e6169496b..e74fb1ed16 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 @@ public interface NodeEntry extends ThinNodeEntry { */ 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 afd0279e5f..5b847c5c5a 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 @@ public class ParallelEvaluator extends AbstractExceptionalParallelEvaluator<Runt DirtyTrackingProgressReceiver progressReceiver, GraphInconsistencyReceiver graphInconsistencyReceiver, ForkJoinPool forkJoinPool, - CycleDetector cycleDetector) { + CycleDetector cycleDetector, + EvaluationVersionBehavior evaluationVersionBehavior) { super( graph, graphVersion, @@ -83,7 +84,8 @@ public class ParallelEvaluator extends AbstractExceptionalParallelEvaluator<Runt 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 14306d484b..60987ac7f2 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 @@ class ParallelEvaluatorContext { 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 @@ class ParallelEvaluatorContext { storedEventFilter, errorInfoManager, graphInconsistencyReceiver, - () -> new NodeEntryVisitor(threadCount, progressReceiver, runnableMaker)); + () -> new NodeEntryVisitor(threadCount, progressReceiver, runnableMaker), + EvaluationVersionBehavior.MAX_CHILD_VERSIONS); } ParallelEvaluatorContext( @@ -101,7 +103,8 @@ class ParallelEvaluatorContext { ErrorInfoManager errorInfoManager, final Function<SkyKey, Runnable> runnableMaker, GraphInconsistencyReceiver graphInconsistencyReceiver, - final ForkJoinPool forkJoinPool) { + final ForkJoinPool forkJoinPool, + EvaluationVersionBehavior evaluationVersionBehavior) { this( graph, graphVersion, @@ -113,7 +116,8 @@ class ParallelEvaluatorContext { storedEventFilter, errorInfoManager, graphInconsistencyReceiver, - () -> new NodeEntryVisitor(forkJoinPool, progressReceiver, runnableMaker)); + () -> new NodeEntryVisitor(forkJoinPool, progressReceiver, runnableMaker), + evaluationVersionBehavior); } private ParallelEvaluatorContext( @@ -127,12 +131,14 @@ class ParallelEvaluatorContext { 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 @@ class ParallelEvaluatorContext { 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 4b2fc51cdf..fc6e3c88c8 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { Preconditions.checkState(!assertDone, "%s had not done: %s", skyKey, key); continue; } + maybeUpdateMaxChildVersion(depEntry); result.add(valueOrNullMarker); } return result; @@ -686,7 +701,6 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { evaluationState); evaluatorContext.signalValuesAndEnqueueIfReady( - skyKey, reverseDeps, valueVersion, enqueueParents); + skyKey, reverseDeps, currentVersion, enqueueParents); evaluatorContext.getReplayingNestedSetPostableVisitor().visit(posts); evaluatorContext.getReplayingNestedSetEventVisitor().visit(events); @@ -777,11 +803,25 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { 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 3d6ece1975..107181ed1d 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 @@ public final class SkyFunctionName implements Serializable { * 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); + } + + /** + * 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) { - SkyFunctionName result = new SkyFunctionName(name, shareabilityOfValue); + 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 @@ public final class SkyFunctionName implements Serializable { 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 @@ public final class SkyFunctionName implements Serializable { 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 0000000000..221f44f959 --- /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 709d9dc237..f8d46856bd 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 @@ public class ExternalPackageUtilTest extends BuildViewTestCase { } 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 @@ public class ExternalPackageUtilTest extends BuildViewTestCase { } 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 @@ public class ExternalPackageUtilTest extends BuildViewTestCase { } 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 c8f9da1a7d..786fe20a10 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.common.collect.Iterables; 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.ArtifactSkyKey; 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 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas } } - 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 5a03f3efbf..b0deb69d58 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.ActionLookupValue.ActionLookupKey; 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 java.util.concurrent.atomic.AtomicReference; 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 @@ abstract class ArtifactFunctionTestCase { } } - 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 31091267d3..a813512129 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 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase { */ 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 3d9971d15d..57502211fb 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.ImmutableMap; 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 @@ public class PackageFunctionTest extends BuildViewTestCase { // 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 b75009d447..4315e34d64 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.Path; 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe } 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe 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 5633ab523d..c0e3317649 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.ActionCache; 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 @@ import org.junit.Before; 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 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { } } - 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 a4b727e22b..5e7f349afc 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.ImmutableList; 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 4a78f59e8c..b431612a8a 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 @@ public class ToolchainUtilTest extends ToolchainTestCase { // 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 712dc503f4..b3808d29c5 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 @@ java_test( "//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 7cac95f273..866fdd70ce 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 @@ import org.junit.runners.JUnit4; @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 632f23d13f..524ea9b098 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 @@ public class DeterministicHelper extends NotifyingHelper { } } + /** 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 d9ebca5ebc..223b14b37c 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 @@ public class EagerInvalidatorTest { } }); 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 @@ public class EagerInvalidatorTest { // 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 @@ public class EagerInvalidatorTest { } }); 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 @@ public class EagerInvalidatorTest { 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 class EagerInvalidatorTest { 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 @@ public class EagerInvalidatorTest { 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 @@ public class EagerInvalidatorTest { 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 @@ public class EagerInvalidatorTest { 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 @@ public class EagerInvalidatorTest { 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 9b129df70e..b688880b1a 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 class GraphTester { public GraphTester() { functionMap.put(NODE_TYPE, new DelegatingFunction()); + functionMap.put(FOR_TESTING_NONHERMETIC, new DelegatingFunction()); } public TestFunction getOrCreate(String name) { @@ -170,6 +171,10 @@ public class GraphTester { 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 @@ public class GraphTester { 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 @@ public class GraphTester { 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 3112cff3ce..d10a243455 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.NODE_TYPE; 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.ExtendedEventHandler; 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { } 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { .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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { } 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 class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { /** 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 @@ public class MemoizingEvaluatorTest { } }, /*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 @@ public class MemoizingEvaluatorTest { @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 class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { /** 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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { } }); - 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { (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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { } }, /*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 @@ public class MemoizingEvaluatorTest { } @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 @@ public class MemoizingEvaluatorTest { */ 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 @@ public class MemoizingEvaluatorTest { * 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { */ @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { /** 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 @@ public class MemoizingEvaluatorTest { 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 class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { } 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { } 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { } }); 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 class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { } 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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { // 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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { @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 @@ public class MemoizingEvaluatorTest { 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 @@ public class MemoizingEvaluatorTest { 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 6c1e86633d..89433c68f6 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 @@ import org.junit.runners.JUnit4; */ @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); |