From 0d89740e2b60fba2f21ec6c98bfc0d33221f8fd1 Mon Sep 17 00:00:00 2001 From: kmb Date: Thu, 22 Apr 2021 09:42:46 -0700 Subject: [PATCH] Use aspect to avoid validation actions blocking test executions PiperOrigin-RevId: 369890289 --- .../lib/analysis/AspectCompleteEvent.java | 29 ++--- .../build/lib/analysis/OutputGroupInfo.java | 41 +++++-- .../lib/analysis/TargetCompleteEvent.java | 15 +++ .../build/lib/buildtool/BuildRequest.java | 27 ++++- .../lib/buildtool/BuildRequestOptions.java | 8 ++ .../lib/buildtool/BuildResultPrinter.java | 41 ++++++- .../com/google/devtools/build/lib/rules/BUILD | 5 + .../build/lib/rules/core/CoreRules.java | 1 + .../build/lib/rules/core/ValidateTarget.java | 63 ++++++++++ .../lib/analysis/OutputGroupProviderTest.java | 53 ++++++-- .../SequencedSkyframeExecutorTest.java | 3 +- .../integration/build_event_stream_test.sh | 16 +++ .../integration/validation_actions_test.sh | 114 +++++++++++++++++- 13 files changed, 361 insertions(+), 55 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/core/ValidateTarget.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java index e1d598f37b0aa7..0a565be62f5368 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java @@ -16,17 +16,13 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CompletionContext; import com.google.devtools.build.lib.actions.EventReportingArtifacts; import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup; -import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer; import com.google.devtools.build.lib.buildeventstream.BuildEventContext; import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId; -import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.OutputGroup; import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint; import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent; import com.google.devtools.build.lib.causes.Cause; @@ -158,31 +154,20 @@ public Collection getChildrenEvents() { @Override public ReportedArtifacts reportedArtifacts() { - ImmutableSet.Builder> builder = ImmutableSet.builder(); - if (artifactOutputGroups != null) { - for (ArtifactsInOutputGroup artifactsInGroup : artifactOutputGroups.values()) { - builder.add(artifactsInGroup.getArtifacts()); - } - } - return new ReportedArtifacts(builder.build(), completionContext); + return TargetCompleteEvent.toReportedArtifacts( + artifactOutputGroups, completionContext, /*baselineCoverageArtifacts=*/ null); } @Override public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) { - ArtifactGroupNamer namer = converters.artifactGroupNamer(); - BuildEventStreamProtos.TargetComplete.Builder builder = BuildEventStreamProtos.TargetComplete.newBuilder(); builder.setSuccess(!failed()); - if (artifactOutputGroups != null) { - artifactOutputGroups.forEach( - (outputGroup, artifactsInGroup) -> - builder.addOutputGroup( - OutputGroup.newBuilder() - .setName(outputGroup) - .setIncomplete(artifactsInGroup.isIncomplete()) - .addFileSets(namer.apply(artifactsInGroup.getArtifacts().toNode())))); - } + builder.addAllOutputGroup( + TargetCompleteEvent.toOutputGroupProtos( + artifactOutputGroups, + converters.artifactGroupNamer(), + /*baselineCoverageArtifacts=*/ null)); return GenericBuildEvent.protoChaining(this).setCompleted(builder.build()).build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java index 7655dc634be280..39d530bd6e6a92 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupInfo.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -112,6 +113,10 @@ public final class OutputGroupInfo extends StructImpl */ public static final String VALIDATION = HIDDEN_OUTPUT_GROUP_PREFIX + "validation"; + /** Helper output group used to request {@link #VALIDATION} outputs from top-level aspect. */ + public static final String VALIDATION_TOP_LEVEL = + HIDDEN_OUTPUT_GROUP_PREFIX + "validation_top_level" + INTERNAL_SUFFIX; + /** * Temporary files created during building a rule, for example, .i, .d and .s files for C++ * compilation. @@ -134,6 +139,21 @@ public final class OutputGroupInfo extends StructImpl public static final ImmutableSortedSet DEFAULT_GROUPS = ImmutableSortedSet.of(DEFAULT, TEMP_FILES, HIDDEN_TOP_LEVEL); + /** Request parameter for {@link #determineOutputGroups}. */ + public enum ValidationMode { + /** Validation outputs not built. */ + OFF, + /** + * Validation outputs built by requesting {@link #VALIDATION} output group Blaze core collects. + */ + OUTPUT_GROUP, + /** + * Validation outputs built by {@code ValidateTarget} aspect "promoting" {@link #VALIDATION} + * output group Blaze core collects to {@link #VALIDATION_TOP_LEVEL} and requesting the latter. + */ + ASPECT; + } + private final ImmutableMap> outputGroups; public OutputGroupInfo(ImmutableMap> outputGroups) { @@ -198,14 +218,13 @@ public static OutputGroupInfo merge(List providers) } public static ImmutableSortedSet determineOutputGroups( - List outputGroups, boolean includeValidationOutputGroup) { - return determineOutputGroups(DEFAULT_GROUPS, outputGroups, includeValidationOutputGroup); + List outputGroups, ValidationMode validationMode) { + return determineOutputGroups(DEFAULT_GROUPS, outputGroups, validationMode); } - public static ImmutableSortedSet determineOutputGroups( - Set defaultOutputGroups, - List outputGroups, - boolean includeValidationOutputGroup) { + @VisibleForTesting + static ImmutableSortedSet determineOutputGroups( + Set defaultOutputGroups, List outputGroups, ValidationMode validationMode) { Set current = Sets.newHashSet(); @@ -235,8 +254,14 @@ public static ImmutableSortedSet determineOutputGroups( } // Add the validation output group regardless of the additions and subtractions above. - if (includeValidationOutputGroup) { - current.add(VALIDATION); + switch (validationMode) { + case OUTPUT_GROUP: + current.add(VALIDATION); + break; + case ASPECT: + current.add(VALIDATION_TOP_LEVEL); + break; + case OFF: // fall out } return ImmutableSortedSet.copyOf(current); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java index 9aa511b18fa079..cf753094146dd7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java @@ -470,6 +470,13 @@ public ImmutableList postedAfter() { @Override public ReportedArtifacts reportedArtifacts() { + return toReportedArtifacts(outputs, completionContext, baselineCoverageArtifacts); + } + + static ReportedArtifacts toReportedArtifacts( + ImmutableMap outputs, + CompletionContext completionContext, + @Nullable NestedSet baselineCoverageArtifacts) { ImmutableSet.Builder> builder = ImmutableSet.builder(); for (ArtifactsInOutputGroup artifactsInGroup : outputs.values()) { if (artifactsInGroup.areImportant()) { @@ -492,6 +499,14 @@ private Iterable getTags() { } private Iterable getOutputFilesByGroup(ArtifactGroupNamer namer) { + return toOutputGroupProtos(outputs, namer, baselineCoverageArtifacts); + } + + /** Returns {@link OutputGroup} protos for given output groups and optional coverage artifacts. */ + static ImmutableList toOutputGroupProtos( + ImmutableMap outputs, + ArtifactGroupNamer namer, + @Nullable NestedSet baselineCoverageArtifacts) { ImmutableList.Builder groups = ImmutableList.builder(); outputs.forEach( (outputGroup, artifactsInOutputGroup) -> { diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java index e29ef7e3617276..e75cb9c725e986 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java @@ -50,6 +50,8 @@ * as --keep_going, --jobs, etc. */ public class BuildRequest implements OptionsProvider { + static final String VALIDATION_ASPECT_NAME = "ValidateTarget"; + private static final ImmutableList> MANDATORY_OPTIONS = ImmutableList.of( BuildRequestOptions.class, @@ -371,8 +373,7 @@ public TopLevelArtifactContext getTopLevelArtifactContext() { getOptions(ExecutionOptions.class).testStrategy.equals("exclusive"), getOptions(BuildEventProtocolOptions.class).expandFilesets, getOptions(BuildEventProtocolOptions.class).fullyResolveFilesetSymlinks, - OutputGroupInfo.determineOutputGroups( - buildOptions.outputGroups, buildOptions.runValidationActions)); + OutputGroupInfo.determineOutputGroups(buildOptions.outputGroups, validationMode())); } public ImmutableSortedSet getMultiCpus() { @@ -380,7 +381,27 @@ public ImmutableSortedSet getMultiCpus() { } public ImmutableList getAspects() { - return ImmutableList.copyOf(getBuildOptions().aspects); + ImmutableList.Builder result = + ImmutableList.builder().addAll(getBuildOptions().aspects); + if (useValidationAspect()) { + result.add(VALIDATION_ASPECT_NAME); + } + return result.build(); + } + + /** Whether {@value #VALIDATION_ASPECT_NAME} is in use. */ + boolean useValidationAspect() { + return validationMode() == OutputGroupInfo.ValidationMode.ASPECT; + } + + private OutputGroupInfo.ValidationMode validationMode() { + BuildRequestOptions buildOptions = getBuildOptions(); + if (!buildOptions.runValidationActions) { + return OutputGroupInfo.ValidationMode.OFF; + } + return buildOptions.useValidationAspect + ? OutputGroupInfo.ValidationMode.ASPECT + : OutputGroupInfo.ValidationMode.OUTPUT_GROUP; } public boolean getCheckForActionConflicts() { diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 818be6dd5a22d7..0ae7da4a6f1631 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -160,6 +160,14 @@ public class BuildRequestOptions extends OptionsBase { help = "Whether to run validation actions as part of the build.") public boolean runValidationActions; + @Option( + name = "experimental_use_validation_aspect", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION, + effectTags = {OptionEffectTag.EXECUTION, OptionEffectTag.AFFECTS_OUTPUTS}, + help = "Whether to run validation actions using aspect (for parallelism with tests).") + public boolean useValidationAspect; + @Option( name = "show_result", defaultValue = "1", diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java index 5329e52cf2a43d..0272123cea41f7 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResultPrinter.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.util.io.OutErr; import java.util.ArrayList; import java.util.Collection; @@ -78,14 +79,38 @@ void showBuildResult( : env.getWorkspace(), request.getBuildOptions().experimentalNoProductNameOutSymlink); OutErr outErr = request.getOutErr(); - Collection targetsToPrint = filterTargetsToPrint(configuredTargets); + + // Produce output as if validation aspect didn't exist; instead we'll consult validation aspect + // if we end up printing targets below. Note that in the presence of other aspects, we may print + // success messages for them but the overall build will still fail if validation aspects (or + // targets) failed. + Collection aspectsToPrint = aspects.keySet(); + if (request.useValidationAspect()) { + aspectsToPrint = + aspectsToPrint.stream() + .filter( + k -> !BuildRequest.VALIDATION_ASPECT_NAME.equals(k.getAspectClass().getName())) + .collect(ImmutableList.toImmutableList()); + } final boolean success; - if (aspects.isEmpty()) { + if (aspectsToPrint.isEmpty()) { // Suppress summary if --show_result value is exceeded: + Collection targetsToPrint = filterTargetsToPrint(configuredTargets); if (targetsToPrint.size() > request.getBuildOptions().maxResultTargets) { return; } - // Filter the targets we care about into three buckets: + + // Filter the targets we care about into three buckets. Targets are only considered successful + // if they and their validation aspects succeeded. Note we determined above that all aspects + // are validation aspects, so just use the full keySet() here. + ImmutableMap validated = + aspects.keySet().stream() + .collect( + ImmutableMap.toImmutableMap( + AspectKey::getBaseConfiguredTargetKey, + k -> result.getSuccessfulAspects().contains(k), + Boolean::logicalAnd)); + Collection succeeded = new ArrayList<>(); Collection failed = new ArrayList<>(); Collection skipped = new ArrayList<>(); @@ -93,8 +118,12 @@ void showBuildResult( for (ConfiguredTarget target : targetsToPrint) { if (configuredTargetsToSkip.contains(target)) { skipped.add(target); + } else if (successfulTargets.contains(target) + && validated.getOrDefault( + ConfiguredTargetKey.builder().setConfiguredTarget(target).build(), Boolean.TRUE)) { + succeeded.add(target); } else { - (successfulTargets.contains(target) ? succeeded : failed).add(target); + failed.add(target); } } @@ -142,14 +171,14 @@ void showBuildResult( success = failed.isEmpty(); } else { // Suppress summary if --show_result value is exceeded: - if (aspects.size() > request.getBuildOptions().maxResultTargets) { + if (aspectsToPrint.size() > request.getBuildOptions().maxResultTargets) { return; } // Filter the targets we care about into two buckets: Collection succeeded = new ArrayList<>(); Collection failed = new ArrayList<>(); ImmutableSet successfulAspects = result.getSuccessfulAspects(); - for (AspectKey aspect : aspects.keySet()) { + for (AspectKey aspect : aspectsToPrint) { (successfulAspects.contains(aspect) ? succeeded : failed).add(aspect); } TopLevelArtifactContext context = request.getTopLevelArtifactContext(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 7108d1d440ff80..467b4b4849c165 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -57,9 +57,14 @@ java_library( ["core/*.java"], ), deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory", + "//src/main/java/com/google/devtools/build/lib/collect/nestedset", + "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java index a0c47d45ed4eef..d2b6c6523b7c59 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java +++ b/src/main/java/com/google/devtools/build/lib/rules/core/CoreRules.java @@ -39,6 +39,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) { builder.addRuleDefinition(new BaseRuleClasses.MakeVariableExpandingRule()); builder.addRuleDefinition(new BaseRuleClasses.BinaryBaseRule()); builder.addRuleDefinition(new BaseRuleClasses.TestBaseRule()); + builder.addNativeAspectClass(new ValidateTarget()); // internally used aspect } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/core/ValidateTarget.java b/src/main/java/com/google/devtools/build/lib/rules/core/ValidateTarget.java new file mode 100644 index 00000000000000..88e53abbd32bb8 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/core/ValidateTarget.java @@ -0,0 +1,63 @@ +// Copyright 2020 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.lib.rules.core; + +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; +import com.google.devtools.build.lib.analysis.ConfiguredAspect; +import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory; +import com.google.devtools.build.lib.analysis.OutputGroupInfo; +import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.packages.AspectDefinition; +import com.google.devtools.build.lib.packages.AspectParameters; +import com.google.devtools.build.lib.packages.NativeAspectClass; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; + +/** + * Non-recursive aspect that promotes {@link OutputGroupInfo#VALIDATION} outputs to {@link + * OutputGroupInfo#VALIDATION_TOP_LEVEL}. By requesting the latter but not the former output group, + * validations avoid blocking test execution. (Using {@link OutputGroupInfo#DEFAULT} would + * accomplish that as well but would be overrideable with {@code --output_groups} flag.) + * + *

Name is chosen to make for semi-sensible "ValidateTarget" aspect events. + */ +class ValidateTarget extends NativeAspectClass implements ConfiguredAspectFactory { + + @Override + public AspectDefinition getDefinition(AspectParameters aspectParameters) { + return AspectDefinition.builder(this) + .applyToFiles(true) // to grab validation outputs from file targets + .build(); + } + + @Override + public ConfiguredAspect create( + ConfiguredTargetAndData ctadBase, + RuleContext context, + AspectParameters parameters, + String toolsRepository) + throws ActionConflictException, InterruptedException { + OutputGroupInfo outputGroupInfo = OutputGroupInfo.get(ctadBase.getConfiguredTarget()); + if (outputGroupInfo != null) { + NestedSet validations = outputGroupInfo.getOutputGroup(OutputGroupInfo.VALIDATION); + if (!validations.isEmpty()) { + return ConfiguredAspect.builder(context) + .addOutputGroup(OutputGroupInfo.VALIDATION_TOP_LEVEL, validations) + .build(); + } + } + return ConfiguredAspect.forNonapplicableTarget(); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/OutputGroupProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/OutputGroupProviderTest.java index b6a084f100a2dc..58588b5c138296 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/OutputGroupProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/OutputGroupProviderTest.java @@ -19,6 +19,7 @@ import static java.util.Arrays.asList; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.OutputGroupInfo.ValidationMode; import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,28 +34,30 @@ public final class OutputGroupProviderTest { @Test public void testDetermineOutputGroupsOverridesDefaults() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("a", "b", "c"), false); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("a", "b", "c"), ValidationMode.OFF); assertThat(outputGroups).containsExactly("a", "b", "c"); } @Test public void testDetermineOutputGroupsAddsToDefaults() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("+a"), false); + determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("+a"), ValidationMode.OFF); assertThat(outputGroups).containsExactly("x", "y", "z", "a"); } @Test public void testDetermineOutputGroupsRemovesFromDefaults() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-y"), false); + determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-y"), ValidationMode.OFF); assertThat(outputGroups).containsExactly("x", "z"); } @Test public void testDetermineOutputGroupsMixedOverrideAdditionOverrides() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("a", "+b"), false); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("a", "+b"), ValidationMode.OFF); // The plain "a" causes the default output groups to be overridden. assertThat(outputGroups).containsExactly("a", "b"); } @@ -62,7 +65,7 @@ public void testDetermineOutputGroupsMixedOverrideAdditionOverrides() throws Exc @Test public void testDetermineOutputGroupsIgnoresUnknownGroup() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-foo"), false); + determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-foo"), ValidationMode.OFF); // "foo" doesn't exist, but that shouldn't be a problem. assertThat(outputGroups).containsExactly("x", "y", "z"); } @@ -70,39 +73,67 @@ public void testDetermineOutputGroupsIgnoresUnknownGroup() throws Exception { @Test public void testDetermineOutputGroupsRemovesPreviouslyAddedGroup() throws Exception { Set outputGroups; - outputGroups = determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("+a", "-a"), false); + outputGroups = + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("+a", "-a"), ValidationMode.OFF); assertThat(outputGroups).containsExactly("x", "y", "z"); // Order matters here. - outputGroups = determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-a", "+a"), false); + outputGroups = + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("-a", "+a"), ValidationMode.OFF); assertThat(outputGroups).containsExactly("x", "y", "z", "a"); } @Test public void testDetermineOutputGroupsContainsValidationGroup() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList(), true); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList(), ValidationMode.OUTPUT_GROUP); assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION); } @Test public void testDetermineOutputGroupsContainsValidationGroupAfterOverride() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("foo"), true); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("foo"), ValidationMode.OUTPUT_GROUP); assertThat(outputGroups).containsExactly("foo", OutputGroupInfo.VALIDATION); } @Test public void testDetermineOutputGroupsContainsValidationGroupAfterAdd() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("+a"), true); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("+a"), ValidationMode.OUTPUT_GROUP); assertThat(outputGroups).containsExactly("x", "y", "z", "a", OutputGroupInfo.VALIDATION); } @Test public void testDetermineOutputGroupsContainsValidationGroupAfterRemove() throws Exception { Set outputGroups = - determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-x"), true); + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), asList("-x"), ValidationMode.OUTPUT_GROUP); assertThat(outputGroups).containsExactly("y", "z", OutputGroupInfo.VALIDATION); } + + @Test + public void testDetermineOutputGroupsContainsValidationGroupDespiteRemove() throws Exception { + Set outputGroups = + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), + asList("-" + OutputGroupInfo.VALIDATION), + ValidationMode.OUTPUT_GROUP); + assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION); + } + + @Test + public void testDetermineOutputGroupsContainsTopLevelValidationGroup() throws Exception { + Set outputGroups = + determineOutputGroups( + ImmutableSet.of("x", "y", "z"), + asList("-" + OutputGroupInfo.VALIDATION_TOP_LEVEL), + ValidationMode.ASPECT); + assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION_TOP_LEVEL); + } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index e3a8fed4440f76..a6599b69606a2a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -1878,7 +1878,8 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) /*runTestsExclusively=*/ false, false, false, - OutputGroupInfo.determineOutputGroups(ImmutableList.of(), true)), + OutputGroupInfo.determineOutputGroups( + ImmutableList.of(), OutputGroupInfo.ValidationMode.OUTPUT_GROUP)), /* trustRemoteArtifacts= */ false)); // The catastrophic exception should be propagated into the BuildFailedException whether or not // --keep_going is set. diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index f5168b661ac051..53abaf6ceefdaf 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -293,6 +293,12 @@ def _my_rule_impl(ctx): ) group_kwargs[name + "_outputs"] = depset( [outfile], transitive=[group_kwargs[name + "_outputs"]]) + valid = ctx.actions.declare_file(ctx.label.name + "-valid") + ctx.actions.run_shell( + outputs = [valid], + command = "printf valid > %s && exit %d" % (valid.path, 0), + ) + group_kwargs["_validation"] = depset([valid]) return [OutputGroupInfo(**group_kwargs)] my_rule = rule(implementation = _my_rule_impl, attrs = { @@ -666,6 +672,7 @@ function test_bep_output_groups() { # 2. bar_outputs (6/0) # 3. baz_outputs (0/1) # 4. skip_outputs (1/0) + # 5. _validation implicit with --experimental_run_validations (1/0) # # We request the first three output groups and expect foo_outputs and # bar_outputs to appear in BEP, because both groups have at least one @@ -675,6 +682,7 @@ function test_bep_output_groups() { --build_event_text_file=bep_output \ --build_event_json_file="$TEST_log" \ --build_event_max_named_set_of_file_entries=1 \ + --experimental_run_validations \ --output_groups=foo_outputs,bar_outputs,baz_outputs \ && fail "expected failure" || true @@ -700,6 +708,7 @@ function test_bep_output_groups() { expect_not_log "\"name\":\"${name}_outputs\"" expect_not_log "\"name\":\"outputgroups/my_lib-${name}.out\"" done + expect_not_log "-valid\"" # validation outputs shouldn't appear in BEP } function test_aspect_artifacts() { @@ -766,6 +775,7 @@ function test_failing_aspect_bep_output_groups() { # 2. bar_outputs (6/0) # 3. baz_outputs (0/1) # 4. skip_outputs (1/0) + # 5. _validation implicit with --experimental_run_validations (1/0) # # We request the first two output groups and expect only bar_outputs to # appear in BEP, because all actions contributing to bar_outputs succeeded. @@ -778,6 +788,8 @@ function test_failing_aspect_bep_output_groups() { --build_event_text_file=bep_output \ --build_event_json_file="$TEST_log" \ --build_event_max_named_set_of_file_entries=1 \ + --experimental_run_validations \ + --experimental_use_validation_aspect \ --aspects=semifailingaspect.bzl%semifailing_aspect \ --output_groups=foo_outputs,bar_outputs,good-aspect-out,bad-aspect-out,mixed-aspect-out \ && fail "expected failure" || true @@ -804,6 +816,10 @@ function test_failing_aspect_bep_output_groups() { expect_log "\"name\":\"semifailingpkg/out2.txt.aspect.good\",\"uri\":" expect_log "\"name\":\"semifailingpkg/out1.txt.aspect.mixed\",\"uri\":" expect_not_log "\"name\":\"semifailingpkg/out2.txt.aspect.mixed\",\"uri\":" + + # Validation outputs shouldn't appear in BEP (incl. no output group) + expect_not_log "-valid\"" + expect_log_n '^{"id":{"targetCompleted":{.*"aspect":"ValidateTarget".*}},"completed":{"success":true}}$' 2 } function test_build_only() { diff --git a/src/test/shell/integration/validation_actions_test.sh b/src/test/shell/integration/validation_actions_test.sh index 500fd1abbc2362..63d9251ddcc131 100755 --- a/src/test/shell/integration/validation_actions_test.sh +++ b/src/test/shell/integration/validation_actions_test.sh @@ -205,6 +205,16 @@ EOF chmod +x validation_actions/validation_tool } +function setup_slow_failing_validation_action() { + cat > validation_actions/validation_tool <<'EOF' +#!/bin/bash +sleep 10 +echo "validation failed!" +exit 1 +EOF + chmod +x validation_actions/validation_tool +} + function assert_exists() { path="$1" [ -f "$path" ] && return 0 @@ -219,8 +229,54 @@ function test_validation_actions() { setup_test_project setup_passing_validation_action - bazel build --experimental_run_validations //validation_actions:foo0 || fail "Expected build to succeed" + bazel build --experimental_run_validations \ + //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed" + + expect_log "Target //validation_actions:foo0 up-to-date:" + expect_log "validation_actions/foo0.main" + assert_exists bazel-bin/validation_actions/foo0.validation +} + +function test_validation_actions_with_validation_aspect() { + setup_test_project + setup_passing_validation_action + + bazel build --experimental_run_validations --experimental_use_validation_aspect \ + //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed" + + # Console printout as if no aspects were running + expect_log "Target //validation_actions:foo0 up-to-date:" + expect_log "validation_actions/foo0.main" + expect_not_log "ValidateTarget" + assert_exists bazel-bin/validation_actions/foo0.validation +} + +function test_validation_actions_with_validation_and_another_aspect() { + setup_test_project + setup_passing_validation_action + + cat > validation_actions/simpleaspect.bzl <<'EOF' +def _simple_aspect_impl(target, ctx): + aspect_out = ctx.actions.declare_file(ctx.label.name + ".aspect") + ctx.actions.write( + output=aspect_out, + content = "Hello from aspect") + return struct(output_groups={ + "aspect-out" : depset([aspect_out]) }) + +simple_aspect = aspect(implementation=_simple_aspect_impl) +EOF + + bazel build --experimental_run_validations \ + --experimental_use_validation_aspect \ + --aspects=validation_actions/simpleaspect.bzl%simple_aspect \ + --output_groups=+aspect-out \ + //validation_actions:foo0 >& "$TEST_log" || fail "Expected build to succeed" + # Console printout includes other aspect but not validation aspect + expect_log "Aspect //validation_actions:simpleaspect.bzl%simple_aspect of //validation_actions:foo0 up-to-date:" + expect_log "validation_actions/foo0.aspect" + expect_not_log "ValidateTarget" assert_exists bazel-bin/validation_actions/foo0.validation } @@ -229,7 +285,8 @@ function test_validation_actions_implicit_output() { setup_passing_validation_action # Requesting an implicit output - bazel build --experimental_run_validations //validation_actions:foo0.implicit || fail "Expected build to succeed" + bazel build --experimental_run_validations \ + //validation_actions:foo0.implicit >& "$TEST_log" || fail "Expected build to succeed" assert_exists bazel-bin/validation_actions/foo0.validation } @@ -238,7 +295,8 @@ function test_validation_actions_through_deps() { setup_test_project setup_passing_validation_action - bazel build --experimental_run_validations //validation_actions:gen || fail "Expected build to succeed" + bazel build --experimental_run_validations \ + //validation_actions:gen >& "$TEST_log" || fail "Expected build to succeed" assert_exists bazel-bin/validation_actions/foo0.validation assert_exists bazel-bin/validation_actions/foo1.validation @@ -250,8 +308,41 @@ function test_failing_validation_action_fails_build() { setup_test_project setup_failing_validation_action - bazel build --experimental_run_validations //validation_actions:gen >& "$TEST_log" && fail "Expected build to fail" + bazel build --experimental_run_validations \ + //validation_actions:gen >& "$TEST_log" && fail "Expected build to fail" + expect_log "validation failed!" + expect_log "Target //validation_actions:gen failed to build" +} + +function test_failing_validation_action_fails_build_manual_validation_aspect() { + setup_test_project + setup_failing_validation_action + + bazel build --experimental_run_validations --aspects=ValidateTarget \ + //validation_actions:gen >& "$TEST_log" && fail "Expected build to fail" + expect_log "validation failed!" +} + +function test_failing_validation_action_fails_build_implicit_output() { + setup_test_project + setup_failing_validation_action + + bazel clean + bazel build --experimental_run_validations --experimental_use_validation_aspect \ + //validation_actions:foo0.implicit >& "$TEST_log" && fail "Expected build to fail" + expect_log "validation failed!" + expect_log "Target //validation_actions:foo0.implicit failed to build" +} + +function test_failing_validation_action_fails_build_genrule_output() { + setup_test_project + setup_failing_validation_action + + bazel build --experimental_run_validations \ + --experimental_use_validation_aspect --aspects=ValidateTarget \ + //validation_actions:generated >& "$TEST_log" && fail "Expected build to fail" expect_log "validation failed!" + expect_log "Target //validation_actions:generated failed to build" } function test_failing_validation_action_with_validations_off_does_not_fail_build() { @@ -294,6 +385,21 @@ function test_failing_validation_action_for_dep_from_test_fails_build() { expect_log "validation failed!" } +function test_slow_failing_validation_action_for_dep_from_test_fails_build() { + setup_test_project + setup_slow_failing_validation_action + + # Validation actions in the deps of the test should fail the build when run + # with "bazel test", even though validation finishes after test + bazel clean # Clean out any previous test or validation outputs + bazel test --experimental_run_validations --experimental_use_validation_aspect \ + //validation_actions:test_with_rule_with_validation_in_deps >& "$TEST_log" && fail "Expected build to fail" + expect_log "validation failed!" + expect_log "Target //validation_actions:test_with_rule_with_validation_in_deps failed to build" + # Potential race: hopefully validation is so slow that test always completes + expect_log "out of 1 test: 1 test passes." +} + function test_validation_actions_do_not_propagate_through_genquery() { setup_test_project setup_failing_validation_action