Get rid of transient errors during preprocessing and throw IOExceptions instead. Transient errors were only detected on IOExceptions, but preprocessing doesn't actually throw IOExceptions except if it fails before it even opens the main file, so there's no sense in trying to construct a package in that case.

--
MOS_MIGRATED_REVID=103119445

3 files changed, 72 insertions, 54 deletions

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 1a7c376731..82361f7bbb 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
@@ -1002,10 +1002,17 @@ public final class PackageFactory {
       prefetchGlobs(packageId, buildFileAST, preprocessingResult.preprocessed,
           buildFile, globber, defaultVisibility, makeEnv);
       return evaluateBuildFile(
-          externalPkg, packageId, buildFileAST, buildFile, globber,
+          externalPkg,
+          packageId,
+          buildFileAST,
+          buildFile,
+          globber,
           Iterables.concat(preprocessingEvents, localReporter.getEvents()),
-          defaultVisibility, preprocessingResult.containsErrors,
-          preprocessingResult.containsTransientErrors, makeEnv, imports, skylarkFileDependencies);
+          defaultVisibility,
+          preprocessingResult.containsErrors,
+          makeEnv,
+          imports,
+          skylarkFileDependencies);
     } catch (InterruptedException e) {
       globber.onInterrupt();
       throw e;
@@ -1034,8 +1041,14 @@ public final class PackageFactory {
     }
 
     Globber globber = createLegacyGlobber(buildFile.getParentDirectory(), packageId, locator);
-    Preprocessor.Result preprocessingResult =
-        preprocess(packageId, buildFile, inputSource, globber);
+    Preprocessor.Result preprocessingResult;
+    try {
+      preprocessingResult = preprocess(packageId, inputSource, globber);
+    } catch (IOException e) {
+      eventHandler.handle(
+          Event.error(Location.fromFile(buildFile), "preprocessing failed: " + e.getMessage()));
+      throw new BuildFileContainsErrorsException(packageId, "preprocessing failed", e);
+    }
     ExternalPackage externalPkg =
         new ExternalPackage.Builder(
             buildFile.getRelative("WORKSPACE"), ruleClassProvider.getRunfilesPrefix()).build();
@@ -1060,20 +1073,13 @@ public final class PackageFactory {
 
   /** Preprocesses the given BUILD file. */
   public Preprocessor.Result preprocess(
-      PackageIdentifier packageId,
-      Path buildFile,
-      CachingPackageLocator locator) throws InterruptedException {
+      PackageIdentifier packageId, Path buildFile, CachingPackageLocator locator)
+      throws InterruptedException, IOException {
     ParserInputSource inputSource;
-    try {
-      inputSource = ParserInputSource.create(buildFile);
-    } catch (IOException e) {
-      List<Event> events = ImmutableList.of(
-          Event.error(Location.fromFile(buildFile), e.getMessage()));
-      return Preprocessor.Result.transientError(buildFile.asFragment(), events);
-    }
+    inputSource = ParserInputSource.create(buildFile);
     Globber globber = createLegacyGlobber(buildFile.getParentDirectory(), packageId, locator);
     try {
-      return preprocess(packageId, buildFile, inputSource, globber);
+      return preprocess(packageId, inputSource, globber);
     } finally {
       globber.onCompletion();
     }
@@ -1084,10 +1090,8 @@ public final class PackageFactory {
    * {@link InterruptedException}.
    */
   public Preprocessor.Result preprocess(
-      PackageIdentifier packageId,
-      Path buildFile,
-      ParserInputSource inputSource,
-      Globber globber) throws InterruptedException {
+      PackageIdentifier packageId, ParserInputSource inputSource, Globber globber)
+      throws InterruptedException, IOException {
     Preprocessor preprocessor = preprocessorFactory.getPreprocessor();
     if (preprocessor == null) {
       return Preprocessor.Result.noPreprocessing(inputSource);
@@ -1099,10 +1103,6 @@ public final class PackageFactory {
           globber,
           Environment.BUILD,
           ruleFactory.getRuleClassNames());
-    } catch (IOException e) {
-      List<Event> events = ImmutableList.of(Event.error(Location.fromFile(buildFile),
-                     "preprocessing failed: " + e.getMessage()));
-      return Preprocessor.Result.transientError(buildFile.asFragment(), events);
     } catch (InterruptedException e) {
       globber.onInterrupt();
       throw e;
@@ -1238,12 +1238,19 @@ public final class PackageFactory {
    * @see PackageFactory#PackageFactory
    */
   @VisibleForTesting // used by PackageFactoryApparatus
-  public Package.LegacyBuilder evaluateBuildFile(Package externalPkg,
-      PackageIdentifier packageId, BuildFileAST buildFileAST, Path buildFilePath, Globber globber,
-      Iterable<Event> pastEvents, RuleVisibility defaultVisibility, boolean containsError,
-      boolean containsTransientError, MakeEnvironment.Builder pkgMakeEnv,
+  public Package.LegacyBuilder evaluateBuildFile(
+      Package externalPkg,
+      PackageIdentifier packageId,
+      BuildFileAST buildFileAST,
+      Path buildFilePath,
+      Globber globber,
+      Iterable<Event> pastEvents,
+      RuleVisibility defaultVisibility,
+      boolean containsError,
+      MakeEnvironment.Builder pkgMakeEnv,
       Map<PathFragment, Extension> imports,
-      ImmutableList<Label> skylarkFileDependencies) throws InterruptedException {
+      ImmutableList<Label> skylarkFileDependencies)
+      throws InterruptedException {
     Package.LegacyBuilder pkgBuilder = new Package.LegacyBuilder(
         packageId, ruleClassProvider.getRunfilesPrefix());
     StoredEventHandler eventHandler = new StoredEventHandler();
@@ -1278,10 +1285,6 @@ public final class PackageFactory {
         pkgBuilder.setContainsErrors();
       }
 
-      if (containsTransientError) {
-        pkgBuilder.setContainsTemporaryErrors();
-      }
-
       if (!validatePackageIdentifier(packageId, buildFileAST.getLocation(), eventHandler)) {
         pkgBuilder.setContainsErrors();
       }
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 acf06f9460..f3c792fbb9 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
@@ -97,23 +97,26 @@ public interface Preprocessor {
     public final ParserInputSource result;
     public final boolean preprocessed;
     public final boolean containsErrors;
-    public final boolean containsTransientErrors;
     public final List<Event> events;
 
-    private Result(ParserInputSource result,
-        boolean preprocessed, boolean containsPersistentErrors, boolean containsTransientErrors,
+    private Result(
+        ParserInputSource result,
+        boolean preprocessed,
+        boolean containsErrors,
         List<Event> events) {
       this.result = result;
       this.preprocessed = preprocessed;
-      this.containsErrors = containsPersistentErrors || containsTransientErrors;
-      this.containsTransientErrors = containsTransientErrors;
+      this.containsErrors = containsErrors;
       this.events = ImmutableList.copyOf(events);
     }
 
     /** Convenience factory for a {@link Result} wrapping non-preprocessed BUILD file contents. */
     public static Result noPreprocessing(ParserInputSource buildFileSource) {
-      return new Result(buildFileSource, /*preprocessed=*/false, /*containsErrors=*/false,
-          /*containsTransientErrors=*/false, ImmutableList.<Event>of());
+      return new Result(
+          buildFileSource,
+          /*preprocessed=*/false,
+          /*containsErrors=*/false,
+          ImmutableList.<Event>of());
     }
 
     /**
@@ -123,32 +126,31 @@ public interface Preprocessor {
      */
     public static Result success(ParserInputSource result, boolean containsErrors,
         List<Event> events) {
-      return new Result(result, /*preprocessed=*/true, /*containsPersistentErrors=*/containsErrors,
-          /*containsTransientErrors=*/false, events);
+      return new Result(result, /*preprocessed=*/true, containsErrors, events);
     }
 
     public static Result invalidSyntax(PathFragment buildFile, List<Event> events) {
-      return new Result(ParserInputSource.create(EMPTY_CHARS, buildFile), /*preprocessed=*/true,
-          /*containsPersistentErrors=*/true, /*containsTransientErrors=*/false, events);
-    }
-
-    public static Result transientError(PathFragment buildFile, List<Event> events) {
-      return new Result(ParserInputSource.create(EMPTY_CHARS, buildFile), /*preprocessed=*/false,
-          /*containsPersistentErrors=*/false, /*containsTransientErrors=*/true, events);
+      return new Result(
+          ParserInputSource.create(EMPTY_CHARS, buildFile),
+          /*preprocessed=*/true,
+          /*containsErrors=*/true,
+          events);
     }
   }
 
   /**
    * Returns a Result resulting from applying Python preprocessing to the contents of "in". If
    * errors happen, they must be reported both as an event on eventHandler and in the function
-   * return value.
+   * return value. An IOException is only thrown when preparing for preprocessing. Once
+   * preprocessing actually begins, any I/O problems encountered will be reflected in the return
+   * value, not manifested as exceptions.
    *
    * @param in the BUILD file to be preprocessed.
    * @param packageName the BUILD file's package.
    * @param globber a globber for evaluating globs.
    * @param globals the global bindings for the Python environment.
    * @param ruleNames the set of names of all rules in the build language.
-   * @throws IOException if there was an I/O problem during preprocessing.
+   * @throws IOException if there was an I/O problem preparing for preprocessing.
    * @return a pair of the ParserInputSource and a map of subincludes seen during the evaluation
    */
   Result preprocess(
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 95fef27c7e..2de3167e86 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
@@ -800,9 +800,22 @@ public class PackageFunction implements SkyFunction {
             packageId, packageLocator);
         Preprocessor.Result preprocessingResult = preprocessCache.getIfPresent(packageId);
         if (preprocessingResult == null) {
-          preprocessingResult = replacementSource == null
-              ? packageFactory.preprocess(packageId, buildFilePath, inputSource, globber)
-              : Preprocessor.Result.noPreprocessing(replacementSource);
+          try {
+            preprocessingResult =
+                replacementSource == null
+                    ? packageFactory.preprocess(packageId, inputSource, globber)
+                    : Preprocessor.Result.noPreprocessing(replacementSource);
+          } catch (IOException e) {
+            env
+                .getListener()
+                .handle(
+                    Event.error(
+                        Location.fromFile(buildFilePath),
+                        "preprocessing failed: " + e.getMessage()));
+            throw new PackageFunctionException(
+                new BuildFileContainsErrorsException(packageId, "preprocessing failed", e),
+                Transience.TRANSIENT);
+          }
           preprocessCache.put(packageId, preprocessingResult);
         }