Fix error handling in skyframe target pattern parsing
Bazel completely swallowed errors in some cases, e.g., if the pattern is
invalid like bazel build foo//bar:baz.
Note that it previously silently ignored empty targets if --experimental_skyframe_target_pattern_evaluator was passed, and now fails (which is consistent with legacy behavior). This is an intentional change, but may break users who are using the experimental flag and are passing empty strings to Bazel.
PiperOrigin-RevId: 184282856
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
index f78e0af..c36de6f 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingResult.java
@@ -13,9 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.pkgcache;
+import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.packages.Target;
-
import java.util.Collection;
/**
@@ -63,4 +63,15 @@
public String getWorkspaceName() {
return workspaceName;
}
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(LoadingResult.class)
+ .add("hasTargetPatternError", hasTargetPatternError)
+ .add("hasLoadingError", hasLoadingError)
+ .add("targetsToAnalyze", targetsToAnalyze)
+ .add("testsToRun", testsToRun)
+ .add("workspaceName", workspaceName)
+ .toString();
+ }
}
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
index b8b1e29..04ff12c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java
@@ -47,6 +47,8 @@
public static final SkyFunctionName PACKAGE_ERROR = SkyFunctionName.create("PACKAGE_ERROR");
public static final SkyFunctionName TARGET_MARKER = SkyFunctionName.create("TARGET_MARKER");
public static final SkyFunctionName TARGET_PATTERN = SkyFunctionName.create("TARGET_PATTERN");
+ public static final SkyFunctionName TARGET_PATTERN_ERROR =
+ SkyFunctionName.create("TARGET_PATTERN_ERROR");
public static final SkyFunctionName PREPARE_DEPS_OF_PATTERNS =
SkyFunctionName.create("PREPARE_DEPS_OF_PATTERNS");
public static final SkyFunctionName PREPARE_DEPS_OF_PATTERN =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 7ae5ac5..9377524 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -410,6 +410,7 @@
packageProgress));
map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
map.put(SkyFunctions.TARGET_MARKER, new TargetMarkerFunction());
+ map.put(SkyFunctions.TARGET_PATTERN_ERROR, new TargetPatternErrorFunction());
map.put(SkyFunctions.TRANSITIVE_TARGET, new TransitiveTargetFunction(ruleClassProvider));
map.put(Label.TRANSITIVE_TRAVERSAL, new TransitiveTraversalFunction());
map.put(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
new file mode 100644
index 0000000..cf21520
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
@@ -0,0 +1,61 @@
+// Copyright 2015 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.skyframe;
+
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.skyframe.LegacySkyKey;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import javax.annotation.Nullable;
+
+/**
+ * SkyFunction that throws a {@link TargetParsingException} for target pattern that could not be
+ * parsed. Must only be requested when a SkyFunction wishes to ignore the errors
+ * in a target pattern in keep_going mode, but to shut down the build in nokeep_going mode.
+ *
+ * <p>This SkyFunction never returns a value, only throws a {@link TargetParsingException}, and
+ * should never return null, since all of its dependencies should already be present.
+ */
+public class TargetPatternErrorFunction implements SkyFunction {
+ // We pass in the error message, which isn't ideal. We could consider reparsing the original
+ // pattern instead, but that requires more information.
+ public static SkyKey key(String errorMessage) {
+ return LegacySkyKey.create(SkyFunctions.TARGET_PATTERN_ERROR, errorMessage);
+ }
+
+ @Nullable
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws TargetErrorFunctionException, InterruptedException {
+ String errorMessage = (String) skyKey.argument();
+ throw new TargetErrorFunctionException(
+ new TargetParsingException(errorMessage), Transience.PERSISTENT);
+ }
+
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+
+ private static class TargetErrorFunctionException extends SkyFunctionException {
+ public TargetErrorFunctionException(
+ TargetParsingException cause, Transience transience) {
+ super(cause, transience);
+ }
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index 75d38b8..28803ff 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -206,8 +206,9 @@
*/
private static ResolvedTargets<Target> getTargetsToBuild(
Environment env, TargetPatternPhaseKey options, PathPackageLocator pkgPath)
- throws InterruptedException {
+ throws InterruptedException {
List<TargetPatternKey> patternSkyKeys = new ArrayList<>();
+ ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder();
for (TargetPatternSkyKeyOrException keyOrException :
TargetPatternValue.keys(
options.getTargetPatterns(),
@@ -224,6 +225,16 @@
// pattern could be parsed successfully).
env.getListener().post(
new ParsingFailedEvent(keyOrException.getOriginalPattern(), e.getMessage()));
+ try {
+ env.getValueOrThrow(
+ TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+ } catch (TargetParsingException ignore) {
+ // We ignore this. Keep going is active.
+ }
+ env.getListener().handle(
+ Event.error(
+ "Skipping '" + keyOrException.getOriginalPattern() + "': " + e.getMessage()));
+ builder.setError();
}
}
Map<SkyKey, ValueOrException<TargetParsingException>> resolvedPatterns =
@@ -232,7 +243,6 @@
return null;
}
- ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder();
for (TargetPatternKey pattern : patternSkyKeys) {
TargetPatternValue value;
try {
@@ -265,6 +275,12 @@
} catch (MissingDepException e) {
return null;
} catch (TargetParsingException e) {
+ try {
+ env.getValueOrThrow(
+ TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+ } catch (TargetParsingException ignore) {
+ // We ignore this. Keep going is active.
+ }
env.getListener().handle(Event.error(e.getMessage()));
return ResolvedTargets.failed();
}
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index c2180ef..6814b89 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -148,6 +148,43 @@
}
@Test
+ public void testMistypedTarget() throws Exception {
+ try {
+ tester.load("foo//bar:missing");
+ fail();
+ } catch (TargetParsingException e) {
+ assertThat(e).hasMessageThat().contains(
+ "invalid target format 'foo//bar:missing': "
+ + "invalid package name 'foo//bar': "
+ + "package names may not contain '//' path separators");
+ }
+ }
+
+ @Test
+ public void testEmptyTarget() throws Exception {
+ try {
+ tester.load("");
+ fail();
+ } catch (TargetParsingException e) {
+ assertThat(e).hasMessageThat().contains("the empty string is not a valid target");
+ }
+ }
+
+ @Test
+ public void testMistypedTargetKeepGoing() throws Exception {
+ LoadingResult result = tester.loadKeepGoing("foo//bar:missing");
+ // Legacy loading phase does _not_ report a target pattern error, and it's work to fix, so we
+ // skip this check for now.
+ if (useSkyframeTargetPatternEval()) {
+ assertThat(result.hasTargetPatternError()).isTrue();
+ }
+ tester.assertContainsError(
+ "invalid target format 'foo//bar:missing': "
+ + "invalid package name 'foo//bar': "
+ + "package names may not contain '//' path separators");
+ }
+
+ @Test
public void testBadTargetPatternWithTest() throws Exception {
tester.addFile("base/BUILD");
LoadingResult loadingResult = tester.loadTestsKeepGoing("//base:missing");
@@ -553,6 +590,20 @@
}
@Test
+ public void testCompileOneDependencyReferencesFile() throws Exception {
+ tester.addFile("base/BUILD",
+ "cc_library(name = 'hello', srcs = ['hello.cc', '//bad:bad.cc'])");
+ tester.useLoadingOptions("--compile_one_dependency");
+ try {
+ tester.load("//base:hello");
+ fail();
+ } catch (TargetParsingException e) {
+ assertThat(e).hasMessageThat()
+ .contains("--compile_one_dependency target '//base:hello' must be a file");
+ }
+ }
+
+ @Test
public void testParsingFailureReported() throws Exception {
LoadingResult loadingResult = tester.loadKeepGoing("//does_not_exist");
assertThat(loadingResult.hasTargetPatternError()).isTrue();
