Fix a couple of bugs related to error handling for top-level aspects.
--
MOS_MIGRATED_REVID=112561390
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 aafa943..032434d 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
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.DependencyEvaluationException;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
+import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.syntax.Type.ConversionException;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -72,11 +73,11 @@
@Nullable
public static SkylarkAspect loadSkylarkAspect(
Environment env, Label extensionLabel, String skylarkValueName)
- throws ConversionException {
-
+ throws ConversionException, SkylarkImportFailedException {
SkyKey importFileKey = SkylarkImportLookupValue.key(extensionLabel, false);
SkylarkImportLookupValue skylarkImportLookupValue =
- (SkylarkImportLookupValue) env.getValue(importFileKey);
+ (SkylarkImportLookupValue) env.getValueOrThrow(
+ importFileKey, SkylarkImportFailedException.class);
if (skylarkImportLookupValue == null) {
return null;
}
@@ -107,6 +108,8 @@
skylarkAspect =
loadSkylarkAspect(
env, skylarkAspectClass.getExtensionLabel(), skylarkAspectClass.getExportedName());
+ } catch (SkylarkImportFailedException e) {
+ throw new AspectFunctionException(skyKey, e);
} catch (ConversionException e) {
throw new AspectFunctionException(skyKey, e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index be7ff9b..af89970 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -61,6 +61,7 @@
import com.google.devtools.build.lib.skyframe.BuildInfoCollectionValue.BuildInfoKeyAndConfig;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException;
import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ConflictException;
+import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.CycleInfo;
@@ -425,8 +426,10 @@
// that the only errors should be analysis errors.
Preconditions.checkState(
cause instanceof ConfiguredValueCreationException
- || cause instanceof AspectCreationException // for top-level aspects
- || cause instanceof ActionConflictException,
+ || cause instanceof ActionConflictException
+ // For top-level aspects
+ || cause instanceof AspectCreationException
+ || cause instanceof SkylarkImportFailedException,
"%s -> %s",
key,
errorInfo);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index cac4c02..a883d60 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -17,8 +17,10 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect;
+import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
import com.google.devtools.build.lib.skyframe.AspectValue.SkylarkAspectLoadingKey;
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.syntax.Type.ConversionException;
@@ -53,6 +55,7 @@
labelLookupMap =
SkylarkImportLookupFunction.labelsForAbsoluteImports(ImmutableSet.of(extensionFile), env);
} catch (SkylarkImportFailedException e) {
+ env.getListener().handle(Event.error(e.getMessage()));
throw new LoadSkylarkAspectFunctionException(e, skyKey);
}
if (labelLookupMap == null) {
@@ -63,8 +66,14 @@
try {
skylarkAspect = AspectFunction.loadSkylarkAspect(
env, labelLookupMap.get(extensionFile), skylarkValueName);
+ } catch (SkylarkImportFailedException e) {
+ env.getListener().handle(Event.error(e.getMessage()));
+ throw new LoadSkylarkAspectFunctionException(
+ new AspectCreationException(e.getMessage()), skyKey);
} catch (ConversionException e) {
- throw new LoadSkylarkAspectFunctionException(e, skyKey);
+ env.getListener().handle(Event.error(e.getMessage()));
+ throw new LoadSkylarkAspectFunctionException(
+ new AspectCreationException(e.getMessage()), skyKey);
}
if (skylarkAspect == null) {
return null;
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index 2b97b11..33ee3af 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -394,4 +394,67 @@
+ "The following files have no generating action:\n"
+ "test/missing_in_action.txt\n");
}
+
+ @Test
+ public void topLevelAspectIsNotAnAspect() throws Exception {
+ scratch.file("test/aspect.bzl", "MyAspect = 4");
+ scratch.file("test/BUILD", "java_library(name = 'xxx')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+ fail();
+ } catch (ViewCreationFailedException e) {
+ // expect to fail.
+ }
+ assertContainsEvent("MyAspect from //test:aspect.bzl is not an aspect");
+ }
+
+ @Test
+ public void topLevelAspectDoesNotExist() throws Exception {
+ scratch.file("test/aspect.bzl", "");
+ scratch.file("test/BUILD", "java_library(name = 'xxx')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+ fail();
+ } catch (ViewCreationFailedException e) {
+ // expect to fail.
+ }
+ assertContainsEvent("MyAspect from //test:aspect.bzl is not an aspect");
+ }
+
+ @Test
+ public void topLevelAspectDoesNotExist2() throws Exception {
+ scratch.file("test/BUILD", "java_library(name = 'xxx')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
+ fail();
+ } catch (ViewCreationFailedException e) {
+ // expect to fail.
+ }
+ assertContainsEvent(
+ "Extension file not found. Unable to load file '//test:aspect.bzl': "
+ + "file doesn't exist or isn't a file");
+ }
+
+ @Test
+ public void topLevelAspectDoesNotExistNoBuildFile() throws Exception {
+ scratch.file("test/BUILD", "java_library(name = 'xxx')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ update(ImmutableList.of("foo/aspect.bzl%MyAspect"), "//test:xxx");
+ fail();
+ } catch (ViewCreationFailedException e) {
+ // expect to fail.
+ }
+ assertContainsEvent(
+ "Every .bzl file must have a corresponding package, but 'foo' does not have one. "
+ + "Please create a BUILD file in the same or any parent directory. "
+ + "Note that this BUILD file does not need to do anything except exist.");
+ }
}
