If globbing throws an IOException, fail to construct the package inst…

…ead of constructing the package with an error. Prior to this change, if a Package.Builder object was constructed, it was guaranteed that a Package (possibly with errors) would be created. This is no longer true: if an IOException is set on the Package.Builder object, it will throw a NoSuchPackageException during #build(). PiperOrigin-RevId: 161832111
dotordogh · Jul 14, 2017 · 28adce5 · 28adce5
1 parent 5abf4ed
commit 28adce5
Show file tree

Hide file tree

Showing 12 changed files with 166 additions and 52 deletions.
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
@@ -39,6 +39,7 @@
 import com.google.devtools.build.lib.vfs.Canonicalizer;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
 import java.io.PrintStream;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -751,6 +752,8 @@ public void onLoadingComplete(Package pkg) {
     private List<String> features = new ArrayList<>();
     private List<Event> events = Lists.newArrayList();
     private List<Postable> posts = Lists.newArrayList();
+    @Nullable String ioExceptionMessage = null;
+    @Nullable private IOException ioException = null;
     private boolean containsErrors = false;
 
     private License defaultLicense = License.NO_LICENSE;
@@ -928,6 +931,12 @@ public Builder addFeatures(Iterable<String> features) {
       return this;
     }
 
+    Builder setIOExceptionAndMessage(IOException e, String message) {
+      this.ioException = e;
+      this.ioExceptionMessage = message;
+      return setContainsErrors();
+    }
+
     /**
      * Declares that errors were encountering while loading this package.
      */
@@ -1284,11 +1293,15 @@ void addRegisteredToolchainLabels(List<Label> toolchains) {
       this.registeredToolchainLabels.addAll(toolchains);
     }
 
-    private Builder beforeBuild(boolean discoverAssumedInputFiles) throws InterruptedException {
+    private Builder beforeBuild(boolean discoverAssumedInputFiles)
+        throws InterruptedException, NoSuchPackageException {
       Preconditions.checkNotNull(pkg);
       Preconditions.checkNotNull(filename);
       Preconditions.checkNotNull(buildFileLabel);
       Preconditions.checkNotNull(makeEnv);
+      if (ioException != null) {
+        throw new NoSuchPackageException(getPackageIdentifier(), ioExceptionMessage, ioException);
+      }
       // Freeze subincludes.
       subincludes = (subincludes == null)
           ? Collections.<Label, Path>emptyMap()
@@ -1337,7 +1350,7 @@ public void acceptLabelAttribute(Label label, Attribute attribute) {
     }
 
     /** Intended for use by {@link com.google.devtools.build.lib.skyframe.PackageFunction} only. */
-    public Builder buildPartial() throws InterruptedException {
+    public Builder buildPartial() throws InterruptedException, NoSuchPackageException {
       if (alreadyBuilt) {
         return this;
       }
@@ -1385,15 +1398,16 @@ public ExternalPackageBuilder externalPackageData() {
       return externalPackageData;
     }
 
-    public Package build() throws InterruptedException {
+    public Package build() throws InterruptedException, NoSuchPackageException {
       return build(/*discoverAssumedInputFiles=*/ true);
     }
 
     /**
-     * Build the package, optionally adding any labels in the package not already associated with
-     * a target as an input file.
+     * Build the package, optionally adding any labels in the package not already associated with a
+     * target as an input file.
      */
-    public Package build(boolean discoverAssumedInputFiles) throws InterruptedException {
+    public Package build(boolean discoverAssumedInputFiles)
+        throws InterruptedException, NoSuchPackageException {
       if (alreadyBuilt) {
         return pkg;
       }

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
@@ -573,11 +573,12 @@ static SkylarkList<Object> callGlob(
     try {
       Globber.Token globToken = context.globber.runAsync(includes, excludes, excludeDirs);
       matches = context.globber.fetch(globToken);
-    } catch (IOException expected) {
-      context.eventHandler.handle(Event.error(ast.getLocation(),
-              "error globbing [" + Joiner.on(", ").join(includes) + "]: " + expected.getMessage()));
-      context.pkgBuilder.setContainsErrors();
-      matches = ImmutableList.<String>of();
+    } catch (IOException e) {
+      String errorMessage =
+          "error globbing [" + Joiner.on(", ").join(includes) + "]: " + e.getMessage();
+      context.eventHandler.handle(Event.error(ast.getLocation(), errorMessage));
+      context.pkgBuilder.setIOExceptionAndMessage(e, errorMessage);
+      matches = ImmutableList.of();
     } catch (BadGlobException e) {
       throw new EvalException(ast.getLocation(), e.getMessage());
     }

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
@@ -567,7 +567,11 @@ public SkyValue compute(SkyKey key, Environment env) throws PackageFunctionExcep
       return null;
     }
     Package.Builder pkgBuilder = packageBuilderAndGlobDeps.value;
-    pkgBuilder.buildPartial();
+    try {
+      pkgBuilder.buildPartial();
+    } catch (NoSuchPackageException e) {
+      throw new PackageFunctionException(e, Transience.TRANSIENT);
+    }
     try {
       // Since the Skyframe dependencies we request below in
       // markDependenciesAndPropagateFilesystemExceptions are requested independently of

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -72,13 +72,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         repoWorkspace, ruleClassProvider.getRunfilesPrefix());
 
     if (workspaceASTValue.getASTs().isEmpty()) {
-      return new WorkspaceFileValue(
-          builder.build(), // resulting package
-          ImmutableMap.<String, Extension>of(), // list of imports
-          ImmutableMap.<String, Object>of(), // list of symbol bindings
-          workspaceRoot, // Workspace root
-          0, // first fragment, idx = 0
-          false); // last fragment
+      try {
+        return new WorkspaceFileValue(
+            builder.build(), // resulting package
+            ImmutableMap.<String, Extension>of(), // list of imports
+            ImmutableMap.<String, Object>of(), // list of symbol bindings
+            workspaceRoot, // Workspace root
+            0, // first fragment, idx = 0
+            false); // last fragment
+      } catch (NoSuchPackageException e) {
+        throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT);
+      }
     }
     WorkspaceFactory parser;
     try (Mutability mutability = Mutability.create("workspace %s", repoWorkspace)) {
@@ -115,13 +119,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
     }
 
-    return new WorkspaceFileValue(
-        builder.build(),
-        parser.getImportMap(),
-        parser.getVariableBindings(),
-        workspaceRoot,
-        key.getIndex(),
-        key.getIndex() < workspaceASTValue.getASTs().size() - 1);
+    try {
+      return new WorkspaceFileValue(
+          builder.build(),
+          parser.getImportMap(),
+          parser.getVariableBindings(),
+          workspaceRoot,
+          key.getIndex(),
+          key.getIndex() < workspaceASTValue.getASTs().size() - 1);
+    } catch (NoSuchPackageException e) {
+      throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT);
+    }
   }
 
   @Override

diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -541,6 +541,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:collect",
         "//src/main/java/com/google/devtools/build/lib:events",
+        "//src/main/java/com/google/devtools/build/lib:inmemoryfs",
         "//src/main/java/com/google/devtools/build/lib:java-compilation",
         "//src/main/java/com/google/devtools/build/lib:java-rules",
         "//src/main/java/com/google/devtools/build/lib:packages",

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisWithIOExceptionsTest.java
@@ -0,0 +1,66 @@
+// Copyright 2017 The Bazel Authors. 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.analysis;
+
+import static org.junit.Assert.fail;
+
+import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
+import com.google.devtools.build.lib.util.BlazeClock;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.IOException;
+import java.util.function.Function;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** {@link AnalysisTestCase} with custom filesystem that can throw on stat if desired. */
+@RunWith(JUnit4.class)
+public class AnalysisWithIOExceptionsTest extends AnalysisTestCase {
+  private static final String FS_ROOT = "/fsg";
+
+  private static final Function<Path, String> NULL_FUNCTION = (path) -> null;
+
+  private Function<Path, String> crashMessage = NULL_FUNCTION;
+
+  @Override
+  protected FileSystem createFileSystem() {
+    return new InMemoryFileSystem(BlazeClock.instance(), PathFragment.create(FS_ROOT)) {
+      @Override
+      public FileStatus stat(Path path, boolean followSymlinks) throws IOException {
+        String crash = crashMessage.apply(path);
+        if (crash != null) {
+          throw new IOException(crash);
+        }
+        return super.stat(path, followSymlinks);
+      }
+    };
+  }
+
+  @Test
+  public void testGlobIOException() throws Exception {
+    scratch.file("b/BUILD", "sh_library(name = 'b', deps= ['//a:a'])");
+    scratch.file("a/BUILD", "sh_library(name = 'a', srcs = glob(['a.sh']))");
+    crashMessage = path -> path.toString().contains("a.sh") ? "bork" : null;
+    reporter.removeHandler(failFastHandler);
+    try {
+      update("//b:b");
+      fail("Expected failure");
+    } catch (ViewCreationFailedException expected) {
+    }
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -812,7 +812,10 @@ public void testBadCharacterInGlob() throws Exception {
    assertGlobFails("glob(['?'])", "glob pattern '?' contains forbidden '?' wildcard");
   }
 
-  /** Tests that a glob evaluation that encounters an I/O error produces a glob error. */
+  /**
+   * Tests that a glob evaluation that encounters an I/O error throws instead of constructing a
+   * package.
+   */
   @Test
   public void testGlobWithIOErrors() throws Exception {
     events.setFailFast(false);
@@ -824,8 +827,11 @@ public void testGlobWithIOErrors() throws Exception {
     unreadableSubdir.setReadable(false);
 
     Path file = scratch.file("/pkg/BUILD", "cc_library(name = 'c', srcs = glob(['globs/**']))");
-    packages.eval("pkg", file);
-    events.assertContainsError("error globbing [globs/**]: Directory is not readable");
+    try {
+      packages.eval("pkg", file);
+    } catch (NoSuchPackageException expected) {
+    }
+    events.assertContainsError("Directory is not readable");
   }
 
   @Test
@@ -995,11 +1001,13 @@ public void testTransientErrorsInGlobbing() throws Exception {
     Path parentDir = buildFile.getParentDirectory();
     scratch.file("/e/data.txt");
     throwOnReaddir = parentDir;
-    Package pkg = packages.createPackage("e", buildFile);
-    assertThat(pkg.containsErrors()).isTrue();
+    try {
+      packages.createPackage("e", buildFile);
+    } catch (NoSuchPackageException expected) {
+    }
     events.setFailFast(true);
     throwOnReaddir = null;
-    pkg = packages.createPackage("e", buildFile);
+    Package pkg = packages.createPackage("e", buildFile);
     assertThat(pkg.containsErrors()).isFalse();
     assertThat(pkg.getRule("e")).isNotNull();
     GlobList globList = (GlobList) pkg.getRule("e").getAttributeContainer().getAttr("data");

diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -159,7 +159,7 @@ public WorkspaceFactoryHelper(boolean allowOverride, String... args) {
       this.exception = exception;
     }
 
-    public Package getPackage() throws InterruptedException {
+    public Package getPackage() throws InterruptedException, NoSuchPackageException {
       return builder.build();
     }
 

diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
 import com.google.devtools.build.lib.packages.GlobCache;
 import com.google.devtools.build.lib.packages.MakeEnvironment;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.Builder;
 import com.google.devtools.build.lib.packages.PackageFactory;
@@ -113,11 +114,10 @@ public BuildFileAST ast(Path buildFile) throws IOException {
     return BuildFileAST.parseBuildFile(inputSource, eventHandler);
   }
 
-  /**
-   * Evaluates the {@code buildFileAST} into a {@link Package}.
-   */
-  public Pair<Package, GlobCache> evalAndReturnGlobCache(String packageName, Path buildFile,
-      BuildFileAST buildFileAST) throws InterruptedException {
+  /** Evaluates the {@code buildFileAST} into a {@link Package}. */
+  public Pair<Package, GlobCache> evalAndReturnGlobCache(
+      String packageName, Path buildFile, BuildFileAST buildFileAST)
+      throws InterruptedException, NoSuchPackageException {
     PackageIdentifier packageId = PackageIdentifier.createInMainRepo(packageName);
     GlobCache globCache =
         new GlobCache(
@@ -147,21 +147,26 @@ public Pair<Package, GlobCache> evalAndReturnGlobCache(String packageName, Path
             new MakeEnvironment.Builder(),
             ImmutableMap.<String, Extension>of(),
             ImmutableList.<Label>of());
-    Package result = resultBuilder.build();
+    Package result;
+    try {
+      result = resultBuilder.build();
+    } catch (NoSuchPackageException e) {
+      // Make sure not to lose events if we fail to construct the package.
+      Event.replayEventsOn(eventHandler, resultBuilder.getEvents());
+      throw e;
+    }
     Event.replayEventsOn(eventHandler, result.getEvents());
     return Pair.of(result, globCache);
   }
 
   public Package eval(String packageName, Path buildFile, BuildFileAST buildFileAST)
-      throws InterruptedException {
+      throws InterruptedException, NoSuchPackageException {
     return evalAndReturnGlobCache(packageName, buildFile, buildFileAST).first;
   }
 
-  /**
-   * Evaluates the {@code buildFileAST} into a {@link Package}.
-   */
+  /** Evaluates the {@code buildFileAST} into a {@link Package}. */
   public Package eval(String packageName, Path buildFile)
-      throws InterruptedException, IOException {
+      throws InterruptedException, IOException, NoSuchPackageException {
     return eval(packageName, buildFile, ast(buildFile));
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.GlobCache;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
@@ -60,7 +61,7 @@ public abstract class PackageFactoryTestBase {
   protected PackageFactoryApparatus packages = createPackageFactoryApparatus();
 
   protected com.google.devtools.build.lib.packages.Package expectEvalSuccess(String... content)
-      throws InterruptedException, IOException {
+      throws InterruptedException, IOException, NoSuchPackageException {
     Path file = scratch.file("/pkg/BUILD", content);
     Package pkg = packages.eval("pkg", file);
     assertThat(pkg.containsErrors()).isFalse();

diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
@@ -338,15 +338,15 @@ public void testTransientErrorsInGlobbing() throws Exception {
     tester.addFile("e/data.txt");
     throwOnReaddir = parentDir;
     tester.sync();
-    Target target = tester.getTarget("//e:e");
-    assertThat(((Rule) target).containsErrors()).isTrue();
-    GlobList<?> globList = (GlobList<?>) ((Rule) target).getAttributeContainer().getAttr("data");
-    assertThat(globList).isEmpty();
+    try {
+      tester.getTarget("//e:e");
+    } catch (NoSuchPackageException expected) {
+    }
     throwOnReaddir = null;
     tester.sync();
-    target = tester.getTarget("//e:e");
+    Target target = tester.getTarget("//e:e");
     assertThat(((Rule) target).containsErrors()).isFalse();
-    globList = (GlobList<?>) ((Rule) target).getAttributeContainer().getAttr("data");
+    GlobList<?> globList = (GlobList<?>) ((Rule) target).getAttributeContainer().getAttr("data");
     assertThat(globList).containsExactly(Label.parseAbsolute("//e:data.txt"));
   }