From 9e16f0ad1016a28d04c498b60d4845169c2ce497 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Mon, 25 Jan 2016 12:43:32 +0000 Subject: Make AspectFunction error handling match TopLevelAspectFunction. Also match ConfiguredTargetFunction for target loading. It isn't currently possible to trigger either of these code paths - the loading phase ensures that we never attempt to analyze targets that fail to load - the Skylark import or conversion cannot fail, because Skylark checks during .bzl execution that all referenced symbols are Skylark aspects Therefore, the only way to trigger this would be if there was a native rule requesting a non-existent or broken Skylark aspect for its dependencies, but that is currently not possible - native rules can only request native aspects. However, for interleaved loading and analysis, we need to limit the set of exception classes that can be thrown from AspectFunction - we do that here by changing the constructor of AspectFunctionException to only accept either NoSuchThingException or AspectCreationException. That in turn requires that we re-throw the Skylark import and conversion exceptions as AspectCreationException, which is exactly what TopLevelAspectFunction is already doing, and necessary for correct error handling if we do ever support Skylark aspects in native rules. Alternatively, I could change the code path to crash Bazel, but that seems strictly worse. Even if we can't test this code, it's conceptually the right way to handle these errors. I'll move part of the error handling into loadSkylarkAspect in a subsequent change. -- MOS_MIGRATED_REVID=112938284 --- .../build/lib/skyframe/AspectFunction.java | 26 +++++++++------------- .../lib/skyframe/ConfiguredTargetFunction.java | 2 +- 2 files changed, 12 insertions(+), 16 deletions(-) (limited to 'src/main/java/com/google') 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 dca5c79101..8d14af31f1 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 @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; @@ -111,9 +112,11 @@ public final class AspectFunction implements SkyFunction { loadSkylarkAspect( env, skylarkAspectClass.getExtensionLabel(), skylarkAspectClass.getExportedName()); } catch (SkylarkImportFailedException e) { - throw new AspectFunctionException(skyKey, e); + env.getListener().handle(Event.error(e.getMessage())); + throw new AspectFunctionException(new AspectCreationException(e.getMessage())); } catch (ConversionException e) { - throw new AspectFunctionException(skyKey, e); + env.getListener().handle(Event.error(e.getMessage())); + throw new AspectFunctionException(new AspectCreationException(e.getMessage())); } if (skylarkAspect == null) { return null; @@ -124,23 +127,22 @@ public final class AspectFunction implements SkyFunction { throw new IllegalStateException(); } + // Keep this in sync with the same code in ConfiguredTargetFunction. PackageValue packageValue = (PackageValue) env.getValue(PackageValue.key(key.getLabel().getPackageIdentifier())); if (packageValue == null) { return null; } - Package pkg = packageValue.getPackage(); if (pkg.containsErrors()) { throw new AspectFunctionException( - skyKey, new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier())); + new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier())); } - Target target; try { target = pkg.getTarget(key.getLabel().getName()); } catch (NoSuchTargetException e) { - throw new AspectFunctionException(skyKey, e); + throw new AspectFunctionException(e); } if (!(target instanceof Rule)) { @@ -302,18 +304,12 @@ public final class AspectFunction implements SkyFunction { * Used to indicate errors during the computation of an {@link AspectValue}. */ private static final class AspectFunctionException extends SkyFunctionException { - public AspectFunctionException(AspectCreationException e) { + public AspectFunctionException(NoSuchThingException e) { super(e, Transience.PERSISTENT); } - /** Used to rethrow a child error that we cannot handle. */ - public AspectFunctionException(SkyKey childKey, Exception transitiveError) { - super(transitiveError, childKey); - } - - /** Used to rethrow a child error that we cannot handle. */ - public AspectFunctionException(SkyKey childKey, NoSuchThingException e) { - super(e, childKey); + public AspectFunctionException(AspectCreationException e) { + super(e, Transience.PERSISTENT); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 1d6329a6a1..8b55b1c59a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -135,7 +135,6 @@ final class ConfiguredTargetFunction implements SkyFunction { if (packageValue == null) { return null; } - Package pkg = packageValue.getPackage(); if (pkg.containsErrors()) { throw new ConfiguredTargetFunctionException( @@ -147,6 +146,7 @@ final class ConfiguredTargetFunction implements SkyFunction { } catch (NoSuchTargetException e) { throw new ConfiguredTargetFunctionException(e); } + transitivePackages.add(packageValue.getPackage()); // TODO(bazel-team): This is problematic - we create the right key, but then end up with a value // that doesn't match; we can even have the same value multiple times. However, I think it's -- cgit v1.2.3