Implement alternate genquery scope collection
The pre-existing GenQuery implementation relied on Skyframe's evaluation
of TransitiveTarget values, as deps of a given genquery rule's
ConfiguredTarget evaluation, to load the transitive closure of the
targets referred to by that genquery rule's `scope` attribute value.
TransitiveTarget nodes are one-to-one with build targets and the Skyframe
dependencies between them correspond with their target-space dependencies
(which may be either due to rules' attribute values, or due to those
rules' applicable aspects as determined by their providers).
Embedding genquery rules' scopes' target-space dependency graphs into
Skyframe as part of those rules' ConfiguredTarget Skyframe evaluations
causes problems when those dependency graphs have cycles. Skyframe
evaluations unconditionally fail when *they* have cycles. However,
`blaze query` semantics allow for a query expression to evaluate to a
non-failure value when the target-space dependency graph involved in
that evaluation contains cycles. Knowing nothing about genquery's
implementation, a Blaze user ought to expect genquery's semantics to be
similar to `blaze query`'s, modulo the constraint explicitly imposed by
genquery's `scope` mechanism. Because of genquery's reliance on
TransitiveTarget Skyframe evaluations, this expectation is not satisfied.
This CL introduces a flag, `--experimental_skip_ttvs_for_genquery`, which
causes genquery to use a different strategy when it is set to true. The
new strategy does not use TransitiveTarget values. Instead, genquery's
ConfiguredTarget Skyframe evaluation gathers necessary package data by
requesting a Skyframe node, GenQueryDirectPackageProviderFactory.Key,
which computes the transitive closure of the genquery rule's scope,
depending on Package values (and Package-related values like
ContainingPackageLookup, as necessary for incremental correctness).
The flag's default value is false, meaning, genquery's strategy remains
the same as before, vulnerable to cycles. Switching this default may come
in a future CL.
Comments in GenQueryDirectPackageProviderFactory.java document the new
strategy.
PiperOrigin-RevId: 474540264
Change-Id: I96092de4281f40d2f91aa4c152349dc6e16de30c
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
index 3b82f10..7029b62 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
@@ -23,7 +23,7 @@
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.skyframe.TransitiveBaseTraversalFunction.TargetAndErrorIfAnyImpl;
+import com.google.devtools.build.lib.skyframe.TargetLoadingUtil.TargetAndErrorIfAny;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.util.GroupedList;
import com.google.devtools.build.lib.util.GroupedList.GroupedListHelper;
@@ -48,8 +48,8 @@
Label label = Label.parseCanonical("//foo:foo");
scratch.file("foo/BUILD", "sh_library(name = '" + label.getName() + "')");
Package pkg = loadPackage(label.getPackageIdentifier());
- TargetAndErrorIfAnyImpl targetAndErrorIfAny =
- new TargetAndErrorIfAnyImpl(
+ TargetAndErrorIfAny targetAndErrorIfAny =
+ new TargetAndErrorIfAny(
/*packageLoadedSuccessfully=*/ true,
/*errorLoadingTarget=*/ null,
pkg.getTarget(label.getName()));
@@ -105,8 +105,8 @@
scratch.file(
"foo/BUILD", "sh_library(name = '" + label.getName() + "', deps = [':bar', ':baz'])");
Package pkg = loadPackage(label.getPackageIdentifier());
- TargetAndErrorIfAnyImpl targetAndErrorIfAny =
- new TargetAndErrorIfAnyImpl(
+ TargetAndErrorIfAny targetAndErrorIfAny =
+ new TargetAndErrorIfAny(
/*packageLoadedSuccessfully=*/ true,
/*errorLoadingTarget=*/ null,
pkg.getTarget(label.getName()));
@@ -144,8 +144,8 @@
Label label = Label.parseCanonical("//foo:foo");
scratch.file("foo/BUILD", "sh_library(name = '" + label.getName() + "', deps = [':bar'])");
Package pkg = loadPackage(label.getPackageIdentifier());
- TargetAndErrorIfAnyImpl targetAndErrorIfAny =
- new TargetAndErrorIfAnyImpl(
+ TargetAndErrorIfAny targetAndErrorIfAny =
+ new TargetAndErrorIfAny(
/*packageLoadedSuccessfully=*/ true,
/*errorLoadingTarget=*/ new NoSuchTargetException("self error is long and last"),
pkg.getTarget(label.getName()));
@@ -196,8 +196,8 @@
"load('//test:aspect.bzl', 'my_rule')",
"my_rule(name = 'foo',attr = [':bad'])");
Package pkg = loadPackage(label.getPackageIdentifier());
- TargetAndErrorIfAnyImpl targetAndErrorIfAny =
- new TargetAndErrorIfAnyImpl(
+ TargetAndErrorIfAny targetAndErrorIfAny =
+ new TargetAndErrorIfAny(
/*packageLoadedSuccessfully=*/ true,
/*errorLoadingTarget=*/ null,
pkg.getTarget(label.getName()));