From b8ac792bc3f8ccdb0b6f70161b48c66caee313bb Mon Sep 17 00:00:00 2001 From: nharmata Date: Thu, 17 Jan 2019 16:28:12 -0800 Subject: [PATCH] Fix long-standing bug with Blaze's exit code in --keep_going mode when encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...". The bug is that we were completely ignoring package loading errors in the SkyFunction work that went into TargetPatternPhaseValue computation, and so therefore the control flow all the way from SkyframeExecutor#loadTargetPatterns to BuildView#createErrorMessage could never hope to take such a package loading error into account. Fixes #7115 RELNOTES: In --keep_going mode, Bazel now correctly returns a non-zero exit code when encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...". PiperOrigin-RevId: 229839139 --- ...ronmentBackedRecursivePackageProvider.java | 15 +++++- .../lib/skyframe/PackageErrorFunction.java | 9 ++-- .../lib/skyframe/TargetPatternFunction.java | 10 +++- .../lib/pkgcache/LoadingPhaseRunnerTest.java | 46 +++++++++++++++++++ .../shell/integration/loading_phase_test.sh | 15 ++++++ 5 files changed, 89 insertions(+), 6 deletions(-) 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 8f3f4e8a8417cc..d42057382fe0e6 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 @@ -39,6 +39,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; /** * A {@link RecursivePackageProvider} backed by an {@link Environment}. Its methods may throw {@link @@ -52,11 +53,18 @@ public final class EnvironmentBackedRecursivePackageProvider extends AbstractRecursivePackageProvider { private final Environment env; + private final AtomicBoolean encounteredPackageErrors = new AtomicBoolean(false); EnvironmentBackedRecursivePackageProvider(Environment env) { this.env = env; } + // TODO(nharmata): Audit the rest of the codebase to determine if we should be calling this method + // in more places. + boolean encounteredPackageErrors() { + return encounteredPackageErrors.get(); + } + @Override public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName) throws NoSuchPackageException, MissingDepException, InterruptedException { @@ -77,7 +85,11 @@ public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier p Preconditions.checkState(env.valuesMissing(), "Should have thrown for %s", packageName); throw new MissingDepException(); } catch (BuildFileContainsErrorsException e) { - // Expected. + // If this is a keep_going build, then the user of this RecursivePackageProvider has two + // options for handling the "package in error" case. The user must either inspect the + // package returned by this method, or else determine whether any errors have been seen via + // the "encounteredPackageErrors" method. + encounteredPackageErrors.set(true); } } return pkgValue.getPackage(); @@ -107,6 +119,7 @@ public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier pa return packageLookupValue.packageExists(); } catch (NoSuchPackageException | InconsistentFilesystemException e) { env.getListener().handle(Event.error(e.getMessage())); + encounteredPackageErrors.set(true); return false; } } 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 index acef91d2dfad14..3ede380c49588d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java @@ -33,10 +33,11 @@ /** * 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. + * loaded, but was in error. Must only be requested when a SkyFunction wishes to ignore the Skyframe + * error from a {@link PackageValue} 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. * *

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. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java index 52308fd69850b9..b195308dcb1de3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java @@ -82,7 +82,12 @@ public void process(Iterable partialResult) { // The exception type here has to match the one on the BatchCallback. Since the callback // defined above never throws, the exact type here is not really relevant. RuntimeException.class); - resolvedTargets = ResolvedTargets.builder().addAll(results).build(); + ResolvedTargets.Builder resolvedTargetsBuilder = + ResolvedTargets.builder().addAll(results); + if (provider.encounteredPackageErrors()) { + resolvedTargetsBuilder.setError(); + } + resolvedTargets = resolvedTargetsBuilder.build(); } catch (TargetParsingException e) { env.getListener().post(new ParsingFailedEvent(patternKey.getPattern(), e.getMessage())); throw new TargetPatternFunctionException(e); @@ -96,6 +101,9 @@ public void process(Iterable partialResult) { } Preconditions.checkNotNull(resolvedTargets, key); ResolvedTargets.Builder