Tolerate injected nodes having deps.
We don't check explicitly that these are the only two ways, but this can happen if the error transience node is a dep of a node that's being injected, or if an injected node is an "external" file that needs to depend on an external package.
The first possibility can happen if there was an IOException reading the node on the previous build.
We handle the situation by just dirtying the node, not injecting it. Actual evaluation can handle the re-stat.
PiperOrigin-RevId: 162622092
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index 1750263..e8b1f63 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -205,11 +205,19 @@
if (prevEntry != null && prevEntry.isDone()) {
try {
Iterable<SkyKey> directDeps = prevEntry.getDirectDeps();
- Preconditions.checkState(
- Iterables.isEmpty(directDeps), "existing entry for %s has deps: %s", key, directDeps);
- if (newValue.equals(prevEntry.getValue())
- && !valuesToDirty.contains(key)
- && !valuesToDelete.contains(key)) {
+ if (Iterables.isEmpty(directDeps)) {
+ if (newValue.equals(prevEntry.getValue())
+ && !valuesToDirty.contains(key)
+ && !valuesToDelete.contains(key)) {
+ it.remove();
+ }
+ } else {
+ // Rare situation of an injected dep that depends on another node. Usually the dep is
+ // the error transience node. When working with external repositories, it can also be an
+ // external workspace file. Don't bother injecting it, just invalidate it.
+ // We'll wastefully evaluate the node freshly during evaluation, but this happens very
+ // rarely.
+ valuesToDirty.add(key);
it.remove();
}
} catch (InterruptedException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index 50b98a6..4c349f8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -44,19 +44,21 @@
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.testutil.TestUtils;
+import com.google.devtools.build.lib.util.BlazeClock;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
-import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.build.lib.vfs.util.FileSystems;
+import com.google.devtools.build.skyframe.BuildDriver;
import com.google.devtools.build.skyframe.ErrorInfo;
+import com.google.devtools.build.skyframe.ErrorInfoSubject;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -1272,6 +1274,41 @@
assertThat(valueForPath(child).exists()).isFalse();
}
+ @Test
+ public void testInjectionOverIOException() throws Exception {
+ Path foo = file("foo");
+ SkyKey fooKey = skyKey("foo");
+ fs.stubStatError(foo, new IOException("bork"));
+ BuildDriver driver = makeDriver();
+ EvaluationResult<FileValue> result =
+ driver.evaluate(
+ ImmutableList.of(fooKey),
+ /*keepGoing=*/ true,
+ /*numThreads=*/ 1,
+ NullEventHandler.INSTANCE);
+ ErrorInfoSubject errorInfoSubject = assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(fooKey);
+ errorInfoSubject.isTransient();
+ errorInfoSubject
+ .hasExceptionThat()
+ .hasMessageThat()
+ .isEqualTo("bork");
+ fs.stubbedStatErrors.remove(foo);
+ differencer.inject(
+ fileStateSkyKey("foo"),
+ FileStateValue.create(
+ RootedPath.toRootedPath(pkgRoot, foo),
+ new TimestampGranularityMonitor(BlazeClock.instance())));
+ result =
+ driver.evaluate(
+ ImmutableList.of(fooKey),
+ /*keepGoing=*/ true,
+ /*numThreads=*/ 1,
+ NullEventHandler.INSTANCE);
+ assertThatEvaluationResult(result).hasNoError();
+ assertThat(result.get(fooKey).exists()).isTrue();
+ }
+
private void checkRealPath(String pathString) throws Exception {
Path realPath = pkgRoot.getRelative(pathString).resolveSymbolicLinks();
assertRealPath(pathString, realPath.relativeTo(pkgRoot).toString());
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 7e21d9e..df97dc6 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -3773,14 +3773,11 @@
tester.getOrCreate(key).addDependency("other").setComputedValue(COPY);
assertThat(tester.evalAndGet("value")).isEqualTo(prevVal);
tester.differencer.inject(ImmutableMap.of(key, val));
- try {
- tester.evalAndGet("value");
- fail("injection over value with deps should have failed");
- } catch (IllegalStateException e) {
- assertThat(e).hasMessage(
- "existing entry for " + NODE_TYPE.getName() + ":value has deps: "
- + "[" + NODE_TYPE.getName() + ":other]");
- }
+ StringValue depVal = new StringValue("newfoo");
+ tester.getOrCreate("other").setConstantValue(depVal);
+ tester.differencer.invalidate(ImmutableList.of(GraphTester.toSkyKey("other")));
+ // Injected value is ignored for value with deps.
+ assertThat(tester.evalAndGet("value")).isEqualTo(depVal);
}
@Test
@@ -3792,14 +3789,7 @@
tester.getOrCreate(key).addDependency("other").setComputedValue(COPY);
assertThat(tester.evalAndGet("value")).isEqualTo(val);
tester.differencer.inject(ImmutableMap.of(key, val));
- try {
- tester.evalAndGet("value");
- fail("injection over value with deps should have failed");
- } catch (IllegalStateException e) {
- assertThat(e).hasMessage(
- "existing entry for " + NODE_TYPE.getName() + ":value has deps: "
- + "[" + NODE_TYPE.getName() + ":other]");
- }
+ assertThat(tester.evalAndGet("value")).isEqualTo(val);
}
@Test
