Skip to content

Commit

Permalink
Failures early in package loading will now fail all --keep_going builds.
Browse files Browse the repository at this point in the history 
When loading all packages under a directory (//foo/...) we use RecursivePkgFunction,
which in --keep_going mode was silently ignoring any NoSuchPackageExceptions thrown
by PackageFunction. Critically, any exceptions thrown by loading a Starlark .bzl file
were being ignored during loading.

RecursivePkgFunction now transitively collects presence of errors into the
RecursivePkgValue so callers (EnvironmentBackedRecursivePackageProvider)
can observe and record that errors happened during loading.

Fixes bazelbuild#7674.

RELNOTES:
None.
PiperOrigin-RevId: 251298005
  • Loading branch information
@michaeledgar @irengrig
michaeledgar authored and irengrig committed Jun 18, 2019
1 parent 94641c6 commit 2581cab
Show file tree
Hide file tree
Showing 4 changed files with 206 additions and 14 deletions.
17 changes: 13 additions & 4 deletions ...ava/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,17 @@ boolean encounteredPackageErrors() {
public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
throws NoSuchPackageException, MissingDepException, InterruptedException {
SkyKey pkgKey = PackageValue.key(packageName);
PackageValue pkgValue =
(PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
if (pkgValue == null) {
throw new MissingDepException();
PackageValue pkgValue;
try {
pkgValue = (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
if (pkgValue == null) {
throw new MissingDepException();
}
} catch (NoSuchPackageException e) {
encounteredPackageErrors.set(true);
throw e;
}

Package pkg = pkgValue.getPackage();
if (pkg.containsErrors()) {
// If this is a nokeep_going build, we must shut the build down by throwing an exception. To
Expand Down Expand Up @@ -187,6 +193,9 @@ public Iterable<PathFragment> getPackagesUnderDirectory(
// the exception types it can accept.
throw new MissingDepException();
}
if (lookup.hasErrors()) {
encounteredPackageErrors.set(true);
}

for (String packageName : lookup.getPackages()) {
// TODO(bazel-team): Make RecursivePkgValue return NestedSet<PathFragment> so this transform
Expand Down
13 changes: 10 additions & 3 deletions src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ protected RecursivePkgValue aggregateWithSubdirectorySkyValues(
// Aggregate the transitive subpackages.
for (SkyValue childValue : subdirectorySkyValues.values()) {
consumer.addTransitivePackages(((RecursivePkgValue) childValue).getPackages());
if (((RecursivePkgValue) childValue).hasErrors()) {
consumer.addTransitiveErrors();
}
}
return consumer.createRecursivePkgValue();
}
Expand All @@ -82,6 +85,7 @@ private static class MyPackageDirectoryConsumer
implements RecursiveDirectoryTraversalFunction.PackageDirectoryConsumer {

private final NestedSetBuilder<String> packages = new NestedSetBuilder<>(Order.STABLE_ORDER);
private boolean hasErrors = false;

@Override
public void notePackage(PathFragment pkgPath) {
Expand All @@ -90,16 +94,19 @@ public void notePackage(PathFragment pkgPath) {

@Override
public void notePackageError(String noSuchPackageExceptionErrorMessage) {
// Nothing to do because the RecursiveDirectoryTraversalFunction has already emitted an error
// event.
hasErrors = true;
}

void addTransitivePackages(NestedSet<String> transitivePackages) {
packages.addTransitive(transitivePackages);
}

void addTransitiveErrors() {
hasErrors = true;
}

RecursivePkgValue createRecursivePkgValue() {
return RecursivePkgValue.create(packages);
return RecursivePkgValue.create(packages, hasErrors);
}
}

Expand Down
16 changes: 11 additions & 5 deletions src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,21 @@
public class RecursivePkgValue implements SkyValue {
@AutoCodec
static final RecursivePkgValue EMPTY =
new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER));
new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER), false);

private final NestedSet<String> packages;
private final boolean hasErrors;

private RecursivePkgValue(NestedSet<String> packages) {
private RecursivePkgValue(NestedSet<String> packages, boolean hasErrors) {
this.packages = packages;
this.hasErrors = hasErrors;
}

static RecursivePkgValue create(NestedSetBuilder<String> packages) {
if (packages.isEmpty()) {
static RecursivePkgValue create(NestedSetBuilder<String> packages, boolean hasErrors) {
if (packages.isEmpty() && !hasErrors) {
return EMPTY;
}
return new RecursivePkgValue(packages.build());
return new RecursivePkgValue(packages.build(), hasErrors);
}

/** Create a transitive package lookup request. */
Expand All @@ -65,6 +67,10 @@ public NestedSet<String> getPackages() {
return packages;
}

public boolean hasErrors() {
return hasErrors;
}

@AutoCodec.VisibleForSerialization
@AutoCodec
static class Key extends RecursivePkgSkyKey {
Expand Down
174 changes: 172 additions & 2 deletions src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
import com.google.common.collect.Maps;
import com.google.common.collect.MoreCollectors;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
Expand Down Expand Up @@ -58,7 +59,6 @@
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.Path;
Expand All @@ -70,8 +70,10 @@
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -1026,8 +1028,147 @@ public void testPackageLoadingError_NoKeepGoing_TargetsBeneathDirectory() throws
runTestPackageLoadingError(/*keepGoing=*/ false, "//bad/...");
}

@Test
public void testPackageLoadingError_KeepGoing_SomeGoodTargetsBeneathDirectory() throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestPackageLoadingError(/*keepGoing=*/ true, "//...");
}

@Test
public void testPackageLoadingError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestPackageLoadingError(/*keepGoing=*/ false, "//...");
}

private void runTestPackageFileInconsistencyError(boolean keepGoing, String... patterns)
throws Exception {
tester.addFile("bad/BUILD", "sh_library(name = 't')\n");
IOException ioExn = new IOException("nope");
tester.throwExceptionOnGetInputStream(tester.getWorkspace().getRelative("bad/BUILD"), ioExn);
if (keepGoing) {
TargetPatternPhaseValue value = tester.loadKeepGoing(patterns);
assertThat(value.hasError()).isTrue();
tester.assertContainsWarning("Target pattern parsing failed");
tester.assertContainsError("error loading package 'bad': nope");
} else {
TargetParsingException exn =
assertThrows(TargetParsingException.class, () -> tester.load(patterns));
assertThat(exn).hasCauseThat().isInstanceOf(BuildFileContainsErrorsException.class);
assertThat(exn).hasCauseThat().hasMessageThat().contains("error loading package 'bad': nope");
}
}

@Test
public void testPackageFileInconsistencyError_KeepGoing_ExplicitTarget() throws Exception {
runTestPackageFileInconsistencyError(true, "//bad:BUILD");
}

@Test
public void testPackageFileInconsistencyError_NoKeepGoing_ExplicitTarget() throws Exception {
runTestPackageFileInconsistencyError(false, "//bad:BUILD");
}

@Test
public void testPackageFileInconsistencyError_KeepGoing_TargetsInPackage() throws Exception {
runTestPackageFileInconsistencyError(true, "//bad:all");
}

@Test
public void testPackageFileInconsistencyError_NoKeepGoing_TargetsInPackage() throws Exception {
runTestPackageFileInconsistencyError(false, "//bad:all");
}

@Test
public void testPackageFileInconsistencyError_KeepGoing_argetsBeneathDirectory()
throws Exception {
runTestPackageFileInconsistencyError(true, "//bad/...");
}

@Test
public void testPackageFileInconsistencyError_NoKeepGoing_TargetsBeneathDirectory()
throws Exception {
runTestPackageFileInconsistencyError(false, "//bad/...");
}

@Test
public void testPackageFileInconsistencyError_KeepGoing_SomeGoodTargetsBeneathDirectory()
throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestPackageFileInconsistencyError(true, "//...");
}

@Test
public void testPackageFileInconsistencyError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestPackageFileInconsistencyError(false, "//...");
}

private void runTestExtensionLoadingError(boolean keepGoing, String... patterns)
throws Exception {
tester.addFile("bad/f1.bzl", "nope");
tester.addFile("bad/BUILD", "load(\":f1.bzl\", \"not_a_symbol\")");
if (keepGoing) {
TargetPatternPhaseValue value = tester.loadKeepGoing(patterns);
assertThat(value.hasError()).isTrue();
tester.assertContainsWarning("Target pattern parsing failed");
} else {
TargetParsingException exn =
assertThrows(TargetParsingException.class, () -> tester.load(patterns));
assertThat(exn).hasCauseThat().isInstanceOf(BuildFileContainsErrorsException.class);
assertThat(exn).hasCauseThat().hasMessageThat().contains("Extension 'bad/f1.bzl' has errors");
}
tester.assertContainsError("/workspace/bad/f1.bzl:1:1: name 'nope' is not defined");
}

@Test
public void testExtensionLoadingError_KeepGoing_ExplicitTarget() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad:BUILD");
}

@Test
public void testExtensionLoadingError_NoKeepGoing_ExplicitTarget() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad:BUILD");
}

@Test
public void testExtensionLoadingError_KeepGoing_TargetsInPackage() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad:all");
}

@Test
public void testExtensionLoadingError_NoKeepGoing_TargetsInPackage() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad:all");
}

@Test
public void testExtensionLoadingError_KeepGoing_TargetsBeneathDirectory() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad/...");
}

@Test
public void testExtensionLoadingError_NoKeepGoing_TargetsBeneathDirectory() throws Exception {
runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad/...");
}

@Test
public void testExtensionLoadingError_KeepGoing_SomeGoodTargetsBeneathDirectory()
throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestExtensionLoadingError(/*keepGoing=*/ true, "//...");
}

@Test
public void testExtensionLoadingError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
throws Exception {
tester.addFile("good/BUILD", "sh_library(name = 't')\n");
runTestExtensionLoadingError(/*keepGoing=*/ false, "//...");
}

private static class LoadingPhaseTester {
private final ManualClock clock = new ManualClock();
private final CustomInMemoryFs fs = new CustomInMemoryFs(clock);
private final Path workspace;

private final AnalysisMock analysisMock;
Expand All @@ -1047,7 +1188,6 @@ private static class LoadingPhaseTester {
private MockToolsConfig mockToolsConfig;

public LoadingPhaseTester() throws IOException {
FileSystem fs = new InMemoryFileSystem(clock);
this.workspace = fs.getPath("/workspace");
workspace.createDirectory();
mockToolsConfig = new MockToolsConfig(workspace);
Expand Down Expand Up @@ -1234,6 +1374,10 @@ public ImmutableSet<Label> getTestSuiteTargets() {
return loadingPhaseCompleteEvent.getFilteredLabels();
}

void throwExceptionOnGetInputStream(Path path, IOException exn) {
fs.throwExceptionOnGetInputStream(path, exn);
}

private Iterable<Event> filteredEvents() {
return Iterables.filter(storedErrors.getEvents(), new Predicate<Event>() {
@Override
Expand Down Expand Up @@ -1275,4 +1419,30 @@ public <T extends Postable> T findPostOnce(Class<T> clazz) {
.collect(MoreCollectors.onlyElement());
}
}

/**
* Custom {@link InMemoryFileSystem} that can be pre-configured per-file to throw a supplied
* IOException instead of the usual behavior.
*/
private static class CustomInMemoryFs extends InMemoryFileSystem {

private final Map<Path, IOException> pathsToErrorOnGetInputStream = Maps.newHashMap();

CustomInMemoryFs(ManualClock manualClock) {
super(manualClock);
}

synchronized void throwExceptionOnGetInputStream(Path path, IOException exn) {
pathsToErrorOnGetInputStream.put(path, exn);
}

@Override
protected synchronized InputStream getInputStream(Path path) throws IOException {
IOException exnToThrow = pathsToErrorOnGetInputStream.get(path);
if (exnToThrow != null) {
throw exnToThrow;
}
return super.getInputStream(path);
}
}
}

0 comments on commit 2581cab

Please sign in to comment.