From 9e26369575f04776c0416fd75a9434a22b4d5e9a Mon Sep 17 00:00:00 2001 From: nharmata Date: Fri, 16 Jun 2017 21:14:10 +0200 Subject: Ban the combination of buildfiles/loadfiles and --output=location. RELNOTES[INC]: The --output=location flag to 'bazel query' cannot be used with query expressions that involve the 'buildfiles' or 'loadfiles' operators. This also applies to 'genquery' rules. PiperOrigin-RevId: 159259061 --- .../lib/query2/engine/BuildFilesFunction.java | 2 +- .../build/lib/query2/engine/LoadFilesFunction.java | 2 +- .../build/lib/query2/output/OutputFormatter.java | 39 ++++++++++++++++++++++ .../build/lib/rules/genquery/GenQuery.java | 7 ++-- .../build/lib/runtime/commands/QueryCommand.java | 8 +++++ src/test/shell/integration/bazel_query_test.sh | 30 +++++++++++++++++ 6 files changed, 84 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java index cbc0ae8d38..c64cb6c5fa 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/BuildFilesFunction.java @@ -31,7 +31,7 @@ import java.util.Set; * * 
expr ::= BUILDFILES '(' expr ')'
*/ -class BuildFilesFunction implements QueryFunction { +public class BuildFilesFunction implements QueryFunction { BuildFilesFunction() { } diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java b/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java index 80b912f6d7..dd25f15aa2 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/query2/engine/LoadFilesFunction.java @@ -28,7 +28,7 @@ import java.util.Set; * * 
expr ::= LOADFILES '(' expr ')'
*/ -class LoadFilesFunction implements QueryEnvironment.QueryFunction { +public class LoadFilesFunction implements QueryEnvironment.QueryFunction { LoadFilesFunction() {} @Override diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java index 78231f9d8c..d504437c22 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java +++ b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java @@ -33,8 +33,16 @@ import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TriState; +import com.google.devtools.build.lib.query2.AbstractBlazeQueryEnvironment; +import com.google.devtools.build.lib.query2.engine.BuildFilesFunction; +import com.google.devtools.build.lib.query2.engine.FunctionExpression; +import com.google.devtools.build.lib.query2.engine.LoadFilesFunction; import com.google.devtools.build.lib.query2.engine.OutputFormatterCallback; import com.google.devtools.build.lib.query2.engine.QueryEnvironment; +import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction; +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.QueryExpressionMapper; import com.google.devtools.build.lib.query2.engine.SynchronizedDelegatingOutputFormatterCallback; import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback; import com.google.devtools.build.lib.query2.output.QueryOptions.OrderOutput; @@ -55,6 +63,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** @@ -156,6 +165,10 @@ public abstract class OutputFormatter implements Serializable { : DependencyFilter.NO_IMPLICIT_DEPS); } + public void verifyCompatible(QueryEnvironment env, QueryExpression expr) + throws QueryException { + } + /** * Format the result (a set of target nodes implicitly ordered according to * the graph maintained by the QueryEnvironment), and print it to "out". @@ -369,6 +382,32 @@ public abstract class OutputFormatter implements Serializable { return "location"; } + @Override + public void verifyCompatible(QueryEnvironment env, QueryExpression expr) + throws QueryException { + if (!(env instanceof AbstractBlazeQueryEnvironment)) { + return; + } + final AtomicBoolean found = new AtomicBoolean(false); + QueryExpressionMapper noteBuildFilesAndLoadLilesMapper = new QueryExpressionMapper() { + @Override + public QueryExpression map(FunctionExpression functionExpression) { + QueryFunction queryFunction = functionExpression.getFunction(); + if (queryFunction instanceof LoadFilesFunction + || queryFunction instanceof BuildFilesFunction) { + found.set(true); + } + return super.map(functionExpression); + } + }; + expr.getMapped(noteBuildFilesAndLoadLilesMapper); + if (found.get()) { + throw new QueryException( + "Query expressions involving 'buildfiles' or 'loadfiles' cannot be used with " + + "--output=location"); + } + } + @Override public OutputFormatterCallback createPostFactoStreamCallback( OutputStream out, final QueryOptions options) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java index d044aafed3..f5abf25019 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java @@ -55,6 +55,7 @@ import com.google.devtools.build.lib.query2.engine.DigraphQueryEvalResult; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.Setting; 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.QueryUtil; import com.google.devtools.build.lib.query2.engine.QueryUtil.AggregateAllOutputFormatterCallback; import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException; @@ -300,7 +301,7 @@ public class GenQuery implements RuleConfiguredTargetFactory { ruleContext.getAnalysisEnvironment().getSkyframeEnv()); // This is a precomputed value so it should have been injected by the rules module by the // time we get there. - formatter = OutputFormatter.getFormatter( + formatter = OutputFormatter.getFormatter( Preconditions.checkNotNull(outputFormatters), queryOptions.outputFormat); // All the packages are already loaded at this point, so there is no need // to start up many threads. 4 are started up to make good use of multiple @@ -324,7 +325,9 @@ public class GenQuery implements RuleConfiguredTargetFactory { ImmutableList.of(), /*packagePath=*/ null, /*blockUniverseEvaluationErrors=*/ false); - queryResult = (DigraphQueryEvalResult) queryEnvironment.evaluateQuery(query, targets); + QueryExpression expr = QueryExpression.parse(query, queryEnvironment); + formatter.verifyCompatible(queryEnvironment, expr); + queryResult = queryEnvironment.evaluateQuery(expr, targets); } catch (SkyframeRestartQueryException e) { // Do not emit errors for skyframe restarts. They make output of the ConfiguredTargetFunction // inconsistent from run to run, and make detecting legitimate errors more difficult. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java index 47571ae9fd..8881db18ba 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java @@ -147,6 +147,14 @@ public final class QueryCommand implements BlazeCommand { .handle(Event.error(null, "Error while parsing '" + query + "': " + e.getMessage())); return ExitCode.COMMAND_LINE_ERROR; } + + try { + formatter.verifyCompatible(queryEnv, expr); + } catch (QueryException e) { + env.getReporter().handle(Event.error(e.getMessage())); + return ExitCode.COMMAND_LINE_ERROR; + } + expr = queryEnv.transformParsedQuery(expr); OutputStream out = env.getReporter().getOutErr().getOutputStream(); diff --git a/src/test/shell/integration/bazel_query_test.sh b/src/test/shell/integration/bazel_query_test.sh index b009adb147..0342370dfd 100755 --- a/src/test/shell/integration/bazel_query_test.sh +++ b/src/test/shell/integration/bazel_query_test.sh @@ -257,4 +257,34 @@ EOF expect_log "//peach:harken" } +function test_location_output_not_allowed_with_buildfiles_or_loadfiles() { + mkdir foo + cat > foo/bzl.bzl < foo/BUILD <& $TEST_log || fail "Expected success" + expect_log "//foo:bzl.bzl" + bazel query 'loadfiles(//foo)' >& $TEST_log || fail "Expected success" + expect_log "//foo:bzl.bzl" + bazel query --output=location '//foo' >& $TEST_log || fail "Expected success" + expect_log "//foo:foo" + + local expected_error_msg="Query expressions involving 'buildfiles' or 'loadfiles' cannot be used with --output=location" + local expected_exit_code=2 + for query_string in 'buildfiles(//foo)' 'loadfiles(//foo)' + do + bazel query --output=location "$query_string" >& $TEST_log \ + && fail "Expected failure" + exit_code=$? + expect_log "$expected_error_msg" + assert_equals "$expected_exit_code" "$exit_code" + done +} + + run_suite "${PRODUCT_NAME} query tests" -- cgit v1.2.3