diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index a047165ce940cc..aec41c328dc484 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -85,6 +85,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException2; +import com.google.devtools.build.skyframe.ValueOrUntypedException; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -195,12 +196,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } // Keep this in sync with the same code in ConfiguredTargetFunction. - PackageValue packageValue; - try { - packageValue = (PackageValue) baseValues.get(packageKey).get(); - } catch (ConfiguredValueCreationException | BzlLoadFailedException e) { - throw new IllegalStateException(e); - } + PackageValue packageValue = (PackageValue) baseValues.get(packageKey).getUnchecked(); if (packageValue == null) { return null; } @@ -216,12 +212,13 @@ public SkyValue compute(SkyKey skyKey, Environment env) ConfiguredTargetValue baseConfiguredTargetValue; try { baseConfiguredTargetValue = - (ConfiguredTargetValue) baseValues.get(baseConfiguredTargetKey).get(); + (ConfiguredTargetValue) + baseValues + .get(baseConfiguredTargetKey) + .getOrThrow(ConfiguredValueCreationException.class); } catch (ConfiguredValueCreationException e) { throw new AspectFunctionException( new AspectCreationException(e.getMessage(), e.getRootCauses(), e.getDetailedExitCode())); - } catch (BzlLoadFailedException e) { - throw new IllegalStateException(e); } ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget(); Preconditions.checkState( @@ -230,16 +227,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) key, associatedTarget); - BuildConfigurationValue configuration; - if (configurationKey == null) { - configuration = null; - } else { - try { - configuration = (BuildConfigurationValue) baseValues.get(configurationKey).get(); - } catch (ConfiguredValueCreationException | BzlLoadFailedException e) { - throw new IllegalStateException(e); - } - } + BuildConfigurationValue configuration = + configurationKey == null + ? null + : (BuildConfigurationValue) baseValues.get(configurationKey).getUnchecked(); PackageValue val = (PackageValue) @@ -493,23 +484,19 @@ static StarlarkDefinedAspect loadAspectFromBzl( @Nullable private static StarlarkDefinedAspect loadStarlarkAspect( - StarlarkAspectClass starlarkAspectClass, - ValueOrException2 bzlLoadValueOrException) + StarlarkAspectClass starlarkAspectClass, ValueOrUntypedException bzlLoadValueOrException) throws AspectCreationException { BzlLoadValue bzlLoadValue; try { - bzlLoadValue = (BzlLoadValue) bzlLoadValueOrException.get(); + bzlLoadValue = + (BzlLoadValue) bzlLoadValueOrException.getOrThrow(BzlLoadFailedException.class); } catch (BzlLoadFailedException e) { throw new AspectCreationException( e.getMessage(), starlarkAspectClass.getExtensionLabel(), e.getDetailedExitCode()); - } catch (Exception e) { - throw new IllegalStateException(e); } - if (bzlLoadValue == null) { return null; } - return loadAspectFromBzl(starlarkAspectClass, bzlLoadValue); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 233f6c8023cb44..599dfe96c9fd8e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -88,6 +88,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException; +import com.google.devtools.build.skyframe.ValueOrUntypedException; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -848,67 +849,9 @@ private static Map resolveConfiguredTargetDepen for (int i = 0; i < 2; i++) { for (Dependency dep : depsToProcess) { SkyKey key = dep.getConfiguredTargetKey(); + ConfiguredTargetValue depValue; try { - ConfiguredTargetValue depValue = - (ConfiguredTargetValue) depValuesOrExceptions.get(key).get(); - - if (depValue == null) { - missedValues = true; - } else { - ConfiguredTarget depCt = depValue.getConfiguredTarget(); - Label depLabel = depCt.getLabel(); - SkyKey packageKey = PackageValue.key(depLabel.getPackageIdentifier()); - PackageValue pkgValue; - if (i == 0) { - ValueOrException packageResult = - depValuesOrExceptions.get(packageKey); - if (packageResult == null) { - aliasPackagesToFetch.add(packageKey); - aliasDepsToRedo.add(dep); - continue; - } else { - pkgValue = (PackageValue) packageResult.get(); - if (pkgValue == null) { - // In a race, the getValuesOrThrow call above may have retrieved the package - // before it was done but the configured target after it was done. Since - // SkyFunctionEnvironment may cache absent values, re-requesting it on this - // evaluation may be useless, just treat it as missing. - missedValues = true; - continue; - } - } - } else { - // We were doing AliasConfiguredTarget mop-up. - pkgValue = (PackageValue) aliasPackageValues.get(packageKey); - if (pkgValue == null) { - // This is unexpected: on the second iteration, all packages should be present, - // since the configured targets that depend on them are present. But since that is - // not a guarantee Skyframe makes, we tolerate their absence. - missedValues = true; - continue; - } - } - try { - BuildConfigurationValue depConfiguration = dep.getConfiguration(); - BuildConfigurationKey depKey = depValue.getConfiguredTarget().getConfigurationKey(); - if (depKey != null && !depKey.equals(depConfiguration.getKey())) { - depConfiguration = (BuildConfigurationValue) env.getValue(depKey); - } - result.put( - key, - new ConfiguredTargetAndData( - depValue.getConfiguredTarget(), - pkgValue.getPackage().getTarget(depLabel.getName()), - depConfiguration, - dep.getTransitionKeys())); - } catch (NoSuchTargetException e) { - throw new IllegalStateException("Target already verified for " + dep, e); - } - if (transitivePackagesForPackageRootResolution != null) { - transitivePackagesForPackageRootResolution.addTransitive( - depValue.getTransitivePackagesForPackageRootResolution()); - } - } + depValue = (ConfiguredTargetValue) depValuesOrExceptions.get(key).get(); } catch (ConfiguredValueCreationException e) { transitiveRootCauses.addTransitive(e.getRootCauses()); detailedExitCode = @@ -917,24 +860,82 @@ private static Map resolveConfiguredTargetDepen if (e.getDetailedExitCode().equals(detailedExitCode)) { rootError = e; } + continue; + } + if (depValue == null) { + missedValues = true; + continue; + } + + ConfiguredTarget depCt = depValue.getConfiguredTarget(); + Label depLabel = depCt.getLabel(); + SkyKey packageKey = PackageValue.key(depLabel.getPackageIdentifier()); + PackageValue pkgValue; + if (i == 0) { + ValueOrUntypedException packageResult = depValuesOrExceptions.get(packageKey); + if (packageResult == null) { + aliasPackagesToFetch.add(packageKey); + aliasDepsToRedo.add(dep); + continue; + } else { + pkgValue = (PackageValue) packageResult.getUnchecked(); + if (pkgValue == null) { + // In a race, the getValuesOrThrow call above may have retrieved the package before it + // was done but the configured target after it was done. Since SkyFunctionEnvironment + // may cache absent values, re-requesting it on this evaluation may be useless, just + // treat it as missing. + missedValues = true; + continue; + } + } + } else { + // We were doing AliasConfiguredTarget mop-up. + pkgValue = (PackageValue) aliasPackageValues.get(packageKey); + if (pkgValue == null) { + // This is unexpected: on the second iteration, all packages should be present, since + // the configured targets that depend on them are present. But since that is not a + // guarantee Skyframe makes, we tolerate their absence. + missedValues = true; + continue; + } + } + + try { + BuildConfigurationValue depConfiguration = dep.getConfiguration(); + BuildConfigurationKey depKey = depValue.getConfiguredTarget().getConfigurationKey(); + if (depKey != null && !depKey.equals(depConfiguration.getKey())) { + depConfiguration = (BuildConfigurationValue) env.getValue(depKey); + } + result.put( + key, + new ConfiguredTargetAndData( + depValue.getConfiguredTarget(), + pkgValue.getPackage().getTarget(depLabel.getName()), + depConfiguration, + dep.getTransitionKeys())); + } catch (NoSuchTargetException e) { + throw new IllegalStateException("Target already verified for " + dep, e); + } + if (transitivePackagesForPackageRootResolution != null) { + transitivePackagesForPackageRootResolution.addTransitive( + depValue.getTransitivePackagesForPackageRootResolution()); } } + if (aliasDepsToRedo.isEmpty()) { break; } aliasPackageValues = env.getValues(aliasPackagesToFetch); depsToProcess = aliasDepsToRedo; } + if (rootError != null) { throw new DependencyEvaluationException( new ConfiguredValueCreationException( ctgValue, rootError.getMessage(), transitiveRootCauses.build(), detailedExitCode), /*depReportedOwnError=*/ true); - } else if (missedValues) { - return null; - } else { - return result; } + return missedValues ? null : result; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java index d5f4edeb289302..57294aaf9372c7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java @@ -42,7 +42,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -135,15 +134,11 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( SkyKey pkgLookupKey = PackageLookupValue.key(packageId); SkyKey dirListingKey = DirectoryListingValue.key(rootedPath); - Map< - SkyKey, - ValueOrException2< - NoSuchPackageException, IOException>> - pkgLookupAndDirectoryListingDeps = - env.getValuesOrThrow( - ImmutableList.of(pkgLookupKey, dirListingKey), - NoSuchPackageException.class, - IOException.class); + List> pkgLookupAndDirectoryListingDeps = + env.getOrderedValuesOrThrow( + ImmutableList.of(pkgLookupKey, dirListingKey), + NoSuchPackageException.class, + IOException.class); if (env.valuesMissing()) { return null; } @@ -152,7 +147,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( pkgLookupValue = (PackageLookupValue) Preconditions.checkNotNull( - pkgLookupAndDirectoryListingDeps.get(pkgLookupKey).get(), + pkgLookupAndDirectoryListingDeps.get(0).get(), "%s %s %s", rootedPath, repositoryName, @@ -167,7 +162,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( dirListingValue = (DirectoryListingValue) Preconditions.checkNotNull( - pkgLookupAndDirectoryListingDeps.get(dirListingKey).get(), + pkgLookupAndDirectoryListingDeps.get(1).getOrThrow(IOException.class), "%s %s %s", rootedPath, repositoryName, @@ -177,12 +172,10 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( // but FileFunction was evaluated for rootedPath above, and didn't throw there. It shouldn't // be able to avoid throwing there but throw here. throw new IllegalStateException( - "Symlink cycle found after not being found for \"" + rootedPath + "\""); + "Symlink cycle found after not being found for \"" + rootedPath + "\"", e); } catch (IOException e) { return reportErrorAndReturn( "Failed to list directory contents", e, rootRelativePath, env.getListener()); - } catch (NoSuchPackageException e) { - throw new IllegalStateException(e); } StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); if (env.valuesMissing()) { @@ -196,8 +189,7 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( repositoryName, excludedPaths, starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)), - /** additionalValuesToAggregate= */ - ImmutableMap.of()); + /*additionalValuesToAggregate=*/ ImmutableMap.of()); } private Iterable getSubdirDeps( diff --git a/src/main/java/com/google/devtools/build/skyframe/ValueOrUntypedException.java b/src/main/java/com/google/devtools/build/skyframe/ValueOrUntypedException.java index 7147c2932d82ab..92f66c6d844daa 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ValueOrUntypedException.java +++ b/src/main/java/com/google/devtools/build/skyframe/ValueOrUntypedException.java @@ -13,17 +13,53 @@ // limitations under the License. package com.google.devtools.build.skyframe; -import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Throwables; import javax.annotation.Nullable; /** * Wrapper for a value or the untyped exception thrown when trying to compute it. * - *

This is an implementation detail of {@link ParallelEvaluator}. It's an abstract class (as - * opposed to an interface) to avoid exposing the methods outside the package. + *

Sky functions should generally prefer using exception-typed subclasses such as {@link + * ValueOrException}, {@link ValueOrException2}, etc. Occasionally, it may be useful to work with + * {@link ValueOrUntypedException} instead, for example to share logic for processing Skyframe + * requests that use different exception types or arity. */ -@VisibleForTesting public abstract class ValueOrUntypedException { + + /** + * Returns the stored value, or {@code null} if it has not yet been computed. If an exception is + * stored, it is wrapped in {@link IllegalStateException} and thrown. + * + *

This method is useful when a value is not expected to store an exception but was requested + * in the same batch as values which may store an exception. + */ + @Nullable + public final SkyValue getUnchecked() { + Exception e = getException(); + if (e != null) { + throw new IllegalStateException(e); + } + return getValue(); + } + + /** + * Returns the stored value, or {@code null} if it has not yet been computed. If an exception is + * stored, it is thrown as-is if it is an instance of {@code exceptionClass} and wrapped in {@link + * IllegalStateException} otherwise. + * + *

This method is useful when a value is only expected to store {@code exceptionClass} but was + * requested in the same batch as values which may store other types of exceptions. + */ + @Nullable + public final SkyValue getOrThrow(Class exceptionClass) throws E { + Exception e = getException(); + if (e != null) { + Throwables.throwIfInstanceOf(e, exceptionClass); + throw new IllegalStateException(e); + } + return getValue(); + } + /** Returns the stored value, if there was one. */ @Nullable abstract SkyValue getValue(); @@ -44,7 +80,7 @@ public static ValueOrUntypedException ofExn(Exception e) { return new ValueOrUntypedExceptionExnImpl(e); } - private static class ValueOrUntypedExceptionImpl extends ValueOrUntypedException { + private static final class ValueOrUntypedExceptionImpl extends ValueOrUntypedException { static final ValueOrUntypedExceptionImpl NULL = new ValueOrUntypedExceptionImpl(null); @Nullable private final SkyValue value; @@ -64,7 +100,7 @@ public Exception getException() { } } - private static class ValueOrUntypedExceptionExnImpl extends ValueOrUntypedException { + private static final class ValueOrUntypedExceptionExnImpl extends ValueOrUntypedException { private final Exception exception; ValueOrUntypedExceptionExnImpl(Exception exception) {