If globbing throws an IOException, fail to construct the package instead 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
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 8f11ea7..7398b1f 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -541,6 +541,7 @@
"//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
new file mode 100644
index 0000000..7610f63
--- /dev/null
+++ 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
index 465ff44..e443321 100644
--- 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 @@
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 @@
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 @@
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
index 6a69e94..59455ec 100644
--- 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 @@
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
index bd16b00..9466ec3 100644
--- 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 @@
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 @@
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
index 27b8901..eb614fc 100644
--- 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 @@
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
index 3f72ce2..afa6a89 100644
--- 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 @@
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"));
}
