Stop throwing an exception if a Package was successfully created but contains errors. Instead, require callers to process the package and throw if they need to.
This allows us to avoid embedding a Package in an exception, which is icky. This also allows us to remove Package#containsTemporaryErrors.
Most callers' changes are fairly straightforward. The exception is EnvironmentBackedRecursivePackageProvider, which cannot throw an exception of its own in case of a package with errors (because it doesn't do that in keep_going mode), but whose request for a package with errors *should* shut down the build in case of nokeep_going mode. To do this in Skyframe, we have a new PackageErrorFunction which is to be called only in this situation, and will unconditionally throw. EnvironmentBackedRecursivePackageProvider can then catch this exception and continue on as usual, except that the exception will shut down the thread pool in a nokeep_going build.
--
MOS_MIGRATED_REVID=103247761
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
index 1fe07a9..ebecdba 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java
@@ -261,7 +261,14 @@
if (packageValue == null) {
return null;
}
- return (ExternalPackage) packageValue.getPackage();
+ ExternalPackage externalPackage = (ExternalPackage) packageValue.getPackage();
+ if (externalPackage.containsErrors()) {
+ throw new RepositoryFunctionException(
+ new BuildFileContainsErrorsException(
+ ExternalPackage.PACKAGE_IDENTIFIER, "Could not load //external package"),
+ Transience.PERSISTENT);
+ }
+ return externalPackage;
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java b/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
index a6e4c87..8b1ed43 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildFileContainsErrorsException.java
@@ -16,15 +16,16 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import javax.annotation.Nullable;
-
/**
* Exception indicating a failed attempt to access a package that could not
* be read or had syntax errors.
*/
public class BuildFileContainsErrorsException extends NoSuchPackageException {
-
- private Package pkg;
+ public BuildFileContainsErrorsException(PackageIdentifier packageIdentifier) {
+ super(
+ packageIdentifier,
+ "Package '" + packageIdentifier.getPackageFragment().getPathString() + "' contains errors");
+ }
public BuildFileContainsErrorsException(PackageIdentifier packageIdentifier, String message) {
super(packageIdentifier, "error loading package", message);
@@ -34,15 +35,4 @@
Throwable cause) {
super(packageIdentifier, "error loading package", message, cause);
}
-
- public BuildFileContainsErrorsException(Package pkg, String msg) {
- this(pkg.getPackageIdentifier(), msg);
- this.pkg = pkg;
- }
-
- @Override
- @Nullable
- public Package getPackage() {
- return pkg;
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java b/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
index 9c2316a..00824dd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NoSuchPackageException.java
@@ -16,8 +16,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import javax.annotation.Nullable;
-
/**
* Exception indicating an attempt to access a package which is not found, does
* not exist, or can't be parsed into a package.
@@ -50,12 +48,4 @@
public PackageIdentifier getPackageId() {
return packageId;
}
-
- /**
- * Return the package if parsing completed enough to construct it. May return null.
- */
- @Nullable
- public Package getPackage() {
- return null;
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 0fbc726..c78b6d0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -169,12 +169,6 @@
private boolean containsErrors;
/**
- * True iff this package contains errors that were caused by temporary conditions (e.g. an I/O
- * error). If this is true, {@link #containsErrors} is also true.
- */
- private boolean containsTemporaryErrors;
-
- /**
* The set of labels subincluded by this package.
*/
private Set<Label> subincludes;
@@ -354,7 +348,6 @@
}
this.buildFile = builder.buildFile;
this.containsErrors = builder.containsErrors;
- this.containsTemporaryErrors = builder.containsTemporaryErrors;
this.subincludes = builder.subincludes.keySet();
this.skylarkFileDependencies = builder.skylarkFileDependencies;
this.defaultLicense = builder.defaultLicense;
@@ -473,14 +466,6 @@
return containsErrors;
}
- /**
- * True iff this package contains errors that were caused by temporary conditions (e.g. an I/O
- * error). If this is true, {@link #containsErrors()} also returns true.
- */
- public boolean containsTemporaryErrors() {
- return containsTemporaryErrors;
- }
-
public List<Event> getEvents() {
return events;
}
@@ -782,7 +767,6 @@
private List<String> features = new ArrayList<>();
private List<Event> events = Lists.newArrayList();
private boolean containsErrors = false;
- private boolean containsTemporaryErrors = false;
private License defaultLicense = License.NO_LICENSE;
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
@@ -856,6 +840,10 @@
return filename;
}
+ public List<Event> getEvents() {
+ return events;
+ }
+
/**
* Sets this package's Make environment.
*/
@@ -955,12 +943,6 @@
return containsErrors;
}
- Builder setContainsTemporaryErrors() {
- setContainsErrors();
- containsTemporaryErrors = true;
- return this;
- }
-
public Builder addEvents(Iterable<Event> events) {
for (Event event : events) {
addEvent(event);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
index 9dbb7ef..867a83a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
@@ -387,9 +387,6 @@
if (packagePb.hasContainsErrors() && packagePb.getContainsErrors()) {
builder.setContainsErrors();
}
- if (packagePb.hasContainsTemporaryErrors() && packagePb.getContainsTemporaryErrors()) {
- builder.setContainsTemporaryErrors();
- }
deserializeTargets(in, context);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 82361f7..2b09e7b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -538,7 +538,7 @@
} catch (IOException expected) {
context.eventHandler.handle(Event.error(ast.getLocation(),
"error globbing [" + Joiner.on(", ").join(includes) + "]: " + expected.getMessage()));
- context.pkgBuilder.setContainsTemporaryErrors();
+ context.pkgBuilder.setContainsErrors();
return GlobList.captureResults(includes, excludes, ImmutableList.<String>of());
} catch (GlobCache.BadGlobException e) {
throw new EvalException(ast.getLocation(), e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
index 73cafd2..9a336c1 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
@@ -166,7 +166,6 @@
}
builder.setContainsErrors(pkg.containsErrors());
- builder.setContainsTemporaryErrors(pkg.containsTemporaryErrors());
builder.build().writeDelimitedTo(out);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
index f3c792f..ec206a7 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
@@ -114,8 +114,8 @@
public static Result noPreprocessing(ParserInputSource buildFileSource) {
return new Result(
buildFileSource,
- /*preprocessed=*/false,
- /*containsErrors=*/false,
+ /*preprocessed=*/ false,
+ /*containsErrors=*/ false,
ImmutableList.<Event>of());
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 683c06d..4826bab 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -35,6 +35,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
@@ -373,6 +374,10 @@
try {
PackageValue packageValue = (PackageValue) graph.getValue(packageKey);
if (packageValue != null) {
+ Package pkg = packageValue.getPackage();
+ if (pkg.containsErrors()) {
+ throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+ }
return packageValue.getPackage().getTarget(label.getName());
} else {
throw (NoSuchThingException) Preconditions.checkNotNull(
@@ -601,7 +606,10 @@
Map<SkyKey, SkyValue> packageValues = graph.getSuccessfulValues(resultKeys);
ImmutableSet.Builder<Target> result = ImmutableSet.builder();
for (SkyValue value : packageValues.values()) {
- result.add(((PackageValue) value).getPackage().getBuildFile());
+ Package pkg = ((PackageValue) value).getPackage();
+ if (!pkg.containsErrors()) {
+ result.add(pkg.getBuildFile());
+ }
}
return result.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index dfe8715..c3e9589 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -228,7 +228,8 @@
ImmutableMap.Builder<PackageIdentifier, Package> packageMapBuilder = ImmutableMap.builder();
for (PackageIdentifier pkgId : packageNames.build()) {
PackageValue pkg = (PackageValue) env.getValue(PackageValue.key(pkgId));
- Preconditions.checkState(pkg != null, "package %s not preloaded", pkgId);
+ Preconditions.checkNotNull(pkg, "package %s not preloaded", pkgId);
+ Preconditions.checkState(!pkg.getPackage().containsErrors(), pkgId);
packageMapBuilder.put(pkg.getPackage().getPackageIdentifier(), pkg.getPackage());
}
return Pair.of(packageMapBuilder.build(), validTargets.build().toSet());
@@ -384,7 +385,7 @@
for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> entry :
env.getValuesOrThrow(packageKeys, NoSuchPackageException.class).entrySet()) {
PackageIdentifier pkgName = (PackageIdentifier) entry.getKey().argument();
- Package pkg = null;
+ Package pkg;
try {
PackageValue packageValue = (PackageValue) entry.getValue().get();
if (packageValue == null) {
@@ -393,11 +394,7 @@
}
pkg = packageValue.getPackage();
} catch (NoSuchPackageException nspe) {
- if (nspe.getPackage() != null) {
- pkg = nspe.getPackage();
- } else {
- continue;
- }
+ continue;
}
Preconditions.checkNotNull(pkg, pkgName);
packages.put(pkgName, pkg);
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 ede34cf..d4f071e 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
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.AspectFactory;
import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Rule;
@@ -74,9 +75,15 @@
return null;
}
+ Package pkg = packageValue.getPackage();
+ if (pkg.containsErrors()) {
+ throw new AspectFunctionException(
+ skyKey, new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier()));
+ }
+
Target target;
try {
- target = packageValue.getPackage().getTarget(key.getLabel().getName());
+ target = pkg.getTarget(key.getLabel().getName());
} catch (NoSuchTargetException e) {
throw new AspectFunctionException(skyKey, e);
}
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 ebc1fa7..43f3521 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
@@ -49,6 +49,7 @@
import com.google.devtools.build.lib.packages.AspectFactory;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -67,6 +68,7 @@
import com.google.devtools.build.lib.syntax.Label;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
@@ -150,6 +152,12 @@
return null;
}
+ Package pkg = packageValue.getPackage();
+ if (pkg.containsErrors()) {
+ throw new ConfiguredTargetFunctionException(
+ new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier()),
+ Transience.PERSISTENT);
+ }
Target target;
try {
target = packageValue.getPackage().getTarget(lc.getLabel().getName());
@@ -791,6 +799,11 @@
super(e, Transience.PERSISTENT);
}
+ public ConfiguredTargetFunctionException(
+ BuildFileContainsErrorsException e, Transience transience) {
+ super(e, transience);
+ }
+
private ConfiguredTargetFunctionException(ConfiguredValueCreationException error) {
super(error, Transience.PERSISTENT);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 63e8e1d..4d2413a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -13,11 +13,13 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
@@ -46,21 +48,26 @@
public Package getPackage(EventHandler eventHandler, PackageIdentifier packageName)
throws NoSuchPackageException, MissingDepException {
SkyKey pkgKey = PackageValue.key(packageName);
- Package pkg;
- try {
- PackageValue pkgValue =
- (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
- if (pkgValue == null) {
+ PackageValue pkgValue =
+ (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
+ if (pkgValue == null) {
+ throw new MissingDepException();
+ }
+ Package pkg = pkgValue.getPackage();
+ if (pkg.containsErrors()) {
+ // If this is a nokeep_going build, we must shut the build down by throwing an exception. To
+ // do that, we request a node that will throw an exception, and then try to catch it and
+ // continue. This gives the framework notification to shut down the build if it should.
+ try {
+ env.getValueOrThrow(
+ PackageErrorFunction.key(packageName), BuildFileContainsErrorsException.class);
+ Preconditions.checkState(env.valuesMissing(), "Should have thrown for %s", packageName);
throw new MissingDepException();
- }
- pkg = pkgValue.getPackage();
- } catch (NoSuchPackageException e) {
- pkg = e.getPackage();
- if (pkg == null) {
- throw e;
+ } catch (BuildFileContainsErrorsException e) {
+ // Expected.
}
}
- return pkg;
+ return pkgValue.getPackage();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index ab25cd8..6dd38da 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -131,8 +131,11 @@
throw new FileOutsidePackageRootsException(rootedPath);
}
} else if (getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE) {
- Preconditions.checkNotNull(
- env.getValue(PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER)));
+ PackageValue pkgValue =
+ (PackageValue)
+ Preconditions.checkNotNull(
+ env.getValue(PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER)));
+ Preconditions.checkState(!pkgValue.getPackage().containsErrors());
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index f657f89..81cf6ec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -62,13 +62,8 @@
if (graph.exists(pkgKey)) {
pkgValue = (PackageValue) graph.getValue(pkgKey);
if (pkgValue == null) {
- NoSuchPackageException noSuchPackageException =
- (NoSuchPackageException) Preconditions.checkNotNull(graph.getException(pkgKey), pkgKey);
- Package pkg = noSuchPackageException.getPackage();
- if (pkg == null) {
- throw noSuchPackageException;
- }
- return pkg;
+ throw (NoSuchPackageException)
+ Preconditions.checkNotNull(graph.getException(pkgKey), pkgKey);
}
} else {
// If the package key does not exist in the graph, then it must not correspond to any package,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
new file mode 100644
index 0000000..c392c04
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
@@ -0,0 +1,76 @@
+// Copyright 2015 Google Inc. 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.skyframe;
+
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
+import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+
+import javax.annotation.Nullable;
+
+/**
+ * SkyFunction that throws a {@link BuildFileContainsErrorsException} for {@link Package} that
+ * loaded, but was in error. Must only be requested when a SkyFunction wishes to ignore the errors
+ * in a {@link Package} in keep_going mode, but to shut down the build in nokeep_going mode. Thus,
+ * this SkyFunction should only be requested when the corresponding {@link PackageFunction} has
+ * already been successfully called and the resulting Package contains an error.
+ *
+ * <p>This SkyFunction never returns a value, only throws a {@link BuildFileNotFoundException}, and
+ * should never return null, since all of its dependencies should already be present.
+ */
+public class PackageErrorFunction implements SkyFunction {
+ public static SkyKey key(PackageIdentifier packageIdentifier) {
+ return new SkyKey(SkyFunctions.PACKAGE_ERROR, packageIdentifier);
+ }
+
+ @Nullable
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env) throws PackageErrorFunctionException {
+ PackageIdentifier packageIdentifier = (PackageIdentifier) skyKey.argument();
+ try {
+ SkyKey packageKey = PackageValue.key(packageIdentifier);
+ // Callers must have tried to load the package already and gotten the package successfully.
+ Package pkg =
+ ((PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class))
+ .getPackage();
+ Preconditions.checkState(pkg.containsErrors(), skyKey);
+ throw new PackageErrorFunctionException(
+ new BuildFileContainsErrorsException(packageIdentifier), Transience.PERSISTENT);
+ } catch (NoSuchPackageException e) {
+ throw new IllegalStateException(
+ "Function should not have been called on package with exception", e);
+ }
+ }
+
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+
+ private static class PackageErrorFunctionException extends SkyFunctionException {
+ public PackageErrorFunctionException(
+ BuildFileContainsErrorsException cause, Transience transience) {
+ super(cause, transience);
+ }
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 1f8d057..eb580be 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -219,10 +219,13 @@
* inconsistent).
*/
private static boolean markDependenciesAndPropagateInconsistentFilesystemExceptions(
- Package pkg, Environment env, Collection<Pair<String, Boolean>> globPatterns,
- Map<Label, Path> subincludes) throws InternalInconsistentFilesystemException {
- boolean packageWasOriginallyInError = pkg.containsErrors();
- boolean packageShouldBeInError = packageWasOriginallyInError;
+ Environment env,
+ Collection<Pair<String, Boolean>> globPatterns,
+ Map<Label, Path> subincludes,
+ PackageIdentifier packageIdentifier,
+ boolean containsErrors)
+ throws InternalInconsistentFilesystemException {
+ boolean packageShouldBeInError = containsErrors;
// TODO(bazel-team): This means that many packages will have to be preprocessed twice. Ouch!
// We need a better continuation mechanism to avoid repeating work. [skyframe-loading]
@@ -232,14 +235,13 @@
// [skyframe-loading]
Set<SkyKey> subincludePackageLookupDepKeys = Sets.newHashSet();
- for (Label label : pkg.getSubincludeLabels()) {
+ for (Label label : subincludes.keySet()) {
// Declare a dependency on the package lookup for the package giving access to the label.
subincludePackageLookupDepKeys.add(PackageLookupValue.key(label.getPackageIdentifier()));
}
Pair<? extends Map<PathFragment, PackageLookupValue>, Boolean> subincludePackageLookupResult =
getPackageLookupDepsAndPropagateInconsistentFilesystemExceptions(
- pkg.getPackageIdentifier(), subincludePackageLookupDepKeys, env,
- packageWasOriginallyInError);
+ packageIdentifier, subincludePackageLookupDepKeys, env, containsErrors);
Map<PathFragment, PackageLookupValue> subincludePackageLookupDeps =
subincludePackageLookupResult.getFirst();
packageShouldBeInError |= subincludePackageLookupResult.getSecond();
@@ -254,53 +256,64 @@
if (subincludePackageLookupValue != null) {
// Declare a dependency on the actual file that was subincluded.
Path subincludeFilePath = subincludeEntry.getValue();
- if (subincludeFilePath != null) {
- if (!subincludePackageLookupValue.packageExists()) {
- // Legacy blaze puts a non-null path when only when the package does indeed exist.
- throw new InternalInconsistentFilesystemException(pkg.getPackageIdentifier(),
- String.format("Unexpected package in %s. Was it modified during the build?",
- subincludeFilePath));
- }
+ if (subincludeFilePath != null && !subincludePackageLookupValue.packageExists()) {
+ // Legacy blaze puts a non-null path when only when the package does indeed exist.
+ throw new InternalInconsistentFilesystemException(
+ packageIdentifier,
+ String.format(
+ "Unexpected package in %s. Was it modified during the build?",
+ subincludeFilePath));
+ }
+ if (subincludePackageLookupValue.packageExists()) {
// Sanity check for consistency of Skyframe and legacy blaze.
Path subincludeFilePathSkyframe =
subincludePackageLookupValue.getRoot().getRelative(label.toPathFragment());
- if (!subincludeFilePathSkyframe.equals(subincludeFilePath)) {
- throw new InternalInconsistentFilesystemException(pkg.getPackageIdentifier(),
- String.format("Inconsistent package location for %s: '%s' vs '%s'. "
- + "Was the source tree modified during the build?",
- label.getPackageFragment(), subincludeFilePathSkyframe, subincludeFilePath));
+ if (subincludeFilePath != null
+ && !subincludeFilePathSkyframe.equals(subincludeFilePath)) {
+ throw new InternalInconsistentFilesystemException(
+ packageIdentifier,
+ String.format(
+ "Inconsistent package location for %s: '%s' vs '%s'. "
+ + "Was the source tree modified during the build?",
+ label.getPackageFragment(),
+ subincludeFilePathSkyframe,
+ subincludeFilePath));
}
// The actual file may be under a different package root than the package being
// constructed.
SkyKey subincludeSkyKey =
- FileValue.key(RootedPath.toRootedPath(subincludePackageLookupValue.getRoot(),
- subincludeFilePath));
+ FileValue.key(
+ RootedPath.toRootedPath(
+ subincludePackageLookupValue.getRoot(),
+ label.getPackageFragment().getRelative(label.getName())));
subincludeFileDepKeys.add(subincludeSkyKey);
}
}
}
- packageShouldBeInError |= markFileDepsAndPropagateInconsistentFilesystemExceptions(
- pkg.getPackageIdentifier(), subincludeFileDepKeys, env, packageWasOriginallyInError);
+ packageShouldBeInError |=
+ markFileDepsAndPropagateInconsistentFilesystemExceptions(
+ packageIdentifier, subincludeFileDepKeys, env, containsErrors);
// TODO(bazel-team): In the long term, we want to actually resolve the glob patterns within
// Skyframe. For now, just logging the glob requests provides correct incrementality and
// adequate performance.
- PackageIdentifier packageId = pkg.getPackageIdentifier();
List<SkyKey> globDepKeys = Lists.newArrayList();
for (Pair<String, Boolean> globPattern : globPatterns) {
String pattern = globPattern.getFirst();
boolean excludeDirs = globPattern.getSecond();
SkyKey globSkyKey;
try {
- globSkyKey = GlobValue.key(packageId, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
+ globSkyKey =
+ GlobValue.key(packageIdentifier, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
} catch (InvalidGlobPatternException e) {
// Globs that make it to pkg.getGlobPatterns() should already be filtered for errors.
throw new IllegalStateException(e);
}
globDepKeys.add(globSkyKey);
}
- packageShouldBeInError |= markGlobDepsAndPropagateInconsistentFilesystemExceptions(
- pkg.getPackageIdentifier(), globDepKeys, env, packageWasOriginallyInError);
+ packageShouldBeInError |=
+ markGlobDepsAndPropagateInconsistentFilesystemExceptions(
+ packageIdentifier, globDepKeys, env, containsErrors);
return packageShouldBeInError;
}
@@ -330,11 +343,6 @@
Package pkg = workspace.getPackage();
Event.replayEventsOn(env.getListener(), pkg.getEvents());
- if (pkg.containsErrors()) {
- throw new PackageFunctionException(new BuildFileContainsErrorsException(
- ExternalPackage.PACKAGE_IDENTIFIER, "Package 'external' contains errors"),
- pkg.containsTemporaryErrors() ? Transience.TRANSIENT : Transience.PERSISTENT);
- }
return new PackageValue(pkg);
}
@@ -382,12 +390,17 @@
if (packageId.equals(ExternalPackage.PACKAGE_IDENTIFIER)) {
return getExternalPackage(env, packageLookupValue.getRoot());
}
- PackageValue externalPackage = (PackageValue) env.getValue(
- PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER));
+ SkyKey externalPackageKey = PackageValue.key(ExternalPackage.PACKAGE_IDENTIFIER);
+ PackageValue externalPackage = (PackageValue) env.getValue(externalPackageKey);
if (externalPackage == null) {
return null;
}
Package externalPkg = externalPackage.getPackage();
+ if (externalPkg.containsErrors()) {
+ throw new PackageFunctionException(
+ new BuildFileContainsErrorsException(ExternalPackage.PACKAGE_IDENTIFIER),
+ Transience.PERSISTENT);
+ }
PathFragment buildFileFragment = packageNameFragment.getChild("BUILD");
RootedPath buildFileRootedPath = RootedPath.toRootedPath(packageLookupValue.getRoot(),
@@ -485,13 +498,12 @@
}
Collection<Pair<String, Boolean>> globPatterns = legacyPkgBuilder.getGlobPatterns();
Map<Label, Path> subincludes = legacyPkgBuilder.getSubincludes();
- Package pkg = legacyPkgBuilder.finishBuild();
- Event.replayEventsOn(env.getListener(), pkg.getEvents());
+ Event.replayEventsOn(env.getListener(), legacyPkgBuilder.getEvents());
boolean packageShouldBeConsideredInError;
try {
packageShouldBeConsideredInError =
- markDependenciesAndPropagateInconsistentFilesystemExceptions(pkg, env,
- globPatterns, subincludes);
+ markDependenciesAndPropagateInconsistentFilesystemExceptions(
+ env, globPatterns, subincludes, packageId, legacyPkgBuilder.containsErrors());
} catch (InternalInconsistentFilesystemException e) {
packageFunctionCache.invalidate(packageId);
throw new PackageFunctionException(e,
@@ -501,14 +513,15 @@
if (env.valuesMissing()) {
return null;
}
+
+ if (packageShouldBeConsideredInError) {
+ legacyPkgBuilder.setContainsErrors();
+ }
+ Package pkg = legacyPkgBuilder.finishBuild();
+
// We know this SkyFunction will not be called again, so we can remove the cache entry.
packageFunctionCache.invalidate(packageId);
- if (packageShouldBeConsideredInError) {
- throw new PackageFunctionException(new BuildFileContainsErrorsException(pkg,
- "Package '" + packageName + "' contains errors"),
- pkg.containsTemporaryErrors() ? Transience.TRANSIENT : Transience.PERSISTENT);
- }
return new PackageValue(pkg);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
index ade85a7..0acf9d9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
@@ -17,6 +17,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyKey;
@@ -35,6 +36,12 @@
this.pkg = Preconditions.checkNotNull(pkg);
}
+ /**
+ * Returns the package. This package may contain errors, in which case the caller should throw
+ * a {@link BuildFileContainsErrorsException} if an error-free package is needed. See also
+ * {@link PackageErrorFunction} for the case where encountering a package with errors should shut
+ * down the build but the caller can handle packages with errors.
+ */
public Package getPackage() {
return pkg;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index 4f9fcab..d0c7633 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -163,15 +163,18 @@
return null;
}
pkg = pkgValue.getPackage();
+ if (pkg.containsErrors()) {
+ env
+ .getListener()
+ .handle(
+ Event.error("package contains errors: " + rootRelativePath.getPathString()));
+ }
} catch (NoSuchPackageException e) {
// The package had errors, but don't fail-fast as there might be subpackages below the
- // current directory, and there might be targets in the package that were successfully
- // loaded.
- env.getListener().handle(Event.error(
- "package contains errors: " + rootRelativePath.getPathString()));
- if (e.getPackage() != null) {
- pkg = e.getPackage();
- }
+ // current directory.
+ env
+ .getListener()
+ .handle(Event.error("package contains errors: " + rootRelativePath.getPathString()));
}
if (pkg != null) {
visitor.visitPackageValue(pkg, env);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 051215d..fa14cd9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -68,16 +68,7 @@
*/
private Package getPackage(PackageIdentifier pkgIdentifier)
throws NoSuchPackageException, InterruptedException {
- Package pkg;
- try {
- pkg = recursivePackageProvider.getPackage(eventHandler, pkgIdentifier);
- } catch (NoSuchPackageException e) {
- pkg = e.getPackage();
- if (pkg == null) {
- throw e;
- }
- }
- return pkg;
+ return recursivePackageProvider.getPackage(eventHandler, pkgIdentifier);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index aba4e49..37d625c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -40,6 +40,7 @@
SkyFunctionName.create("SKYLARK_IMPORTS_LOOKUP");
public static final SkyFunctionName GLOB = SkyFunctionName.create("GLOB");
public static final SkyFunctionName PACKAGE = SkyFunctionName.create("PACKAGE");
+ public static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.create("PACKAGE_ERROR");
public static final SkyFunctionName TARGET_MARKER = SkyFunctionName.create("TARGET_MARKER");
public static final SkyFunctionName TARGET_PATTERN = SkyFunctionName.create("TARGET_PATTERN");
public static final SkyFunctionName PREPARE_DEPS_OF_PATTERNS =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
index 1c7cbfa..6644611 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
@@ -22,7 +22,6 @@
import com.google.devtools.build.lib.syntax.Label;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.SkyValue;
import javax.annotation.Nullable;
@@ -58,11 +57,10 @@
return null;
}
SkyKey key = PackageValue.key(label.getPackageIdentifier());
- SkyValue value = env.getValue(key);
- if (value == null) {
+ PackageValue packageValue = (PackageValue) env.getValue(key);
+ if (packageValue == null || packageValue.getPackage().containsErrors()) {
return null;
}
- PackageValue packageValue = (PackageValue) value;
return packageValue.getPackage().getTarget(label.getName());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 9a4f082..252df16 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -326,6 +326,7 @@
map.put(SkyFunctions.PACKAGE, new PackageFunction(
reporter, pkgFactory, packageManager, showLoadingProgress, packageFunctionCache,
preprocessCache, numPackagesLoaded));
+ map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
map.put(SkyFunctions.TARGET_MARKER, new TargetMarkerFunction());
map.put(SkyFunctions.TRANSITIVE_TARGET, new TransitiveTargetFunction(ruleClassProvider));
map.put(SkyFunctions.TRANSITIVE_TRAVERSAL, new TransitiveTraversalFunction());
@@ -1356,20 +1357,27 @@
}
try {
- return callUninterruptibly(new Callable<Set<Package>>() {
- @Override
- public Set<Package> call() throws Exception {
- EvaluationResult<PackageValue> result = buildDriver.evaluate(
- valueNames, false, ResourceUsage.getAvailableProcessors(), errorEventListener);
- Preconditions.checkState(!result.hasError(),
- "unexpected errors: %s", result.errorMap());
- Set<Package> packages = Sets.newHashSet();
- for (PackageValue value : result.values()) {
- packages.add(value.getPackage());
- }
- return packages;
- }
- });
+ return callUninterruptibly(
+ new Callable<Set<Package>>() {
+ @Override
+ public Set<Package> call() throws Exception {
+ EvaluationResult<PackageValue> result =
+ buildDriver.evaluate(
+ valueNames,
+ false,
+ ResourceUsage.getAvailableProcessors(),
+ errorEventListener);
+ Preconditions.checkState(
+ !result.hasError(), "unexpected errors: %s", result.errorMap());
+ Set<Package> packages = Sets.newHashSet();
+ for (PackageValue value : result.values()) {
+ Package pkg = value.getPackage();
+ Preconditions.checkState(!pkg.containsErrors(), pkg.getName());
+ packages.add(pkg);
+ }
+ return packages;
+ }
+ });
} catch (Exception e) {
throw new IllegalStateException(e);
}
@@ -1475,8 +1483,8 @@
EvaluationResult<PackageValue> result =
buildDriver.evaluate(ImmutableList.of(key), /*keepGoing=*/true,
DEFAULT_THREAD_COUNT, eventHandler);
- if (result.hasError()) {
- ErrorInfo error = result.getError();
+ ErrorInfo error = result.getError(key);
+ if (error != null) {
if (!Iterables.isEmpty(error.getCycleInfo())) {
reportCycles(result.getError().getCycleInfo(), key);
// This can only happen if a package is freshly loaded outside of the target parsing
@@ -1486,7 +1494,8 @@
}
Throwable e = error.getException();
// PackageFunction should be catching, swallowing, and rethrowing all transitive
- // errors as NoSuchPackageExceptions.
+ // errors as NoSuchPackageExceptions or constructing packages with errors, since we're in
+ // keep_going mode.
Throwables.propagateIfInstanceOf(e, NoSuchPackageException.class);
throw new IllegalStateException("Unexpected Exception type from PackageValue for '"
+ pkgName + "'' with root causes: " + Iterables.toString(error.getRootCauses()), e);
@@ -1500,16 +1509,14 @@
// so this will never throw for packages that are not loaded. However, no code currently
// relies on having the exception thrown.
try {
- return callUninterruptibly(new Callable<Package>() {
- @Override
- public Package call() throws Exception {
- return getPackage(errorEventListener, pkgName);
- }
- });
+ return callUninterruptibly(
+ new Callable<Package>() {
+ @Override
+ public Package call() throws InterruptedException, NoSuchPackageException {
+ return getPackage(errorEventListener, pkgName);
+ }
+ });
} catch (NoSuchPackageException e) {
- if (e.getPackage() != null) {
- return e.getPackage();
- }
throw e;
} catch (Exception e) {
throw new IllegalStateException(e); // Should never happen.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
index 80ccb13..b7d94e7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
@@ -46,7 +46,9 @@
this.env = env;
}
- private Package getPackage(PackageIdentifier pkgIdentifier) throws NoSuchPackageException{
+ @Override
+ public Package getLoadedPackage(final PackageIdentifier pkgIdentifier)
+ throws NoSuchPackageException {
SkyKey key = PackageValue.key(pkgIdentifier);
PackageValue value = (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class);
if (value != null) {
@@ -56,19 +58,6 @@
}
@Override
- public Package getLoadedPackage(final PackageIdentifier pkgIdentifier)
- throws NoSuchPackageException {
- try {
- return getPackage(pkgIdentifier);
- } catch (NoSuchPackageException e) {
- if (e.getPackage() != null) {
- return e.getPackage();
- }
- throw e;
- }
- }
-
- @Override
public Target getLoadedTarget(Label label) throws NoSuchPackageException,
NoSuchTargetException {
Package pkg = getLoadedPackage(label.getPackageIdentifier());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index 161a218..4742ffb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -78,14 +78,7 @@
@Override
public Package getPackage(EventHandler eventHandler, PackageIdentifier packageIdentifier)
throws NoSuchPackageException, InterruptedException {
- try {
- return packageLoader.getPackage(eventHandler, packageIdentifier);
- } catch (NoSuchPackageException e) {
- if (e.getPackage() != null) {
- return e.getPackage();
- }
- throw e;
- }
+ return packageLoader.getPackage(eventHandler, packageIdentifier);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
index 75d4ad8..2318644 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerFunction.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -79,7 +80,6 @@
}
SkyKey pkgSkyKey = PackageValue.key(label.getPackageIdentifier());
- NoSuchPackageException nspe = null;
Package pkg;
try {
PackageValue value = (PackageValue)
@@ -89,14 +89,8 @@
}
pkg = value.getPackage();
} catch (NoSuchPackageException e) {
- // For consistency with pre-Skyframe Blaze, we can return a valid Target from a Package
- // containing errors.
- pkg = e.getPackage();
- if (pkg == null) {
- // Re-throw this exception with our key because root causes should be targets, not packages.
- throw new TargetMarkerFunctionException(e);
- }
- nspe = e;
+ // Re-throw this exception with our key because root causes should be targets, not packages.
+ throw new TargetMarkerFunctionException(e);
}
Target target;
@@ -106,12 +100,14 @@
throw new TargetMarkerFunctionException(e);
}
- if (nspe != null) {
+ if (pkg.containsErrors()) {
// There is a target, but its package is in error. We rethrow so that the root cause is the
// target, not the package. Note that targets are only in error when their package is
// "in error" (because a package is in error if there was an error evaluating the package, or
// if one of its targets was in error).
- throw new TargetMarkerFunctionException(new NoSuchTargetException(target, nspe));
+ throw new TargetMarkerFunctionException(
+ new NoSuchTargetException(
+ target, new BuildFileContainsErrorsException(label.getPackageIdentifier())));
}
return TargetMarkerValue.TARGET_MARKER_INSTANCE;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
index d627a0c..90fd670 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternsResultBuilder.java
@@ -18,12 +18,10 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.syntax.Label;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.util.HashSet;
@@ -106,16 +104,9 @@
private Package findPackageInGraph(PackageIdentifier pkgIdentifier,
WalkableGraph walkableGraph) {
- SkyKey key = PackageValue.key(pkgIdentifier);
- Package pkg = null;
- NoSuchPackageException nspe = (NoSuchPackageException) walkableGraph.getException(key);
- if (nspe != null) {
- pkg = nspe.getPackage();
- } else {
- pkg = ((PackageValue) walkableGraph.getValue(key)).getPackage();
- }
- Preconditions.checkNotNull(pkg, pkgIdentifier);
- return pkg;
+ return Preconditions.checkNotNull(
+ ((PackageValue) walkableGraph.getValue(PackageValue.key(pkgIdentifier))), pkgIdentifier)
+ .getPackage();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
index 5bb9c4f..130f681 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -303,9 +304,13 @@
return ValuesMissing.INSTANCE;
}
+ Package pkg = packageValue.getPackage();
+ if (pkg.containsErrors()) {
+ throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+ }
packageLoadedSuccessfully = true;
try {
- target = packageValue.getPackage().getTarget(label.getName());
+ target = pkg.getTarget(label.getName());
} catch (NoSuchTargetException unexpected) {
// Not expected since the TargetMarkerFunction would have failed earlier if the Target
// was not present.
@@ -318,18 +323,9 @@
// We know that a Target may be extracted, but we need to get it out of the Package
// (which is known to be in error).
- Package pkg;
- try {
- PackageValue packageValue = (PackageValue) env.getValueOrThrow(packageKey,
- NoSuchPackageException.class);
- if (packageValue == null) {
- return ValuesMissing.INSTANCE;
- }
- throw new IllegalStateException(
- "Expected bad package: " + label.getPackageIdentifier());
- } catch (NoSuchPackageException nsp) {
- pkg = Preconditions.checkNotNull(nsp.getPackage(), label.getPackageIdentifier());
- }
+ PackageValue packageValue =
+ (PackageValue) Preconditions.checkNotNull(env.getValue(packageKey), label);
+ Package pkg = packageValue.getPackage();
try {
target = pkg.getTarget(label.getName());
} catch (NoSuchTargetException nste) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index ab47e4f..6d6bce0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
+import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.Target;
@@ -162,11 +163,17 @@
for (Entry<Attribute, Label> entry : transitions.entries()) {
SkyKey packageKey = PackageValue.key(entry.getValue().getPackageIdentifier());
try {
- PackageValue pkgValue = (PackageValue) env.getValueOrThrow(packageKey,
- NoSuchThingException.class);
+ PackageValue pkgValue =
+ (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class);
if (pkgValue == null) {
continue;
}
+ Package pkg = pkgValue.getPackage();
+ if (pkg.containsErrors()) {
+ // Do nothing. This error was handled when we computed the corresponding
+ // TransitiveTargetValue.
+ continue;
+ }
Collection<Label> labels = AspectDefinition.visitAspectsIfRequired(target, entry.getKey(),
pkgValue.getPackage().getTarget(entry.getValue().getName())).values();
for (Label label : labels) {
