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, 31 insertions, 0 deletions

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
+}