From 3533ab5a77254e09e0ac8366c49d279a7179baf9 Mon Sep 17 00:00:00 2001 From: Janak Ramakrishnan Date: Thu, 14 Jan 2016 18:14:09 +0000 Subject: Stop filtering out targets not in the graph in SkyQueryEnvironment. Instead, just warn if a target turns out to not be present. This means that queries may return unexpected results. For instance, if "query" means "bazel query --order_output=no ", then here are the results of two queries: query --universe_scope=//foo/... //foo:output_file //foo:output_file query --universe_scope=//foo/... deps(//foo:output_file) WARNING: Targets not in graph [//foo:output_file generated_file] //foo:output_file -- MOS_MIGRATED_REVID=112163475 --- .../build/lib/query2/SkyQueryEnvironment.java | 34 ++++++++++++---------- .../build/skyframe/DelegatingWalkableGraph.java | 8 +++-- 2 files changed, 25 insertions(+), 17 deletions(-) (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java index 28431bec76..927b4ec09f 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java @@ -23,7 +23,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet.Builder; import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.Sets; import com.google.devtools.build.lib.cmdline.Label; @@ -32,6 +31,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.collect.CompactHashSet; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.graph.Digraph; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; @@ -231,10 +231,21 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment { }); } + /** Targets may not be in the graph because they are not in the universe or depend on cycles. */ + private void warnIfMissingTargets( + Iterable targets, Set result) { + if (Iterables.size(targets) != result.size()) { + Set missingTargets = Sets.difference(ImmutableSet.copyOf(targets), result); + eventHandler.handle(Event.warn("Targets were missing from graph: " + missingTargets)); + } + } + @Override public Collection getFwdDeps(Iterable targets) { Set result = new HashSet<>(); - for (Map.Entry> entry : getRawFwdDeps(targets).entrySet()) { + Map> rawFwdDeps = getRawFwdDeps(targets); + warnIfMissingTargets(targets, rawFwdDeps.keySet()); + for (Map.Entry> entry : rawFwdDeps.entrySet()) { result.addAll(filterFwdDeps(entry.getKey(), entry.getValue())); } return result; @@ -244,6 +255,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment { public Collection getReverseDeps(Iterable targets) { Set result = CompactHashSet.create(); Map> rawReverseDeps = getRawReverseDeps(targets); + warnIfMissingTargets(targets, rawReverseDeps.keySet()); CompactHashSet visited = CompactHashSet.create(); @@ -354,7 +366,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment { targets.add(target); } } - batchStreamedCallback.process(filterTargetsNotInGraph(targets)); + batchStreamedCallback.process(targets); } private void processLastPending() throws QueryException, InterruptedException { @@ -547,9 +559,11 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment { graph.getMissingAndExceptions(unsuccessfulKeys).entrySet(); for (Map.Entry entry : errorEntries) { if (entry.getValue() == null) { - throw new QueryException(entry.getKey().argument() + " does not exist in graph"); + // Targets may be in the graph because they are not in the universe or depend on cycles. + eventHandler.handle(Event.warn(entry.getKey().argument() + " does not exist in graph")); + } else { + errorMessagesBuilder.add(entry.getValue().getMessage()); } - errorMessagesBuilder.add(entry.getValue().getMessage()); } // Lastly, report all found errors. @@ -559,16 +573,6 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment { } } - private Set filterTargetsNotInGraph(Set targets) { - Map map = Maps.toMap(targets, TARGET_TO_SKY_KEY); - Set present = graph.getSuccessfulValues(map.values()).keySet(); - if (present.size() == targets.size()) { - // Optimize for case of all targets being in graph. - return targets; - } - return Maps.filterValues(map, Predicates.in(present)).keySet(); - } - @Override protected void preloadOrThrow(QueryExpression caller, Collection patterns) throws QueryException, TargetParsingException { diff --git a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java index fb1b00e588..c99bc32285 100644 --- a/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java +++ b/src/main/java/com/google/devtools/build/skyframe/DelegatingWalkableGraph.java @@ -15,7 +15,6 @@ package com.google.devtools.build.skyframe; import com.google.common.base.Function; import com.google.common.base.Predicates; -import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.devtools.build.lib.util.Preconditions; @@ -52,9 +51,14 @@ public class DelegatingWalkableGraph implements WalkableGraph { return entry; } + /** + * Returns a map giving the {@link NodeEntry} corresponding to the given {@code keys}. If there is + * no node in the graph corresponding to a {@link SkyKey} in {@code keys}, it is silently ignored + * and will not be present in the returned map. This tolerance allows callers to avoid + * pre-filtering their keys by checking for existence, which can be expensive. + */ private static Map getEntries(Iterable keys, QueryableGraph graph) { Map result = graph.getBatch(keys); - Preconditions.checkState(result.size() == Iterables.size(keys), "%s %s", keys, result); for (Map.Entry entry : result.entrySet()) { Preconditions.checkState(entry.getValue().isDone(), entry); } -- cgit v1.2.3