Return helpful no-such-package errors for build settings and actually error out of the build for rule transition errors (i.e. errors that happen in SkyframeExecutor#getConfigurations) PiperOrigin-RevId: 249831851
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java index 9f224bb..1e37e93 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.ConfigurationResolver; +import com.google.devtools.build.lib.analysis.config.ConfigurationResolver.TopLevelTargetsAndConfigsResult; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.config.TransitionResolver; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; @@ -38,7 +39,6 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; import java.util.LinkedHashSet; -import java.util.List; /** * Utility functions for use during analysis. @@ -179,7 +179,7 @@ * <p>Preserves the original input ordering. */ // Keep this in sync with PrepareAnalysisPhaseFunction. - public static List<TargetAndConfiguration> getTargetsWithConfigs( + public static TopLevelTargetsAndConfigsResult getTargetsWithConfigs( BuildConfigurationCollection configurations, Collection<Target> targets, ExtendedEventHandler eventHandler, @@ -201,9 +201,8 @@ Multimap<BuildConfiguration, Dependency> asDeps = AnalysisUtils.targetsToDeps(nodes, ruleClassProvider); - return ImmutableList.copyOf( - ConfigurationResolver.getConfigurationsFromExecutor( - nodes, asDeps, eventHandler, skyframeExecutor)); + return ConfigurationResolver.getConfigurationsFromExecutor( + nodes, asDeps, eventHandler, skyframeExecutor); } @VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 33d20e1..1556fc2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -37,6 +37,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.ConfigurationResolver.TopLevelTargetsAndConfigsResult; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.constraints.TopLevelConstraintSemantics; import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory; @@ -222,7 +223,7 @@ // Prepare the analysis phase BuildConfigurationCollection configurations; - Collection<TargetAndConfiguration> topLevelTargetsWithConfigs; + TopLevelTargetsAndConfigsResult topLevelTargetsWithConfigsResult; if (viewOptions.skyframePrepareAnalysis) { PrepareAnalysisPhaseValue prepareAnalysisPhaseValue; try (SilentCloseable c = Profiler.instance().profile("Prepare analysis phase")) { @@ -232,7 +233,7 @@ // Determine the configurations configurations = prepareAnalysisPhaseValue.getConfigurations(eventHandler, skyframeExecutor); - topLevelTargetsWithConfigs = + topLevelTargetsWithConfigsResult = prepareAnalysisPhaseValue.getTopLevelCts(eventHandler, skyframeExecutor); } } else { @@ -249,7 +250,7 @@ keepGoing); } try (SilentCloseable c = Profiler.instance().profile("AnalysisUtils.getTargetsWithConfigs")) { - topLevelTargetsWithConfigs = + topLevelTargetsWithConfigsResult = AnalysisUtils.getTargetsWithConfigs( configurations, targets, eventHandler, ruleClassProvider, skyframeExecutor); } @@ -265,6 +266,9 @@ configurations.getTargetConfigurations().get(0).getMakeEnvironment())); } + Collection<TargetAndConfiguration> topLevelTargetsWithConfigs = + topLevelTargetsWithConfigsResult.getTargetsAndConfigs(); + // Report the generated association of targets to configurations Multimap<Label, BuildConfiguration> byLabel = ArrayListMultimap.<Label, BuildConfiguration>create(); @@ -430,7 +434,7 @@ viewOptions, skyframeAnalysisResult, targetsToSkip, - topLevelTargetsWithConfigs); + topLevelTargetsWithConfigsResult); logger.info("Finished analysis"); return result; } @@ -444,7 +448,7 @@ AnalysisOptions viewOptions, SkyframeAnalysisResult skyframeAnalysisResult, Set<ConfiguredTarget> targetsToSkip, - Collection<TargetAndConfiguration> topLevelTargetsWithConfigs) + TopLevelTargetsAndConfigsResult topLevelTargetsWithConfigs) throws InterruptedException { Set<Label> testsToRun = loadingResult.getTestsToRunLabels(); Set<ConfiguredTarget> configuredTargets = @@ -503,7 +507,8 @@ skyframeExecutor, eventHandler); - String error = createErrorMessage(loadingResult, skyframeAnalysisResult); + String error = + createErrorMessage(loadingResult, skyframeAnalysisResult, topLevelTargetsWithConfigs); final WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph(); final ActionGraph actionGraph = @@ -540,21 +545,32 @@ topLevelOptions, skyframeAnalysisResult.getPackageRoots(), loadingResult.getWorkspaceName(), - topLevelTargetsWithConfigs); + topLevelTargetsWithConfigs.getTargetsAndConfigs()); } + /** + * Check for errors in "chronological" order (acknowledge that loading and analysis are + * interleaved, but sequential on the single target scale). + */ @Nullable public static String createErrorMessage( TargetPatternPhaseValue loadingResult, - @Nullable SkyframeAnalysisResult skyframeAnalysisResult) { - return loadingResult.hasError() - ? "command succeeded, but there were errors parsing the target pattern" - : loadingResult.hasPostExpansionError() - || (skyframeAnalysisResult != null && skyframeAnalysisResult.hasLoadingError()) - ? "command succeeded, but there were loading phase errors" - : (skyframeAnalysisResult != null && skyframeAnalysisResult.hasAnalysisError()) - ? "command succeeded, but not all targets were analyzed" - : null; + @Nullable SkyframeAnalysisResult skyframeAnalysisResult, + @Nullable TopLevelTargetsAndConfigsResult topLevelTargetsAndConfigs) { + if (loadingResult.hasError()) { + return "command succeeded, but there were errors parsing the target pattern"; + } + if (loadingResult.hasPostExpansionError() + || (skyframeAnalysisResult != null && skyframeAnalysisResult.hasLoadingError())) { + return "command succeeded, but there were loading phase errors"; + } + if (topLevelTargetsAndConfigs != null && topLevelTargetsAndConfigs.hasError()) { + return "command succeeded, but top level configurations could not be created"; + } + if (skyframeAnalysisResult != null && skyframeAnalysisResult.hasAnalysisError()) { + return "command succeeded, but not all targets were analyzed"; + } + return null; } private static NestedSet<Artifact> getBaselineCoverageArtifacts(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index 55276a8..c61e90c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue; import com.google.devtools.build.lib.skyframe.PlatformMappingValue; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; +import com.google.devtools.build.lib.skyframe.SkyframeExecutor.ConfigurationsResult; import com.google.devtools.build.lib.skyframe.TransitiveTargetKey; import com.google.devtools.build.lib.skyframe.TransitiveTargetValue; import com.google.devtools.build.lib.util.OrderedSetMultimap; @@ -672,7 +673,7 @@ // TODO(gregce): merge this more with resolveConfigurations? One crucial difference is // resolveConfigurations can null-return on missing deps since it executes inside Skyfunctions. // Keep this in sync with {@link PrepareAnalysisPhaseFunction#resolveConfigurations}. - public static LinkedHashSet<TargetAndConfiguration> getConfigurationsFromExecutor( + public static TopLevelTargetsAndConfigsResult getConfigurationsFromExecutor( Iterable<TargetAndConfiguration> defaultContext, Multimap<BuildConfiguration, Dependency> targetsToEvaluate, ExtendedEventHandler eventHandler, @@ -688,13 +689,15 @@ // could be successfully Skyframe-evaluated. Map<TargetAndConfiguration, TargetAndConfiguration> successfullyEvaluatedTargets = new LinkedHashMap<>(); + boolean hasError = false; if (!targetsToEvaluate.isEmpty()) { for (BuildConfiguration fromConfig : targetsToEvaluate.keySet()) { - Multimap<Dependency, BuildConfiguration> evaluatedTargets = + ConfigurationsResult configurationsResult = skyframeExecutor.getConfigurations( eventHandler, fromConfig.getOptions(), targetsToEvaluate.get(fromConfig)); + hasError |= configurationsResult.hasError(); for (Map.Entry<Dependency, BuildConfiguration> evaluatedTarget : - evaluatedTargets.entries()) { + configurationsResult.getConfigurationMap().entries()) { Target target = labelsToTargets.get(evaluatedTarget.getKey().getLabel()); successfullyEvaluatedTargets.put( new TargetAndConfiguration(target, fromConfig), @@ -713,7 +716,30 @@ result.add(originalInput); } } - return result; + return new TopLevelTargetsAndConfigsResult(result, hasError); + } + + /** + * The result of {@link #getConfigurationsFromExecutor} which also registers if an error was + * recorded. + */ + public static class TopLevelTargetsAndConfigsResult { + private final Collection<TargetAndConfiguration> configurations; + private final boolean hasError; + + public TopLevelTargetsAndConfigsResult( + Collection<TargetAndConfiguration> configurations, boolean hasError) { + this.configurations = configurations; + this.hasError = hasError; + } + + public boolean hasError() { + return hasError; + } + + public Collection<TargetAndConfiguration> getTargetsAndConfigs() { + return configurations; + } } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java index 4f955ea..daec57e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java
@@ -80,6 +80,7 @@ } /** Exception class for exceptions thrown during application of a starlark-defined transition */ + // TODO(juliexxia): add more information to this exception e.g. originating target of transition public static class TransitionException extends Exception { private final String message;
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index 638e4ce..bb82cf2 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -138,7 +138,7 @@ env.getReporter().handle(Event.progress("Loading complete.")); env.getReporter().post(new NoAnalyzeEvent()); logger.atInfo().log("No analysis requested, so finished"); - String errorMessage = BuildView.createErrorMessage(loadingResult, null); + String errorMessage = BuildView.createErrorMessage(loadingResult, null, null); if (errorMessage != null) { throw new BuildFailedException(errorMessage); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java index 9743624..6c8feb0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseValue.java
@@ -23,11 +23,13 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.ConfigurationResolver.TopLevelTargetsAndConfigsResult; import com.google.devtools.build.lib.analysis.config.FragmentClassSet; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; @@ -92,15 +94,17 @@ * Returns the intended top-level targets and configurations for the build. Note that this * performs additional Skyframe calls for the involved configurations and targets, which may be * expensive. + * + * <p>Skips targets that have errors and registers the errors to be reported later as part of + * {@link com.google.devtools.build.lib.analysis.AnalysisResult} error resolution. */ - public Collection<TargetAndConfiguration> getTopLevelCts( + public TopLevelTargetsAndConfigsResult getTopLevelCts( ExtendedEventHandler eventHandler, SkyframeExecutor skyframeExecutor) { List<TargetAndConfiguration> result = new ArrayList<>(); Map<BuildConfigurationValue.Key, BuildConfiguration> configs = skyframeExecutor.getConfigurations( eventHandler, - topLevelCtKeys - .stream() + topLevelCtKeys.stream() .map(ctk -> ctk.getConfigurationKey()) .filter(Predicates.notNull()) .collect(Collectors.toSet())); @@ -108,18 +112,22 @@ // TODO(ulfjack): This performs one Skyframe call per top-level target. This is not a // regression, but we should fix it nevertheless, either by doing a bulk lookup call or by // migrating the consumers of these to Skyframe so they can directly request the values. + boolean hasError = false; for (ConfiguredTargetKey key : topLevelCtKeys) { Target target; try { target = skyframeExecutor.getPackageManager().getTarget(eventHandler, key.getLabel()); } catch (NoSuchPackageException | NoSuchTargetException | InterruptedException e) { - throw new RuntimeException("Failed to get package from TargetPatternPhaseValue", e); + eventHandler.handle( + Event.error("Failed to get package from TargetPatternPhaseValue: " + e.getMessage())); + hasError = true; + continue; } BuildConfiguration config = key.getConfigurationKey() == null ? null : configs.get(key.getConfigurationKey()); result.add(new TargetAndConfiguration(target, config)); } - return result; + return new TopLevelTargetsAndConfigsResult(result, hasError); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 501f129..c1f6d2a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1702,12 +1702,13 @@ ExtendedEventHandler eventHandler, BuildConfiguration originalConfig, Iterable<Dependency> keys) - throws TransitionException, InvalidConfigurationException { + throws InvalidConfigurationException { checkActive(); Multimap<Dependency, BuildConfiguration> configs; if (originalConfig != null) { - configs = getConfigurations(eventHandler, originalConfig.getOptions(), keys); + configs = + getConfigurations(eventHandler, originalConfig.getOptions(), keys).getConfigurationMap(); } else { configs = ArrayListMultimap.<Dependency, BuildConfiguration>create(); for (Dependency key : keys) { @@ -1950,11 +1951,10 @@ */ // Keep this in sync with {@link PrepareAnalysisPhaseFunction#getConfigurations}. // TODO(ulfjack): Remove this legacy method after switching to the Skyframe-based implementation. - public Multimap<Dependency, BuildConfiguration> getConfigurations( + public ConfigurationsResult getConfigurations( ExtendedEventHandler eventHandler, BuildOptions fromOptions, Iterable<Dependency> keys) throws InvalidConfigurationException { - Multimap<Dependency, BuildConfiguration> builder = - ArrayListMultimap.<Dependency, BuildConfiguration>create(); + ConfigurationsResult.Builder builder = ConfigurationsResult.newBuilder(); Set<Dependency> depsToEvaluate = new HashSet<>(); ImmutableSortedSet<Class<? extends BuildConfiguration.Fragment>> allFragments = null; @@ -1987,6 +1987,7 @@ // No fragments to compute here. } else if (fragmentsResult.getError(TransitiveTargetKey.of(key.getLabel())) != null) { labelsWithErrors.add(key.getLabel()); + builder.setHasError(); } else { TransitiveTargetValue ttv = (TransitiveTargetValue) fragmentsResult.get(TransitiveTargetKey.of(key.getLabel())); @@ -2022,6 +2023,8 @@ StarlarkTransition.replayEvents(eventHandler, transition); } catch (TransitionException e) { eventHandler.handle(Event.error(e.getMessage())); + builder.setHasError(); + continue; } for (BuildOptions toOption : toOptions) { configSkyKeys.add(toConfigurationKey(platformMappingValue, depFragments, toOption)); @@ -2050,6 +2053,8 @@ fromOptions, key.getTransition(), buildSettingPackages); } catch (TransitionException e) { eventHandler.handle(Event.error(e.getMessage())); + builder.setHasError(); + continue; } for (BuildOptions toOption : toOptions) { BuildConfigurationValue.Key configKey = @@ -2064,7 +2069,53 @@ } } } - return builder; + return builder.build(); + } + + /** + * The result of {@link #getConfigurations(ExtendedEventHandler, BuildOptions, Iterable)} which + * also registers if an error was recorded. + */ + public static class ConfigurationsResult { + private final Multimap<Dependency, BuildConfiguration> configurations; + private final boolean hasError; + + private ConfigurationsResult( + Multimap<Dependency, BuildConfiguration> configurations, boolean hasError) { + this.configurations = configurations; + this.hasError = hasError; + } + + public boolean hasError() { + return hasError; + } + + public Multimap<Dependency, BuildConfiguration> getConfigurationMap() { + return configurations; + } + + public static Builder newBuilder() { + return new Builder(); + } + + /** Builder for {@link ConfigurationsResult} */ + public static class Builder { + private final Multimap<Dependency, BuildConfiguration> configurations = + ArrayListMultimap.<Dependency, BuildConfiguration>create(); + private boolean hasError = false; + + void put(Dependency key, BuildConfiguration value) { + configurations.put(key, value); + } + + void setHasError() { + this.hasError = true; + } + + ConfigurationsResult build() { + return new ConfigurationsResult(configurations, hasError); + } + } } private PlatformMappingValue getPlatformMappingValue( @@ -2100,11 +2151,20 @@ } private Map<SkyKey, SkyValue> collectBuildSettingValues( - ConfigurationTransition transition, ExtendedEventHandler eventHandler) { + ConfigurationTransition transition, ExtendedEventHandler eventHandler) + throws TransitionException { ImmutableSet<SkyKey> buildSettingPackageKeys = StarlarkTransition.getAllBuildSettingPackageKeys(transition); EvaluationResult<SkyValue> buildSettingsResult = evaluateSkyKeys(eventHandler, buildSettingPackageKeys, true); + if (buildSettingsResult.hasError()) { + throw new TransitionException( + new NoSuchPackageException( + ((PackageValue.Key) buildSettingsResult.getError().getRootCauseOfException()) + .argument(), + "Unable to find build setting package", + buildSettingsResult.getError().getException())); + } ImmutableMap.Builder<SkyKey, SkyValue> buildSettingValues = new ImmutableMap.Builder<>(); buildSettingPackageKeys.forEach(k -> buildSettingValues.put(k, buildSettingsResult.get(k))); return buildSettingValues.build();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java index 89de7ca..86bfdc3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -498,7 +498,7 @@ useConfiguration(ImmutableMap.of("//test:cute-animal-fact", "cats can't taste sugar")); reporter.removeHandler(failFastHandler); - getConfiguration(getConfiguredTarget("//test")); + getConfiguredTarget("//test"); assertContainsEvent( "expected value of type 'string' for " + "//test:cute-animal-fact, but got 24 (int)"); } @@ -519,13 +519,33 @@ writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); reporter.removeHandler(failFastHandler); - getConfiguration(getConfiguredTarget("//test")); + getConfiguredTarget("//test"); assertContainsEvent( "no such target '//test:i-am-not-real': target " + "'i-am-not-real' not declared in package 'test'"); } @Test + public void testTransitionOnBuildSetting_noSuchPackage() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + scratch.file( + "test/transitions.bzl", + "def _transition_impl(settings, attr):", + " return {'//i-am-not-real': 'imaginary-friend'}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//i-am-not-real']", + ")"); + writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test"); + assertContainsEvent("no such package 'i-am-not-real': Unable to find build setting package"); + } + + @Test public void testTransitionOnBuildSetting_notABuildSetting() throws Exception { setSkylarkSemanticsOptions( "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api");
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 1157519..ddf1a04 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -191,16 +191,17 @@ @VisibleForTesting public BuildConfiguration getConfigurationForTesting( Target target, BuildConfiguration config, ExtendedEventHandler eventHandler) - throws StarlarkTransition.TransitionException, InvalidConfigurationException { + throws InvalidConfigurationException { List<TargetAndConfiguration> node = ImmutableList.<TargetAndConfiguration>of(new TargetAndConfiguration(target, config)); - LinkedHashSet<TargetAndConfiguration> configs = + Collection<TargetAndConfiguration> configs = ConfigurationResolver.getConfigurationsFromExecutor( - node, - AnalysisUtils.targetsToDeps( - new LinkedHashSet<TargetAndConfiguration>(node), ruleClassProvider), - eventHandler, - skyframeExecutor); + node, + AnalysisUtils.targetsToDeps( + new LinkedHashSet<TargetAndConfiguration>(node), ruleClassProvider), + eventHandler, + skyframeExecutor) + .getTargetsAndConfigs(); return configs.iterator().next().getConfiguration(); }