From 27e15ad11410eb1014f5247fd0eeb31a46733c07 Mon Sep 17 00:00:00 2001 From: John Cater Date: Tue, 24 Nov 2020 16:19:44 -0800 Subject: [PATCH] Clean up ConfiguredTargetValueAccessor and ConfiguredTargetAccessor - Fix looking up a target from the walkable graph. - This prevents ConfiguredTargetValueAccessor from needing to look into ConfiguredTargetAccessor. - And removes duplicated code. - Move CTVA into the correct package. Part of #11993. Closes #12549. PiperOrigin-RevId: 344151199 --- .../aquery/ActionGraphQueryEnvironment.java | 3 +-- .../aquery/AqueryThreadsafeCallback.java | 1 - .../ConfiguredTargetValueAccessor.java | 19 +++++++++++--- .../cquery/ConfiguredTargetAccessor.java | 25 ++++++------------- .../lib/query2/engine/QueryEnvironment.java | 8 ++++++ 5 files changed, 31 insertions(+), 25 deletions(-) rename src/main/java/com/google/devtools/build/lib/query2/{ => aquery}/ConfiguredTargetValueAccessor.java (89%) diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java index 2b386eb935498b..23d0a1ef20c124 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java @@ -30,7 +30,6 @@ import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.pkgcache.PackageManager; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; -import com.google.devtools.build.lib.query2.ConfiguredTargetValueAccessor; import com.google.devtools.build.lib.query2.NamedThreadSafeOutputFormatterCallback; import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment; import com.google.devtools.build.lib.query2.SkyQueryEnvironment; @@ -102,7 +101,7 @@ public ActionGraphQueryEnvironment( .build(); this.accessor = new ConfiguredTargetValueAccessor( - walkableGraphSupplier.get(), this.configuredTargetKeyExtractor); + walkableGraphSupplier.get(), this::getTarget, this.configuredTargetKeyExtractor); } public ActionGraphQueryEnvironment( diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryThreadsafeCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryThreadsafeCallback.java index 75f72798684bbf..e99e006d3470c8 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryThreadsafeCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/AqueryThreadsafeCallback.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.query2.aquery; import com.google.devtools.build.lib.events.ExtendedEventHandler; -import com.google.devtools.build.lib.query2.ConfiguredTargetValueAccessor; import com.google.devtools.build.lib.query2.NamedThreadSafeOutputFormatterCallback; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor; import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue; diff --git a/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java similarity index 89% rename from src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java rename to src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java index bcb7ff81edc017..2d260231d28802 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/ConfiguredTargetValueAccessor.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ConfiguredTargetValueAccessor.java @@ -11,7 +11,7 @@ // 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.query2; +package com.google.devtools.build.lib.query2.aquery; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -21,9 +21,10 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; -import com.google.devtools.build.lib.query2.cquery.ConfiguredTargetAccessor; import com.google.devtools.build.lib.query2.engine.KeyExtractor; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor; +import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetLookup; +import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetNotFoundException; import com.google.devtools.build.lib.query2.engine.QueryException; import com.google.devtools.build.lib.query2.engine.QueryExpression; import com.google.devtools.build.lib.query2.engine.QueryVisibility; @@ -48,13 +49,16 @@ public class ConfiguredTargetValueAccessor implements TargetAccessor { private final WalkableGraph walkableGraph; + private final TargetLookup targetLookup; private final KeyExtractor configuredTargetKeyExtractor; public ConfiguredTargetValueAccessor( WalkableGraph walkableGraph, + TargetLookup targetLookup, KeyExtractor configuredTargetKeyExtractor) { this.walkableGraph = walkableGraph; + this.targetLookup = targetLookup; this.configuredTargetKeyExtractor = configuredTargetKeyExtractor; } @@ -138,8 +142,15 @@ public ImmutableSet> getVisibility( } private Target getTargetFromConfiguredTargetValue(ConfiguredTargetValue configuredTargetValue) { - return ConfiguredTargetAccessor.getTargetFromConfiguredTarget( - configuredTargetValue.getConfiguredTarget(), walkableGraph); + // Dereference any aliases that might be present. + Label label = configuredTargetValue.getConfiguredObject().getOriginalLabel(); + try { + return targetLookup.getTarget(label); + } catch (InterruptedException e) { + throw new IllegalStateException("Thread interrupted in the middle of getting a Target.", e); + } catch (TargetNotFoundException e) { + throw new IllegalStateException("Unable to get target from package in accessor.", e); + } } /** Returns the AspectValues that are attached to the given configuredTarget. */ diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java index effb45670025eb..877d459972271c 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetAccessor.java @@ -32,11 +32,11 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper; import com.google.devtools.build.lib.packages.ExecGroup; -import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor; +import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetNotFoundException; import com.google.devtools.build.lib.query2.engine.QueryException; import com.google.devtools.build.lib.query2.engine.QueryExpression; import com.google.devtools.build.lib.query2.engine.QueryVisibility; @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue; -import com.google.devtools.build.lib.skyframe.PackageValue; import com.google.devtools.build.lib.skyframe.ToolchainContextKey; import com.google.devtools.build.lib.skyframe.UnloadedToolchainContext; import com.google.devtools.build.skyframe.WalkableGraph; @@ -169,25 +168,15 @@ public ImmutableSet> getVisibility( } public Target getTargetFromConfiguredTarget(ConfiguredTarget configuredTarget) { - return getTargetFromConfiguredTarget(configuredTarget, walkableGraph); - } - - public static Target getTargetFromConfiguredTarget( - ConfiguredTarget configuredTarget, WalkableGraph walkableGraph) { - Target target = null; + // Dereference any aliases that might be present. + Label label = configuredTarget.getOriginalLabel(); try { - // Dereference any aliases that might be present. - Label label = configuredTarget.getOriginalLabel(); - target = - ((PackageValue) walkableGraph.getValue(PackageValue.key(label.getPackageIdentifier()))) - .getPackage() - .getTarget(label.getName()); - } catch (NoSuchTargetException e) { + return queryEnvironment.getTarget(label); + } catch (InterruptedException e) { + throw new IllegalStateException("Thread interrupted in the middle of getting a Target.", e); + } catch (TargetNotFoundException e) { throw new IllegalStateException("Unable to get target from package in accessor.", e); - } catch (InterruptedException e2) { - throw new IllegalStateException("Thread interrupted in the middle of getting a Target.", e2); } - return target; } /** Returns the rule that generates the given output file. */ diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java index 2cb89a5860f844..cfffefd1ae024c 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java @@ -17,7 +17,9 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.util.DetailedExitCode; import java.util.Collection; import java.util.List; @@ -159,6 +161,12 @@ public final FilteringQueryFunction asFilteringFunction() { public abstract int getExpressionToFilterIndex(); } + /** Functional interface for classes that need to look up a Target from a Label. */ + @FunctionalInterface + interface TargetLookup { + Target getTarget(Label label) throws TargetNotFoundException, InterruptedException; + } + /** * Exception type for the case where a target cannot be found. It's basically a wrapper for * whatever exception is internally thrown.