diff options
29 files changed, 298 insertions, 235 deletions
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 1fe07a96d6..ebecdba6b1 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 @@ public abstract class RepositoryFunction implements SkyFunction { 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 a6e4c87de7..8b1ed43643 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 @@ package com.google.devtools.build.lib.packages; 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 @@ public class BuildFileContainsErrorsException extends NoSuchPackageException { 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 9c2316ad19..00824dd509 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 @@ package com.google.devtools.build.lib.packages; 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 abstract class NoSuchPackageException extends NoSuchThingException { 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 0fbc72681a..c78b6d022a 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 @@ public class Package implements Serializable { 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 @@ public class Package implements Serializable { } 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 @@ public class Package implements Serializable { 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 @@ public class Package implements Serializable { 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 @@ public class Package implements Serializable { return filename; } + public List<Event> getEvents() { + return events; + } + /** * Sets this package's Make environment. */ @@ -955,12 +943,6 @@ public class Package implements Serializable { 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 9dbb7efdc4..867a83aa1b 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 @@ public class PackageDeserializer { 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 82361f7bbb..2b09e7b803 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 @@ public final class PackageFactory { } 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 73cafd2a86..9a336c1d80 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 @@ public class PackageSerializer { } 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 f3c792fbb9..ec206a794d 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 interface Preprocessor { 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 683c06d69d..4826baba6c 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.collect.CompactHashSet; 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 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> { 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 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> { 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 dfe87157cf..c3e9589d69 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 @@ public class GenQuery implements RuleConfiguredTargetFactory { 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 @@ public class GenQuery implements RuleConfiguredTargetFactory { 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 @@ public class GenQuery implements RuleConfiguredTargetFactory { } 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 ede34cfdc6..d4f071e10d 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.collect.nestedset.NestedSetBuilder; 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 @@ public final class AspectFunction implements SkyFunction { 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 ebc1fa72db..43f35213b7 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.AspectDefinition; 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.EvalException; 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 63e8e1d32a..4d2413aa7b 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 final class EnvironmentBackedRecursivePackageProvider implements Recursiv 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 ab25cd8145..6dd38da11d 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 @@ public class ExternalFilesHelper { 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 f657f89d4d..81cf6eca27 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 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka 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 0000000000..c392c045d3 --- /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 1f8d05750a..eb580be9ba 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 @@ public class PackageFunction implements SkyFunction { * 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 @@ public class PackageFunction implements SkyFunction { // [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 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { 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 @@ public class PackageFunction implements SkyFunction { } 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 @@ public class PackageFunction implements SkyFunction { if (env.valuesMissing()) { return null; } - // 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); + 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); + 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 ade85a7727..0acf9d9c0f 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.common.base.Preconditions; 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 @@ public class PackageValue implements SkyValue { 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 4f9fcab1c7..d0c76333f2 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 @@ abstract class RecursiveDirectoryTraversalFunction 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 051215d39c..fa14cd9c47 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 @@ public class RecursivePackageProviderBackedTargetPatternResolver */ 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 aba4e49062..37d625c8bb 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 @@ public final class SkyFunctions { 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 1c7cbfa751..6644611493 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.packages.TargetUtils; 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 @@ public final class SkyframeDependencyResolver extends DependencyResolver { 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 9a4f082abd..252df16a3b 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { // 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 80ccb1380e..b7d94e7dd7 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 @@ class SkyframePackageLoaderWithValueEnvironment implements 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 @@ class SkyframePackageLoaderWithValueEnvironment implements } @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 161a218660..4742ffbcb8 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 @@ class SkyframePackageManager implements PackageManager { @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 75d4ad80bd..23186442d9 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 @@ public final class TargetMarkerFunction implements SkyFunction { } SkyKey pkgSkyKey = PackageValue.key(label.getPackageIdentifier()); - NoSuchPackageException nspe = null; Package pkg; try { PackageValue value = (PackageValue) @@ -89,14 +89,8 @@ public final class TargetMarkerFunction implements SkyFunction { } 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 @@ public final class TargetMarkerFunction implements SkyFunction { 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 d627a0c1f1..90fd670225 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.common.collect.Maps; 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 @@ abstract class TargetPatternsResultBuilder { 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 5bb9c4f451..130f681d88 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.Iterables; 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 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> 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 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> // 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 ab47e4f86c..6d6bce0a5a 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.Attribute; 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 @@ public class TransitiveTargetFunction 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) { |