From bb3f3299e1c9cdab39aa70c1c773de4f630c57b5 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 25 Sep 2024 13:43:58 -0700 Subject: [PATCH] Automated rollback of commit 4607ad439fe8869c8e8951d953e2d3adb613e6d6. *** Reason for rollback *** [] *** Original change description *** Automated rollback of commit 76b42af100b0a1079a25a7ba24bffcd6bc53f6e0. *** Reason for rollback *** The internal tests were fixed. *** Original change description *** Automated rollback of commit 750f0a1c05f56403e3b4a3d08b6d7de6ce3ed7f7. *** Reason for rollback *** This CL breaks internal tests. *** Original change description *** Fix `_validation` output group merging While https://github.com/bazelbuild/bazel/commit/cd725834eba3edb53b7a7dd9867363ef1fe00e57 made it so that a rul... *** PiperOrigin-RevId: 678831804 Change-Id: I7725ff31af4c184c79162249283018d808b6b41e --- .../java/com/google/devtools/build/lib/BUILD | 1 - .../build/lib/analysis/AspectCollection.java | 8 +--- .../build/lib/analysis/OutputGroupInfo.java | 3 -- .../build/lib/buildtool/BuildRequest.java | 8 ++-- .../lib/buildtool/BuildResultPrinter.java | 3 +- .../devtools/build/lib/runtime/commands/BUILD | 1 - .../lib/runtime/commands/TestCommand.java | 3 +- .../integration/validation_actions_test.sh | 40 ++----------------- 8 files changed, 10 insertions(+), 57 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 9b62db0755c029..017fc9a94449b7 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -334,7 +334,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_options", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_phase_complete_event", - "//src/main/java/com/google/devtools/build/lib/analysis:aspect_collection", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:build_info_event", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java index 2c122c394eda3d..1c2f21f962b840 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java @@ -148,9 +148,6 @@ */ @Immutable public final class AspectCollection { - /** The name of the native aspect that collects validation outputs. */ - public static final String VALIDATION_ASPECT_NAME = "ValidateTarget"; - /** aspects that should be visible to a dependency */ private final ImmutableSet usedAspects; @@ -286,14 +283,11 @@ public static AspectCollection create(Iterable aspectPath) ImmutableList.copyOf(aspectMap.entrySet()).reverse()) { for (AspectDescriptor depAspectDescriptor : deps.keySet()) { Aspect depAspect = aspectMap.get(depAspectDescriptor); - // As any aspect can add validation outputs, the special validation aspect that collects - // their outputs has to depend on all aspects. if (depAspect .getDefinition() .getRequiredProvidersForAspects() .isSatisfiedBy(aspect.getValue().getDefinition().getAdvertisedProviders()) - || depAspect.getDefinition().requires(aspect.getValue()) - || depAspect.getAspectClass().getName().equals(VALIDATION_ASPECT_NAME)) { + || depAspect.getDefinition().requires(aspect.getValue())) { deps.get(depAspectDescriptor).add(aspect.getKey()); } } 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 9bae6a523b8666..85de02050d592f 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 @@ -209,9 +209,6 @@ public static OutputGroupInfo merge(List providers) throws Merg "Output group " + VALIDATION + " provided twice with incompatible depset orders"); } } - if (!validationOutputs.isEmpty()) { - outputGroups.put(VALIDATION, validationOutputs.build()); - } return createInternal(ImmutableMap.copyOf(outputGroups)); } 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 34965720ca1145..2a8b2930241ecf 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 @@ -22,7 +22,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.AnalysisOptions; -import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.OutputGroupInfo; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; @@ -58,6 +57,7 @@ * as --keep_going, --jobs, etc. */ public class BuildRequest implements OptionsProvider { + public static final String VALIDATION_ASPECT_NAME = "ValidateTarget"; private static final ImmutableList> MANDATORY_OPTIONS = ImmutableList.of( @@ -429,8 +429,8 @@ public TopLevelArtifactContext getTopLevelArtifactContext() { public ImmutableList getAspects() { List aspects = getBuildOptions().aspects; ImmutableList.Builder result = ImmutableList.builder().addAll(aspects); - if (!aspects.contains(AspectCollection.VALIDATION_ASPECT_NAME) && useValidationAspect()) { - result.add(AspectCollection.VALIDATION_ASPECT_NAME); + if (!aspects.contains(VALIDATION_ASPECT_NAME) && useValidationAspect()) { + result.add(VALIDATION_ASPECT_NAME); } return result.build(); } @@ -453,7 +453,7 @@ public ImmutableMap getAspectsParameters() throws ViewCreationFa } } - /** Whether {@value AspectCollection#VALIDATION_ASPECT_NAME} is in use. */ + /** Whether {@value #VALIDATION_ASPECT_NAME} is in use. */ public boolean useValidationAspect() { return validationMode() == OutputGroupInfo.ValidationMode.ASPECT; } 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 596b4236ce88da..9ec081f9f6044c 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 @@ -19,7 +19,6 @@ 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.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; @@ -390,7 +389,7 @@ private static PartitionedAspectKeys partitionAspectKeys( var aspectsToPrintBuilder = ImmutableSet.builder(); var validationAspectsBuilder = ImmutableList.builder(); for (AspectKey key : keys) { - if (Objects.equals(key.getAspectClass().getName(), AspectCollection.VALIDATION_ASPECT_NAME)) { + if (Objects.equals(key.getAspectClass().getName(), BuildRequest.VALIDATION_ASPECT_NAME)) { validationAspectsBuilder.add(key); } else { aspectsToPrintBuilder.add(key); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD index 8ed4ede4289ea1..4c7bd4074eba7f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD @@ -36,7 +36,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_options", - "//src/main/java/com/google/devtools/build/lib/analysis:aspect_collection", "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java index f02d1321763f34..17816ad875c7f8 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java @@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.buildtool.BuildRequest; @@ -244,7 +243,7 @@ private static DetailedExitCode analyzeTestResults( if (buildRequest.useValidationAspect()) { validatedTargets = buildResult.getSuccessfulAspects().stream() - .filter(key -> AspectCollection.VALIDATION_ASPECT_NAME.equals(key.getAspectName())) + .filter(key -> BuildRequest.VALIDATION_ASPECT_NAME.equals(key.getAspectName())) .map(AspectKey::getBaseConfiguredTargetKey) .collect(ImmutableSet.toImmutableSet()); } else { diff --git a/src/test/shell/integration/validation_actions_test.sh b/src/test/shell/integration/validation_actions_test.sh index a7cd089f112fb7..1f68bbabfaf246 100755 --- a/src/test/shell/integration/validation_actions_test.sh +++ b/src/test/shell/integration/validation_actions_test.sh @@ -514,7 +514,9 @@ function test_validation_actions_flags() { expect_log "Target //validation_actions:foo0 up-to-date:" } -function setup_validation_tool_aspect() { +function test_validation_actions_in_rule_and_aspect() { + setup_test_project + mkdir -p aspect cat > aspect/BUILD <<'EOF' exports_files(["aspect_validation_tool"]) @@ -546,45 +548,10 @@ EOF echo "aspect validation output" > $1 EOF chmod +x aspect/aspect_validation_tool -} - -function test_validation_actions_in_rule_and_aspect_no_use_validation_aspect() { - setup_test_project - setup_validation_tool_aspect - setup_passing_validation_action - - bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \ - --noexperimental_use_validation_aspect \ - //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 - assert_exists bazel-bin/validation_actions/foo0.aspect_validation - - cat > aspect/aspect_validation_tool <<'EOF' -#!/bin/bash -echo "aspect validation failed!" -exit 1 -EOF - - bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \ - --noexperimental_use_validation_aspect \ - //validation_actions:foo0 >& "$TEST_log" && fail "Expected build to fail" - expect_log "aspect validation failed!" -} - -function test_validation_actions_in_rule_and_aspect_use_validation_aspect() { - setup_test_project - setup_validation_tool_aspect setup_passing_validation_action bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \ - --experimental_use_validation_aspect \ //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 - assert_exists bazel-bin/validation_actions/foo0.aspect_validation cat > aspect/aspect_validation_tool <<'EOF' #!/bin/bash @@ -593,7 +560,6 @@ exit 1 EOF bazel build --run_validations --aspects=//aspect:def.bzl%validation_aspect \ - --experimental_use_validation_aspect \ //validation_actions:foo0 >& "$TEST_log" && fail "Expected build to fail" expect_log "aspect validation failed!" }