diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java index cb22bba7ea3ed7..b71909828328fe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java @@ -444,12 +444,26 @@ public StateMachine computeConfigConditions(Tasks tasks) { return DONE; } + @Override + public void acceptConfigConditions(ConfigConditions configConditions) { + this.configConditions = configConditions; + } + + @Override + public void acceptConfigConditionsError(ConfiguredValueCreationException e) { + emitTransitionErrorMessage(e.getMessage()); + } + public StateMachine computeTransition(Tasks tasks) { + if (configConditions == null) { + return DONE; + } + // logic to compute transition with ConfigConditions var transitionData = RuleTransitionData.create( target.getAssociatedRule(), - configConditions != null ? configConditions.asProviders() : null, + configConditions.asProviders(), preRuleTransitionKey.getConfigurationKey().getOptionsChecksum()); ConfigurationTransition transition = null; @@ -506,16 +520,6 @@ public void acceptTransitionError(OptionsParsingException e) { emitTransitionErrorMessage(e.getMessage()); } - @Override - public void acceptConfigConditionsError(ConfiguredValueCreationException e) { - emitTransitionErrorMessage(e.getMessage()); - } - - @Override - public void acceptConfigConditions(ConfigConditions configConditions) { - this.configConditions = configConditions; - } - @Override public void acceptPlatformInfo(PlatformInfo info) { this.platformInfo = info; diff --git a/src/main/java/com/google/devtools/build/skyframe/state/Lookup.java b/src/main/java/com/google/devtools/build/skyframe/state/Lookup.java index a9d4c21f491680..77682f3e7c7665 100644 --- a/src/main/java/com/google/devtools/build/skyframe/state/Lookup.java +++ b/src/main/java/com/google/devtools/build/skyframe/state/Lookup.java @@ -19,6 +19,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import com.google.errorprone.annotations.ForOverride; import java.util.function.Consumer; /** Captures information about a lookup requested by a state machine. */ @@ -48,22 +49,24 @@ final SkyKey key() { @Override public final void acceptValue(SkyKey unusedKey, SkyValue value) { - acceptValue(value); + acceptValueInternal(value); parent.signalChildDoneAndEnqueueIfReady(); } - abstract void acceptValue(SkyValue value); + @ForOverride + protected abstract void acceptValueInternal(SkyValue value); @Override public final boolean tryHandleException(SkyKey unusedKey, Exception exception) { - boolean handled = tryHandleException(exception); + boolean handled = tryHandleExceptionInternal(exception); if (handled) { parent.signalChildDoneAndEnqueueIfReady(); } return handled; } - abstract boolean tryHandleException(Exception exception); + @ForOverride + protected abstract boolean tryHandleExceptionInternal(Exception exception); static final class ConsumerLookup extends Lookup { private final Consumer sink; @@ -84,12 +87,12 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { } @Override - void acceptValue(SkyValue value) { + protected void acceptValueInternal(SkyValue value) { sink.accept(value); } @Override - boolean tryHandleException(Exception unusedException) { + protected boolean tryHandleExceptionInternal(Exception unusedException) { return false; } } @@ -120,7 +123,7 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { if (e instanceof InterruptedException) { throw (InterruptedException) e; } - if (!tryHandleException(e)) { + if (!tryHandleException(key, e)) { throw new IllegalArgumentException("Unexpected exception for " + key(), e); } } @@ -128,12 +131,12 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { } @Override - void acceptValue(SkyValue value) { + protected void acceptValueInternal(SkyValue value) { sink.acceptValueOrException(value, /* exception= */ null); } @Override - boolean tryHandleException(Exception exception) { + protected boolean tryHandleExceptionInternal(Exception exception) { if (exceptionClass.isInstance(exception)) { sink.acceptValueOrException(/* value= */ null, exceptionClass.cast(exception)); return true; @@ -172,7 +175,7 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { if (e instanceof InterruptedException) { throw (InterruptedException) e; } - if (!tryHandleException(e)) { + if (!tryHandleException(key, e)) { throw new IllegalArgumentException("Unexpected exception for " + key(), e); } } @@ -180,12 +183,12 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { } @Override - void acceptValue(SkyValue value) { + protected void acceptValueInternal(SkyValue value) { sink.acceptValueOrException2(value, /* e1= */ null, /* e2= */ null); } @Override - boolean tryHandleException(Exception exception) { + protected boolean tryHandleExceptionInternal(Exception exception) { if (exceptionClass1.isInstance(exception)) { sink.acceptValueOrException2( /* value= */ null, exceptionClass1.cast(exception), /* e2= */ null); @@ -235,7 +238,7 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { if (e instanceof InterruptedException) { throw (InterruptedException) e; } - if (!tryHandleException(e)) { + if (!tryHandleException(key, e)) { throw new IllegalArgumentException("Unexpected exception for " + key(), e); } } @@ -243,12 +246,12 @@ boolean doLookup(LookupEnvironment env) throws InterruptedException { } @Override - void acceptValue(SkyValue value) { + protected void acceptValueInternal(SkyValue value) { sink.acceptValueOrException3(value, /* e1= */ null, /* e2= */ null, /* e3= */ null); } @Override - boolean tryHandleException(Exception exception) { + protected boolean tryHandleExceptionInternal(Exception exception) { if (exceptionClass1.isInstance(exception)) { sink.acceptValueOrException3( /* value= */ null, exceptionClass1.cast(exception), /* e2= */ null, /* e3= */ null); diff --git a/src/test/java/com/google/devtools/build/skyframe/StateMachineTest.java b/src/test/java/com/google/devtools/build/skyframe/StateMachineTest.java index 758922f987ce89..291f09b4c933c5 100644 --- a/src/test/java/com/google/devtools/build/skyframe/StateMachineTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/StateMachineTest.java @@ -402,6 +402,11 @@ public void handledException(@TestParameter boolean keepGoing) throws Interrupte private static class StringOrExceptionProducer extends ValueOrExceptionProducer implements SkyKeyComputeState { + // Static boolean isProcessValueOrExceptionCalled is added to verify StateMachine chained after + // `step()` is invoked regardless of KEY_A1 looks up ends with a value or an exception. + // See b/290998109#comment6. + public static boolean isProcessValueOrExceptionCalled = false; + @Override public StateMachine step(Tasks tasks) { tasks.lookUp( @@ -414,7 +419,10 @@ public StateMachine step(Tasks tasks) { } setException(e); }); - return DONE; + return t -> { + isProcessValueOrExceptionCalled = true; + return DONE; + }; } } @@ -437,6 +445,7 @@ public void valueOrExceptionProducer_propagatesValues() throws InterruptedExcept return DONE_VALUE; }); assertThat(eval(ROOT_KEY, /* keepGoing= */ false).get(ROOT_KEY)).isEqualTo(DONE_VALUE); + assertThat(StringOrExceptionProducer.isProcessValueOrExceptionCalled).isTrue(); } @Test @@ -470,6 +479,7 @@ public void valueOrExceptionProducer_propagatesExceptions(@TestParameter boolean assertThat(result.get(ROOT_KEY)).isNull(); assertThatEvaluationResult(result).hasSingletonErrorThat(KEY_A1); } + assertThat(StringOrExceptionProducer.isProcessValueOrExceptionCalled).isTrue(); } /** @@ -637,6 +647,121 @@ public void valueOrException2Producer_propagatesExceptions( } } + /** + * {@link #valueOrException2Producer_singleLookup_propagatesValuesAndInvokesRunAfter} and {@link + * #valueOrException2Producer_singleLookup_propagatesExceptionsAndInvokesRunAfter} are added in + * order to verify that if looking up the SkyKey throws an exception, the runAfter {@link + * StateMachine} defined as the return of {@link StringOrException2ProducerWithSingleLookup#step} + * is invoked. + * + *

These tests are designed not to be integrated into {@link + * #valueOrException2Producer_propagatesValues} and {@link + * #valueOrException2Producer_propagatesExceptions}. The reason is that only when {@link + * Driver#drive} looks up **one** newly added {@link SkyKey}, will {@link Lookup#doLookup} be + * called. And these tests aim at covering calling this method. + * + *

Similar tests for {@link ValueOrException3Producer} are also added below. + * + *

See b/290998109#comment6 for more details. + */ + private static class StringOrException2ProducerWithSingleLookup + extends ValueOrException2Producer + implements SkyKeyComputeState { + public static boolean isProcessValueOrExceptionCalled = false; + + @Override + public StateMachine step(Tasks tasks) { + tasks.lookUp( + KEY_A1, + SomeErrorException1.class, + SomeErrorException2.class, + (v, e1, e2) -> { + if (v != null) { + setValue((StringValue) v); + } + if (e1 != null) { + setException1(new SomeErrorException1(e1.getMessage())); + } + if (e2 != null) { + setException2(new SomeErrorException2(e2.getMessage())); + } + }); + return t -> { + if (getException1() == null && getException2() == null) { + setValue(SUCCESS_VALUE); + } + isProcessValueOrExceptionCalled = true; + return DONE; + }; + } + } + + @Test + public void valueOrException2Producer_singleLookup_propagatesValuesAndInvokesRunAfter() + throws InterruptedException { + tester + .getOrCreate(ROOT_KEY) + .setBuilder( + (k, env) -> { + var producer = env.getState(StringOrException2ProducerWithSingleLookup::new); + SkyValue value; + try { + if ((value = producer.tryProduceValue(env)) == null) { + return null; + } + assertThat(value).isEqualTo(SUCCESS_VALUE); + } catch (SomeErrorException e) { + fail("Unexpecteded exception: " + e); + } + return DONE_VALUE; + }); + assertThat(eval(ROOT_KEY, /* keepGoing= */ false).get(ROOT_KEY)).isEqualTo(DONE_VALUE); + assertThat(StringOrException2ProducerWithSingleLookup.isProcessValueOrExceptionCalled).isTrue(); + } + + @Test + public void valueOrException2Producer_singleLookup_propagatesExceptionsAndInvokesRunAfter( + @TestParameter boolean trueForException1) throws InterruptedException { + var hasRestarted = new AtomicBoolean(false); + tester + .getOrCreate(KEY_A1) + .unsetConstantValue() + .setBuilder( + (k, env) -> { + throw new ExceptionWrapper( + trueForException1 + ? new SomeErrorException1("Exception 1") + : new SomeErrorException2("Exception 2")); + }); + + tester + .getOrCreate(ROOT_KEY) + .setBuilder( + (k, env) -> { + var producer = env.getState(StringOrException2ProducerWithSingleLookup::new); + if (!hasRestarted.getAndSet(true)) { + try { + assertThat(producer.tryProduceValue(env)).isNull(); + } catch (SomeErrorException e) { + fail("Unexpecteded exception: " + e); + } + return null; + } + if (trueForException1) { + assertThrows(SomeErrorException1.class, () -> producer.tryProduceValue(env)); + } else { + assertThrows(SomeErrorException2.class, () -> producer.tryProduceValue(env)); + } + return DONE_VALUE; + }); + + var result = eval(ROOT_KEY, /* keepGoing= */ false); + + assertThat(result.get(ROOT_KEY)).isNull(); + assertThatEvaluationResult(result).hasSingletonErrorThat(KEY_A1); + assertThat(StringOrException2ProducerWithSingleLookup.isProcessValueOrExceptionCalled).isTrue(); + } + private static class StringOrException3Producer extends ValueOrException3Producer< StringValue, SomeErrorException1, SomeErrorException2, SomeErrorException3> @@ -763,6 +888,125 @@ public void valueOrException3Producer_propagatesExceptions( } } + /** See the comments above {@link StringOrException2ProducerWithSingleLookup} for more details. */ + private static class StringOrException3ProducerWithSingleLookup + extends ValueOrException3Producer< + StringValue, SomeErrorException1, SomeErrorException2, SomeErrorException3> + implements SkyKeyComputeState { + public static boolean isProcessValueOrExceptionCalled = false; + + @Override + public StateMachine step(Tasks tasks) { + tasks.lookUp( + KEY_A1, + SomeErrorException1.class, + SomeErrorException2.class, + SomeErrorException3.class, + (v, e1, e2, e3) -> { + if (v != null) { + setValue((StringValue) v); + } + if (e1 != null) { + setException1(new SomeErrorException1(e1.getMessage())); + } + if (e2 != null) { + setException2(new SomeErrorException2(e2.getMessage())); + } + if (e3 != null) { + setException3(new SomeErrorException3(e3.getMessage())); + } + }); + return t -> { + if (getException1() == null && getException2() == null && getException3() == null) { + setValue(SUCCESS_VALUE); + } + isProcessValueOrExceptionCalled = true; + return DONE; + }; + } + } + + @Test + public void valueOrException3Producer_singleLookup_propagatesValues() + throws InterruptedException { + tester + .getOrCreate(ROOT_KEY) + .setBuilder( + (k, env) -> { + var producer = env.getState(StringOrException3ProducerWithSingleLookup::new); + SkyValue value; + try { + if ((value = producer.tryProduceValue(env)) == null) { + return null; + } + assertThat(value).isEqualTo(SUCCESS_VALUE); + } catch (SomeErrorException e) { + fail("Unexpecteded exception: " + e); + } + return DONE_VALUE; + }); + assertThat(eval(ROOT_KEY, /* keepGoing= */ false).get(ROOT_KEY)).isEqualTo(DONE_VALUE); + assertThat(StringOrException3ProducerWithSingleLookup.isProcessValueOrExceptionCalled).isTrue(); + } + + @Test + public void valueOrException3Producer_singleLookup_propagatesExceptionsAndExecuteRunAfter( + @TestParameter ValueOrException3ExceptionCase exceptionCase) throws InterruptedException { + var hasRestarted = new AtomicBoolean(false); + tester + .getOrCreate(KEY_A1) + .unsetConstantValue() + .setBuilder( + (k, env) -> { + Exception exception = null; + switch (exceptionCase) { + case ONE: + exception = new SomeErrorException1("Exception 1"); + break; + case TWO: + exception = new SomeErrorException2("Exception 2"); + break; + case THREE: + exception = new SomeErrorException3("Exception 3"); + break; + } + throw new ExceptionWrapper(exception); + }); + + tester + .getOrCreate(ROOT_KEY) + .setBuilder( + (k, env) -> { + var producer = env.getState(StringOrException3ProducerWithSingleLookup::new); + if (!hasRestarted.getAndSet(true)) { + try { + assertThat(producer.tryProduceValue(env)).isNull(); + } catch (SomeErrorException e) { + fail("Unexpecteded exception: " + e); + } + return null; + } + switch (exceptionCase) { + case ONE: + assertThrows(SomeErrorException1.class, () -> producer.tryProduceValue(env)); + break; + case TWO: + assertThrows(SomeErrorException2.class, () -> producer.tryProduceValue(env)); + break; + case THREE: + assertThrows(SomeErrorException3.class, () -> producer.tryProduceValue(env)); + break; + } + return DONE_VALUE; + }); + + var result = eval(ROOT_KEY, /* keepGoing= */ false); + + assertThat(result.get(ROOT_KEY)).isNull(); + assertThatEvaluationResult(result).hasSingletonErrorThat(KEY_A1); + assertThat(StringOrException3ProducerWithSingleLookup.isProcessValueOrExceptionCalled).isTrue(); + } + @Test public void lookupValue_matrix( @TestParameter LookupType lookupType,