When an aspect is in error with --allow_analysis_failures, create t…

…he result prior to closing the `RuleContext`. Fixes a crash with required fragment tracking. PiperOrigin-RevId: 457810773 Change-Id: I4168f5a046bd28d8d624ac24263d499b9ce91176
graywolf-at-work · Jun 28, 2022 · 5d75c08 · 5d75c08
1 parent d20f25e
commit 5d75c08
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 4 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -544,12 +544,12 @@ public ConfiguredAspect createAspect(
               ruleContext,
               aspect.getParameters(),
               ruleClassProvider.getToolsRepository());
+      if (configuredAspect == null) {
+        return erroredConfiguredAspect(ruleContext);
+      }
     } finally {
       ruleContext.close();
     }
-    if (configuredAspect == null) {
-      return erroredConfiguredAspect(ruleContext);
-    }
 
     validateAdvertisedProviders(
         configuredAspect,

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java
@@ -461,4 +461,54 @@ public void invalidStarlarkFragmentsFiltered() throws Exception {
 
     assertThat(requiredFragments.getFragmentClasses()).contains(JavaConfiguration.class);
   }
+
+  @Test
+  public void aspectInErrorWithAllowAnalysisFailures() throws Exception {
+    scratch.file(
+        "a/defs.bzl",
+        "def _error_aspect_impl(target, ctx):",
+        "  fail(ctx.var['FAIL_MESSAGE'])",
+        "error_aspect = aspect(implementation = _error_aspect_impl)",
+        "",
+        "def _my_rule_impl(ctx):",
+        "  pass",
+        "my_rule = rule(",
+        "  implementation = _my_rule_impl,",
+        "  attrs = {'dep': attr.label(aspects = [error_aspect])},",
+        ")");
+    scratch.file(
+        "a/BUILD",
+        "load(':defs.bzl', 'my_rule', 'error_aspect')",
+        "my_rule(name = 'a')",
+        "my_rule(name = 'b', dep = ':a')");
+
+    useConfiguration(
+        "--allow_analysis_failures",
+        "--define=FAIL_MESSAGE=abc",
+        "--include_config_fragments_provider=direct");
+    getConfiguredTarget("//a:b");
+    RequiredConfigFragmentsProvider requiredFragments =
+        getAspect("//a:defs.bzl%error_aspect").getProvider(RequiredConfigFragmentsProvider.class);
+
+    assertThat(requiredFragments.getDefines()).containsExactly("FAIL_MESSAGE");
+  }
+
+  @Test
+  public void configuredTargetInErrorWithAllowAnalysisFailures() throws Exception {
+    scratch.file(
+        "a/defs.bzl",
+        "def _error_rule_impl(ctx):",
+        "  fail(ctx.var['FAIL_MESSAGE'])",
+        "error_rule = rule(implementation = _error_rule_impl)");
+    scratch.file("a/BUILD", "load(':defs.bzl', 'error_rule')", "error_rule(name = 'error')");
+
+    useConfiguration(
+        "--allow_analysis_failures",
+        "--define=FAIL_MESSAGE=abc",
+        "--include_config_fragments_provider=direct");
+    RequiredConfigFragmentsProvider requiredFragments =
+        getConfiguredTarget("//a:error").getProvider(RequiredConfigFragmentsProvider.class);
+
+    assertThat(requiredFragments.getDefines()).containsExactly("FAIL_MESSAGE");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1124,7 +1124,7 @@ protected Set<BuildConfigurationKey> getKnownConfigurations(String label) throws
 
   /**
    * Returns the {@link ConfiguredAspect} with the given label. For example: {@code
-   * //my:base_target%my_aspect}.
+   * //my:defs.bzl%my_aspect}.
    *
    * <p>Assumes only one configured aspect exists for this label. If this isn't true, or you need
    * finer grained selection for different configurations, you'll need to expand this method.