diff options
12 files changed, 166 insertions, 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 index 4ec65dd3b0..ec5806869a 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 @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.util.SpellChecker; 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 class Package { 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 class Package { 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 @@ public class Package { 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 class Package { } /** 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 class Package { 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 index cab4a4f6d6..3eead14e7d 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 @@ -573,11 +573,12 @@ public final class PackageFactory { 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 index 33fb4aa5cb..4a6010faba 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 @@ -567,7 +567,11 @@ public class PackageFunction implements SkyFunction { 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 index 2372793333..71f2c98ff6 100644 --- 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 class WorkspaceFileFunction implements SkyFunction { 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 class WorkspaceFileFunction implements SkyFunction { 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 index 8f11ea7943..7398b1f51d 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 @@ 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 new file mode 100644 index 0000000000..7610f637cb --- /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 465ff44d07..e443321abc 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 @@ public class PackageFactoryTest extends PackageFactoryTestBase { 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 class PackageFactoryTest extends PackageFactoryTestBase { 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 class PackageFactoryTest extends PackageFactoryTestBase { 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 6a69e94745..59455eca3a 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 @@ public class WorkspaceFactoryTest { 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 bd16b008ca..9466ec38c3 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.CachingPackageLocator; 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 class PackageFactoryApparatus { 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 class PackageFactoryApparatus { 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 27b89019c2..eb614fc49c 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.Location; 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 index 3f72ce207d..afa6a89cbd 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 @@ public class IncrementalLoadingTest { 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")); } diff --git a/src/tools/generate_workspace/src/main/java/com/google/devtools/build/workspace/WorkspaceResolver.java b/src/tools/generate_workspace/src/main/java/com/google/devtools/build/workspace/WorkspaceResolver.java index 97032b6e55..fdf12cd2d9 100644 --- a/src/tools/generate_workspace/src/main/java/com/google/devtools/build/workspace/WorkspaceResolver.java +++ b/src/tools/generate_workspace/src/main/java/com/google/devtools/build/workspace/WorkspaceResolver.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.AggregatingAttributeMapper; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; +import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension; import com.google.devtools.build.lib.packages.RuleClassProvider; @@ -90,7 +91,12 @@ public class WorkspaceResolver { handler.handle(Event.error(Location.fromFile(workspacePath), e.getMessage())); } - return builder.build(); + try { + return builder.build(); + } catch (NoSuchPackageException e) { + // TODO(kchodorow): delete this entire method. + throw new IllegalStateException("No globs expected in WORKSPACE files", e); + } } /** |