Add a FailureDetail for Packages in error due to an error constructing a Rule in the Package. File bug reports in various places when we're missing a FailureDetail.
PiperOrigin-RevId: 423098609
diff --git a/src/main/java/com/google/devtools/build/lib/events/Event.java b/src/main/java/com/google/devtools/build/lib/events/Event.java
index b941028..ce99ffee 100644
--- a/src/main/java/com/google/devtools/build/lib/events/Event.java
+++ b/src/main/java/com/google/devtools/build/lib/events/Event.java
@@ -24,7 +24,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
-import java.util.function.Function;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
@@ -404,22 +403,6 @@
}
/**
- * Converts a list of {@link SyntaxError}s to events, each with a specified property, and replays
- * them on {@code handler}.
- */
- public static <T> void replayEventsOn(
- EventHandler handler,
- List<SyntaxError> errors,
- Class<T> propertyType,
- Function<SyntaxError, T> toProperty) {
- for (SyntaxError error : errors) {
- handler.handle(
- Event.error(error.location(), error.message())
- .withProperty(propertyType, toProperty.apply(error)));
- }
- }
-
- /**
* Returns a {@link StarlarkThread.PrintHandler} that sends {@link EventKind#DEBUG} events to the
* provided {@link EventHandler}.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD
index 146d5cf..fcfa651 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD
@@ -66,6 +66,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider",
+ "//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator",
"//src/main/java/com/google/devtools/build/lib/collect",
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 25c5112..b540928 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
@@ -28,6 +28,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -868,9 +869,7 @@
*/
public static Event error(Location location, String message, Code code) {
Event error = Event.error(location, message);
- // The DetailedExitCode's message is the base event's toString because that string nicely
- // includes the location value.
- return error.withProperty(DetailedExitCode.class, createDetailedCode(error.toString(), code));
+ return error.withProperty(DetailedExitCode.class, createDetailedCode(message, code));
}
private static DetailedExitCode createDetailedCode(String errorMessage, Code code) {
@@ -1306,7 +1305,9 @@
}
/**
- * Declares that errors were encountering while loading this package.
+ * Declares that errors were encountering while loading this package. If called, {@link
+ * #addEvent} or {@link #addEvents} should already have been called with an {@link Event} of
+ * type {@link EventKind#ERROR} that includes a {@link FailureDetail}.
*/
public Builder setContainsErrors() {
containsErrors = true;
@@ -1346,6 +1347,7 @@
return failureDetailOverride;
}
+ List<Event> undetailedEvents = null;
for (Event event : this.events) {
if (event.getKind() != EventKind.ERROR) {
continue;
@@ -1354,6 +1356,16 @@
if (detailedExitCode != null && detailedExitCode.getFailureDetail() != null) {
return detailedExitCode.getFailureDetail();
}
+ if (containsErrors) {
+ if (undetailedEvents == null) {
+ undetailedEvents = new ArrayList<>();
+ }
+ undetailedEvents.add(event);
+ }
+ }
+ if (undetailedEvents != null) {
+ BugReport.sendBugReport(
+ new IllegalStateException("Package has undetailed error from " + undetailedEvents));
}
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
index 8c7ff9b..f863a65 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
@@ -34,7 +34,7 @@
* can be asked if a specific package is included in it.
*/
public class PackageGroup implements Target {
- private boolean containsErrors;
+ private final boolean containsErrors;
private final Label label;
private final Location location;
private final Package containingPackage;
@@ -55,13 +55,14 @@
// TODO(bazel-team): Consider refactoring so constructor takes a PackageGroupContents.
ImmutableList.Builder<PackageSpecification> packagesBuilder = ImmutableList.builder();
+ boolean errorsFound = false;
for (String packageSpecification : packageSpecifications) {
PackageSpecification specification = null;
try {
specification =
PackageSpecification.fromString(label.getRepository(), packageSpecification);
} catch (PackageSpecification.InvalidPackageSpecificationException e) {
- containsErrors = true;
+ errorsFound = true;
eventHandler.handle(
Package.error(location, e.getMessage(), Code.INVALID_PACKAGE_SPECIFICATION));
}
@@ -70,6 +71,7 @@
packagesBuilder.add(specification);
}
}
+ this.containsErrors = errorsFound;
this.packageSpecifications = PackageGroupContents.create(packagesBuilder.build());
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 59598f2..ba42c48 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.packages.License.DistributionType;
import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode;
+import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
@@ -813,7 +814,7 @@
}
void reportError(String message, EventHandler eventHandler) {
- eventHandler.handle(Event.error(location, message));
+ eventHandler.handle(Package.error(location, message, PackageLoading.Code.STARLARK_EVAL_ERROR));
this.containsErrors = true;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index 963956d..623177a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
+import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -172,6 +173,13 @@
} catch (SyntaxError.Exception ex) {
// compilation failed
Event.replayEventsOn(localReporter, ex.errors());
+ builder.setFailureDetailOverride(
+ FailureDetails.FailureDetail.newBuilder()
+ .setMessage(ex.getMessage())
+ .setPackageLoading(
+ FailureDetails.PackageLoading.newBuilder()
+ .setCode(PackageLoading.Code.SYNTAX_ERROR))
+ .build());
}
// cleanup (success or failure)
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
index 83e7efa..09a83c1 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
@@ -22,6 +22,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.flogger.GoogleLogger;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.events.ErrorSensingEventHandler;
@@ -244,7 +245,13 @@
QueryExpression expression, String message, DetailedExitCode detailedExitCode)
throws QueryException {
if (!keepGoing) {
- throw new QueryException(expression, message, detailedExitCode.getFailureDetail());
+ if (detailedExitCode != null) {
+ throw new QueryException(expression, message, detailedExitCode.getFailureDetail());
+ } else {
+ BugReport.sendBugReport(
+ new IllegalStateException("Undetailed failure: " + message + " for " + expression));
+ throw new QueryException(expression, message, Code.NON_DETAILED_ERROR);
+ }
}
eventHandler.handle(createErrorEvent(expression, message, detailedExitCode));
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/BUILD b/src/main/java/com/google/devtools/build/lib/query2/common/BUILD
index 648a689..9cd7ea4 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/common/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/query2/common/BUILD
@@ -14,6 +14,7 @@
"AbstractBlazeQueryEnvironment.java",
],
deps = [
+ "//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/TargetEdgeErrorObserver.java b/src/main/java/com/google/devtools/build/lib/query2/query/TargetEdgeErrorObserver.java
index cadea50..6ce7f5d 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/TargetEdgeErrorObserver.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/TargetEdgeErrorObserver.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.query2.query;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.packages.Attribute;
@@ -93,6 +94,9 @@
FailureDetail failureDetail = node.getPackage().getFailureDetail();
if (failureDetail != null) {
errorCode.compareAndSet(/*expect=*/ null, /*update=*/ DetailedExitCode.of(failureDetail));
+ } else {
+ BugReport.sendBugReport(
+ new IllegalStateException("Undetailed error from package: " + node));
}
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
index a5051ab..16a335d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
@@ -81,7 +81,9 @@
assertThat(cause).isInstanceOf(LoadingFailedCause.class);
assertThat(cause.getLabel()).isEqualTo(topLevel);
assertThat(((LoadingFailedCause) cause).getMessage())
- .isEqualTo("Target '//foo:foo' contains an error and its package is in error");
+ .isEqualTo(
+ "Target '//foo:foo' contains an error and its package is in error: //foo:foo: missing"
+ + " value for mandatory attribute 'outs' in 'genrule' rule");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
index c9b13a8..7d7e9cd 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.devtools.build.lib.testutil.TestConstants.GENRULE_SETUP;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.fail;
@@ -1800,6 +1801,28 @@
}
@Test
+ public void badRuleInDeps() throws Exception {
+ runBadRuleInDeps(Code.STARLARK_EVAL_ERROR);
+ }
+
+ protected final void runBadRuleInDeps(Object code) throws Exception {
+ writeFile("foo/BUILD", "sh_library(name = 'foo', deps = ['//bar:bar'])");
+ writeFile("bar/BUILD", "sh_library(name = 'bar', srcs = 'bad_single_file')");
+ EvalThrowsResult evalThrowsResult =
+ evalThrows("deps(//foo:foo)", /*unconditionallyThrows=*/ false);
+ FailureDetail.Builder failureDetailBuilder = FailureDetail.newBuilder();
+ if (code instanceof FailureDetails.PackageLoading.Code) {
+ failureDetailBuilder.setPackageLoading(
+ FailureDetails.PackageLoading.newBuilder().setCode((Code) code));
+ } else if (code instanceof Query.Code) {
+ failureDetailBuilder.setQuery(FailureDetails.Query.newBuilder().setCode((Query.Code) code));
+ }
+ assertThat(evalThrowsResult.getFailureDetail())
+ .comparingExpectedFieldsOnly()
+ .isEqualTo(failureDetailBuilder.build());
+ }
+
+ @Test
public void buildfilesBazel() throws Exception {
writeFile("bar/BUILD.bazel");
writeFile("bar/bar.bzl", "sym = 0");
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
index 2009df8..86eeec5 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD
@@ -60,6 +60,7 @@
"//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
"//src/test/java/com/google/devtools/build/lib/testutil:TestPackageFactoryBuilderFactory",
"//src/test/java/com/google/devtools/build/skyframe:testutil",
+ "//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:jsr305",
"//third_party:junit4",
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
index 6a86c0f..73fb8ee 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
@@ -707,6 +707,9 @@
@Override
public void testRegression1309697() {}
+ @Override
+ public void badRuleInDeps() {}
+
// Can't handle cycles.
@Override
public void testDotDotDotWithCycle() {}
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
index 135a233..28cf41b 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
@@ -71,6 +71,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsProvider;
+import com.google.errorprone.annotations.ForOverride;
import java.io.IOException;
import java.util.AbstractSet;
import java.util.ArrayList;
@@ -152,6 +153,7 @@
this.blockUniverseEvaluationErrors = blockUniverseEvaluationErrors;
}
+ @ForOverride
protected QueryEnvironmentFactory makeQueryEnvironmentFactory() {
return new QueryEnvironmentFactory();
}
