Set the version of a computed node to the max of its child versions rather than the graph version when that is feasible.

  * It's not feasible when the computation accesses outside state, i.e. is non-hermetic, so see below.
  * It's also more complicated (and not worth the trouble) when the computation is taking place just for the error status.

Have SkyFunctionName declare whether the function it corresponds to is hermetic or non-hermetic. Only non-hermetically-generated SkyValues can be directly marked changed, and non-hermetic SkyFunctions have their values saved at the graph version, not the max of the child versions. All SkyFunctions are hermetic except for the ones that can be explicitly dirtied.

A marked-hermetic SkyFunction that has a transient error due to filesystem access can be re-evaluated and get the correct version: if it throws an IOException at version 1 and then, when re-evaluated at version 2 with unchanged dependencies, has a value, the version will be version 1.

All Skyframe unit tests that were doing non-hermetic things to nodes need to declare that those nodes are non-hermetic. I tried to make the minimal set of changes there, so that we had good incidental coverage of hermetic+non-hermetic nodes. Also did some drive-by clean-ups around that code.

Artifacts are a weird case, since they're doing untracked filesystem access (for source directories). Using max(child versions) for them gives rise to the following correctness bug: 1. do a build at v1 that creates a FileStateValue for dir/ at v1. Then at v2, add a file to dir/ and do a build that consumes dir/ as a source artifact. Now the artifact for dir/ will (incorrectly) have v1. Then at v1, do that build again. We'll consume the "artifact from the future". However, this can only have an effect when using the local action cache, since the incorrect value of the artifact (the mtime) is only consumed by the action cache. Bazel is already broken in this way (incremental builds don't invalidate directories), so this change doesn't make things worse.

PiperOrigin-RevId: 204210719

1 files changed, 88 insertions, 69 deletions

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>() {