cquery: interpret "//foo:bar" as "all configured targets with label //foo:bar". This is the final change for "cquery somepath: easy mode": https://docs.google.com/document/d/1ZbcOi8htQrKichHGWq6Dc8-HI7oFjvoBYeRoTedsgWY/edit RELNOTES: cquery: "//foo:bar" now means "all configured targets with label //foo:bar" instead of "choose an arbitrary configured target with label //foo:bar". See cquery docs for details. PiperOrigin-RevId: 290839563
diff --git a/site/docs/cquery.html b/site/docs/cquery.html index 2ee676a..098e0e0 100644 --- a/site/docs/cquery.html +++ b/site/docs/cquery.html
@@ -7,6 +7,7 @@ <ul class="toc"> <li><a href="#overview">Overview</a></li> <li><a href="#basic-syntax">Basic Syntax</a></li> + <li><a href="#target-pattern-evaluation">Target Pattern Evaluation</a></li> <li><a href="#functions">Functions</a></li> <li><a href="#options">Options</a></li> <li><a href="#compare">cquery vs. query</a></li> @@ -77,10 +78,10 @@ <p> Each result includes information about the configuration in which the target was built. For most targets, this is an (opaque) hash of the build options - and values in that configuration. Targets configured in the host configuration - are marked with HOST. Input files, like those found in the <code>srcs</code> - attribute of many library rules, have no need for configuration and are marked - with null. + and values in that configuration. Host-configured targets are marked + <code>(HOST)</code>. Non-generated source files, like those commonly + found in <code>srcs</code>, have no need for configuration and are + marked <code>(null)</code>. </p> <p> @@ -122,54 +123,64 @@ for querying dependencies of top-level build targets. </p> +<h2 id='target-pattern-evaluation'>Target Pattern Evaluation</h2> + +<p> + <code>//foo</code> has a different meaning for <code>cquery</code> than + for <code>query</code>. This is because <code>cquery</code> + evaluates <i>configured</i> targets and the build graph may have multiple + configured versions of <code>//foo</code>. +</p> + +<p> + For <code>cquery</code>, a target pattern in the query expression evaluates + to every configured target with a label that matches that pattern. Output is + deterministic, but <code>cquery</code> makes no ordering guarantee beyond the + <a href="query.html#graph-order">core query ordering contract</a>. +</p> + +<p> + This produces subtler results for query expressions than with <code>query</code>. + For example, the following can produce multiple results: + + <pre> +# Analyzes //foo in the target configuration, but also analyzes +# //genrule_with_foo_as_tool which depends on a host-configured +# //foo. So there are two configured target instances of //foo in +# the build graph. +$ bazel cquery //foo --universe_scope=//foo,//genrule_with_foo_as_tool +//foo (9f87702f17541d32a56488e3b8be8a98e52b06945f7) +//foo (HOST) + </pre> + +</p> + +<p> + If you want to precisely declare which instance to query over, use + the <a href="#config"><code>config</code></a> function. +</p> + + +<p> + See <code>query</code>'s <a href="query.html#target-patterns">target pattern + documentation</a> for more information on target patterns. +</p> + <h2 id='functions'>Functions</h2> <p> Of the <a href="query.html#functions" title="list of query functions">set of functions</a> - supported by the traditional <code>query</code>, <code>cquery</code> supports + supported by <code>query</code>, <code>cquery</code> supports all - but <a href="query.html#siblings">siblings</a>, <a href="query.html#buildfiles">buildfiles</a>, - and <a href="query.html#tests">tests</a>. -</p> - -<h3><code>query</code> functions with <code>cquery</code>-specific notes:</h3> - -<h4 id="somepath">somepath</h4> - -<p><code>expr ::= somepath(expr, expr)</code></p> - -<p> - <code>cquery</code>'s <code>somepath</code> works conceptually exactly - like <code>query</code>'s: it computes paths between two sets of targets. But - since <code>cquery</code> evaluates <i>configured</i> targets, <code>//foo</code> - and <code>//bar</code> are ambiguous. What if a - top level-configured <code>//foo</code> depends on a - top level-configured <code>//bar</code> and a custom-configured <code>//foo</code> - depends on a host-configured <code>//bar</code>? Which ones - should <code>cquery</code> choose? + but <a href="query.html#siblings"><code>siblings</code></a>, <a href="query.html#buildfiles"><code>buildfiles</code></a>, + and <a href="query.html#tests"><code>tests</code></a>. </p> <p> - When multiple choices exist, <code>cquery</code> chooses an arbitrary one. This - choice is deterministic but no guarantee is made which one will be chosen. - This might mean <code>cquery</code> searches a version of <code>//bar</code> - that <i>isn't</i> in <code>foo</code>'s deps, even if some other version is. + <code>cquery</code> also introduces the following new functions: </p> -<p> - If <code>somepath</code> isn't returning any paths, try grepping for - your <i>to</i> target from <code>deps(//from-target)</code>, i.e. <code>bazel - cquery 'deps(//from-target)' | grep - '//to-target'</code>. If <code>//to-target</code> appears in the results, - re-run <code>somepath</code> with - <a href="#config"><code>config()</code></a> to clarify which version you want, - e.g.: <code>bazel cquery 'somepath(//from-target, config(//to-target, - host))'</code>). -</p> - -<h3><code>cquery</code>-unique functions:</h3> - -<h4 id="config">config</h4> +<h3 id="config">config</h3> <p><code>expr ::= config(expr, word)</code></p>
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java index 4038802..61ce4d8 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java
@@ -296,11 +296,7 @@ partialResult -> { List<ConfiguredTarget> transformedResult = new ArrayList<>(); for (Target target : partialResult) { - ConfiguredTarget configuredTarget = - getConfiguredTarget(target.getLabel()); - if (configuredTarget != null) { - transformedResult.add(configuredTarget); - } + transformedResult.addAll(getConfiguredTargets(target.getLabel())); } callback.process(transformedResult); }, @@ -313,39 +309,6 @@ } } - private ConfiguredTarget getConfiguredTarget(Label label) throws InterruptedException { - // Try with target configuration. - ConfiguredTarget configuredTarget = getTargetConfiguredTarget(label); - if (configuredTarget != null) { - return configuredTarget; - } - // Try with host configuration (even when --notool_deps is set in the case that top-level - // targets are configured in the host configuration so we are doing a host-configuration-only - // query). - configuredTarget = getHostConfiguredTarget(label); - if (configuredTarget != null) { - return configuredTarget; - } - - // Try as a source file. - configuredTarget = getNullConfiguredTarget(label); - if (configuredTarget != null) { - return configuredTarget; - } - - // Finally, try every other configuration in the build (e.g. configurations that are the result - // of transitions and therefore not top-level). - for (BuildConfiguration configuration : transitiveConfigurations.values()) { - configuredTarget = getConfiguredTarget(label, configuration); - if (configuredTarget != null) { - return configuredTarget; - } - } - - // No matches: give up. - return null; - } - /** * Returns the {@link ConfiguredTarget} for the given label and configuration if it exists, else * null. @@ -364,6 +327,26 @@ } /** + * Returns all configured targets in Skyframe with the given label. + * + * <p>If there are no matches, returns an empty list. + */ + private List<ConfiguredTarget> getConfiguredTargets(Label label) throws InterruptedException { + ImmutableList.Builder<ConfiguredTarget> ans = ImmutableList.builder(); + for (BuildConfiguration config : transitiveConfigurations.values()) { + ConfiguredTarget ct = getConfiguredTarget(label, config); + if (ct != null) { + ans.add(ct); + } + } + ConfiguredTarget nullConfiguredTarget = getNullConfiguredTarget(label); + if (nullConfiguredTarget != null) { + ans.add(nullConfiguredTarget); + } + return ans.build(); + } + + /** * Processes the targets in {@code targets} with the requested {@code configuration} * * @param pattern the original pattern that {@code targets} were parsed from. Used for error @@ -379,53 +362,50 @@ ThreadSafeMutableSet<ConfiguredTarget> targets, String configuration, Callback<ConfiguredTarget> callback) { - return new QueryTaskCallable<Void>() { - @Override - public Void call() throws QueryException, InterruptedException { - List<ConfiguredTarget> transformedResult = new ArrayList<>(); - boolean userFriendlyConfigName = true; - for (ConfiguredTarget target : targets) { - Label label = getCorrectLabel(target); - ConfiguredTarget configuredTarget; - switch (configuration) { - case "host": - configuredTarget = getHostConfiguredTarget(label); + return () -> { + List<ConfiguredTarget> transformedResult = new ArrayList<>(); + boolean userFriendlyConfigName = true; + for (ConfiguredTarget target : targets) { + Label label = getCorrectLabel(target); + ConfiguredTarget configuredTarget; + switch (configuration) { + case "host": + configuredTarget = getHostConfiguredTarget(label); + break; + case "target": + configuredTarget = getTargetConfiguredTarget(label); + break; + case "null": + configuredTarget = getNullConfiguredTarget(label); + break; + default: + BuildConfiguration config = transitiveConfigurations.get(configuration); + if (config != null) { + configuredTarget = getConfiguredTarget(label, config); + userFriendlyConfigName = false; break; - case "target": - configuredTarget = getTargetConfiguredTarget(label); - break; - case "null": - configuredTarget = getNullConfiguredTarget(label); - break; - default: - BuildConfiguration config = transitiveConfigurations.get(configuration); - if (config != null) { - configuredTarget = getConfiguredTarget(label, config); - userFriendlyConfigName = false; - break; - } - throw new QueryException( - "Unknown value '" - + configuration - + "'. The second argument of config() must be 'target', 'host', 'null', or a" - + " valid configuration hash (i.e. one of the outputs of 'blaze config')"); - } - if (configuredTarget != null) { - transformedResult.add(configuredTarget); - } + } + throw new QueryException( + "Unknown value '" + + configuration + + "'. The second argument of config() must be 'target', 'host', 'null', or a" + + " valid configuration hash (i.e. one of the outputs of 'blaze config')"); } - if (transformedResult.isEmpty()) { - throw new QueryException( - String.format( - "No target (in) %s could be found in the %s", - pattern, - userFriendlyConfigName - ? "'" + configuration + "' configuration" - : "configuration with checksum '" + configuration + "'")); + if (configuredTarget != null) { + transformedResult.add(configuredTarget); } - callback.process(transformedResult); - return null; } + if (transformedResult.isEmpty()) { + throw new QueryException( + String.format( + "No target (in) %s could be found in the %s", + pattern, + userFriendlyConfigName + ? "'" + configuration + "' configuration" + : "configuration with checksum '" + configuration + "'")); + } + callback.process(transformedResult); + return null; }; }
diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java index 9635d54..7c196eb 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java
@@ -573,4 +573,51 @@ eval("somepath(//test:top, filter(//test:bar, deps(//test:top)))"); assertThat(result).isNotEmpty(); } + + @Override + public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel() { + // When the same target exists in multiple configurations, cquery doesn't guarantee which + // instance is evaluated first. So disable this test. + } + + @Test + public void testLabelExpressionsMatchesAllConfiguredTargetsWithLabel() throws Exception { + createConfigTransitioningRuleClass(); + writeFile( + "test/BUILD", + "load('//test:rules.bzl', 'rule_with_deps_transition', 'simple_rule', 'string_flag')", + "string_flag(", + " name = 'my_flag',", + " build_setting_default = '')", + "rule_with_deps_transition(", + " name = 'transitioner',", + " deps = [':simple'])", + "simple_rule(name = 'simple')"); + + helper.setUniverseScope("//test:transitioner,//test:simple"); + Set<ConfiguredTarget> result = eval("//test:simple"); + assertThat(result.size()).isEqualTo(2); + } + + @Test + public void testConfigFunctionRefinesMultipleMatches() throws Exception { + // Peer to testLabelExpressionsMatchesAllConfiguredTargetsWithLabel. The point of that test is + // to show "cquery //foo:bar" might return multiple configured targets. The point of this test + // is to show that config() can refine the same query to a specific one. + createConfigTransitioningRuleClass(); + writeFile( + "test/BUILD", + "load('//test:rules.bzl', 'rule_with_deps_transition', 'simple_rule', 'string_flag')", + "string_flag(", + " name = 'my_flag',", + " build_setting_default = '')", + "rule_with_deps_transition(", + " name = 'transitioner',", + " deps = [':simple'])", + "simple_rule(name = 'simple')"); + + helper.setUniverseScope("//test:transitioner,//test:simple"); + Set<ConfiguredTarget> result = eval("config(//test:simple, target)"); + assertThat(result.size()).isEqualTo(1); + } }
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 8d8317f..638a1d0 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
@@ -13,13 +13,13 @@ // limitations under the License. package com.google.devtools.build.lib.query2.testutil; +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.devtools.build.lib.testutil.TestConstants.GENRULE_SETUP; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.fail; -import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -175,16 +175,11 @@ } protected ImmutableList<String> resultSetToListOfStrings(Set<T> results) { - return Ordering.natural() - .immutableSortedCopy( - Iterables.transform( - results, - new Function<T, String>() { - @Override - public String apply(T node) { - return helper.getLabel(node); - } - })); + return results.stream() + .map(node -> helper.getLabel(node)) + .distinct() + .sorted(Ordering.natural()) + .collect(toImmutableList()); } protected void assertContains(Set<T> x, Set<T> y) throws Exception {