Move storage of queryExpression for cquery out of BuildRequest and straight into processRequest method in BuildTool. This is an extension of CL/181816980 and prevents pollution of BuildRequest.

PiperOrigin-RevId: 182576704

3 files changed, 41 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
index 6d400c6699..35328c1908 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
@@ -28,7 +28,6 @@ import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
 import com.google.devtools.build.lib.pkgcache.LoadingOptions;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
-import com.google.devtools.build.lib.query2.engine.QueryExpression;
 import com.google.devtools.build.lib.runtime.BlazeCommandEventHandler;
 import com.google.devtools.build.lib.runtime.KeepGoingOption;
 import com.google.devtools.build.lib.runtime.LoadingPhaseThreadsOption;
@@ -41,7 +40,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
-import javax.annotation.Nullable;
 
 /**
  * A BuildRequest represents a single invocation of the build tool by a user.
@@ -71,8 +69,6 @@ public class BuildRequest implements OptionsClassProvider {
   private boolean runningInEmacs;
   private boolean runTests;
 
-  private QueryExpression queryExpression = null;
-
   private static final ImmutableList<Class<? extends OptionsBase>> MANDATORY_OPTIONS =
       ImmutableList.of(
           BuildRequestOptions.class,
@@ -171,22 +167,6 @@ public class BuildRequest implements OptionsClassProvider {
     return outErr;
   }
 
-  /**
-   * If this BuildRequest was created as part of a cquery, return the query expression, if not this
-   * will return null. TODO(juliexxia): find a better way to get this information through to
-   * BuildTool without polluting BuildRequest (like put into constructor of BuildTool or refactor to
-   * have a cquery specific BuildTool).
-   */
-  @Nullable
-  public QueryExpression getQueryExpression() {
-    return queryExpression;
-  }
-
-  /** Set this BuildRequest's query expression. */
-  public void setQueryExpression(QueryExpression expr) {
-    this.queryExpression = expr;
-  }
-
   @Override
   @SuppressWarnings("unchecked")
   public <T extends OptionsBase> T getOptions(Class<T> clazz) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 5adcbc8cc5..43031f6550 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -112,12 +112,21 @@ public final class BuildTool {
     this.runtime = env.getRuntime();
   }
 
+  public void buildTargets(BuildRequest request, BuildResult result, TargetValidator validator)
+      throws BuildFailedException, InterruptedException, ViewCreationFailedException,
+          TargetParsingException, LoadingFailedException, AbruptExitException,
+          InvalidConfigurationException, TestExecException,
+          ConfiguredTargetQueryCommandLineException {
+    buildTargets(request, result, validator, null);
+  }
+
   /**
    * The crux of the build system. Builds the targets specified in the request using the specified
    * Executor.
    *
    * <p>Performs loading, analysis and execution for the specified set of targets, honoring the
-   * configuration options in the BuildRequest. Returns normally iff successful, throws an exception
+   * configuration options in the BuildRequest. Also performs a query on results of analysis phase
+   * if a query expression is specified. Returns normally iff successful, throws an exception
    * otherwise.
    *
    * <p>Callers must ensure that {@link #stopRequest} is called after this method, even if it
@@ -133,8 +142,14 @@ public final class BuildTool {
    *     the request object are populated
    * @param result the build result that is the mutable result of this build
    * @param validator target validator
+   * @param queryExpression if a query is to run after building targets, this will be the query
+   *     expression. If not, this will be null.
    */
-  public void buildTargets(BuildRequest request, BuildResult result, TargetValidator validator)
+  public void buildTargets(
+      BuildRequest request,
+      BuildResult result,
+      TargetValidator validator,
+      QueryExpression queryExpression)
       throws BuildFailedException, InterruptedException, ViewCreationFailedException,
           TargetParsingException, LoadingFailedException, AbruptExitException,
           InvalidConfigurationException, TestExecException,
@@ -238,14 +253,17 @@ public final class BuildTool {
         // not reproducible at the level of a single command. Either tolerate, or wipe the analysis
         // graph beforehand if this option is specified, or add another option to wipe if desired
         // (SkyframeExecutor#handleConfiguredTargetChange should be sufficient).
-        if (request.getQueryExpression() != null) {
+        if (queryExpression != null) {
           if (!env.getSkyframeExecutor().tracksStateForIncrementality()) {
             throw new ConfiguredTargetQueryCommandLineException(
                 "Configured query is not allowed if incrementality state is not being kept");
           }
           try {
             doConfiguredTargetQuery(
-                request, configurations, analysisResult.getTopLevelTargetsWithConfigs());
+                request,
+                configurations.getHostConfiguration(),
+                analysisResult.getTopLevelTargetsWithConfigs(),
+                queryExpression);
           } catch (QueryException | IOException e) {
             if (!request.getKeepGoing()) {
               throw new ViewCreationFailedException("Error doing configured target query", e);
@@ -325,12 +343,18 @@ public final class BuildTool {
       getReporter().handle(Event.error(e.getMessage()));
     }
   }
+
+  public BuildResult processRequest(BuildRequest request, TargetValidator validator) {
+    return processRequest(request, validator, null);
+  }
+
   /**
    * The crux of the build system. Builds the targets specified in the request using the specified
    * Executor.
    *
    * <p>Performs loading, analysis and execution for the specified set of targets, honoring the
-   * configuration options in the BuildRequest. Returns normally iff successful, throws an exception
+   * configuration options in the BuildRequest. Also performs a query on results of analysis phase
+   * if a query expression is specified. Returns normally iff successful, throws an exception
    * otherwise.
    *
    * <p>The caller is responsible for setting up and syncing the package cache.
@@ -342,16 +366,19 @@ public final class BuildTool {
    *        options; during this method's execution, the actualTargets and successfulTargets fields
    *        of the request object are populated
    * @param validator target validator
+   * @param queryExpression if a query is to run after building targets, this will be the query
+   *     expression. If not, this will be null.
    * @return the result as a {@link BuildResult} object
    */
-  public BuildResult processRequest(BuildRequest request, TargetValidator validator) {
+  public BuildResult processRequest(
+      BuildRequest request, TargetValidator validator, QueryExpression queryExpression) {
     BuildResult result = new BuildResult(request.getStartTime());
     env.getEventBus().register(result);
     maybeSetStopOnFirstFailure(request, result);
     Throwable catastrophe = null;
     ExitCode exitCode = ExitCode.BLAZE_INTERNAL_ERROR;
     try {
-      buildTargets(request, result, validator);
+      buildTargets(request, result, validator, queryExpression);
       exitCode = ExitCode.SUCCESS;
     } catch (BuildFailedException e) {
       if (e.isErrorAlreadyShown()) {
@@ -412,12 +439,11 @@ public final class BuildTool {
 
   private void doConfiguredTargetQuery(
       BuildRequest request,
-      BuildConfigurationCollection configurations,
-      List<TargetAndConfiguration> topLevelTargetsWithConfigs)
+      BuildConfiguration hostConfiguration,
+      List<TargetAndConfiguration> topLevelTargetsWithConfigs,
+      QueryExpression queryExpression)
       throws InterruptedException, QueryException, IOException {
 
-    QueryExpression expr = request.getQueryExpression();
-
     // Currently, CTQE assumes that all top level targets take on the same default config and we
     // don't have the ability to map multiple configs to multiple top level targets.
     // So for now, we only allow multiple targets when they all carry the same config.
@@ -427,7 +453,7 @@ public final class BuildTool {
     for (TargetAndConfiguration targAndConfig : topLevelTargetsWithConfigs) {
       if (!targAndConfig.getConfiguration().equals(sampleConfig)) {
         throw new QueryException(
-            new TargetLiteral(expr.toString()),
+            new TargetLiteral(queryExpression.toString()),
             String.format(
                 "Top level targets %s and %s have different configurations (top level "
                     + "targets with different configurations is not supported)",
@@ -443,14 +469,14 @@ public final class BuildTool {
             env.getReporter(),
             env.getRuntime().getQueryFunctions(),
             sampleConfig,
-            configurations.getHostConfiguration(),
+            hostConfiguration,
             env.newTargetPatternEvaluator().getOffset(),
             env.getPackageManager().getPackagePath(),
             () -> walkableGraph,
             request.getOptions(CommonQueryOptions.class).toSettings());
     QueryEvalResult result =
         configuredTargetQueryEnvironment.evaluateQuery(
-            expr,
+            queryExpression,
             new ThreadSafeOutputFormatterCallback<ConfiguredTarget>() {
               @Override
               public void processOutput(Iterable<ConfiguredTarget> partialResult)
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CqueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CqueryCommand.java
index 9c1ba0abb5..b99002ad9c 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CqueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CqueryCommand.java
@@ -104,7 +104,6 @@ public final class CqueryCommand implements BlazeCommand {
             env.getReporter().getOutErr(),
             env.getCommandId(),
             env.getCommandStartTime());
-    request.setQueryExpression(expr);
-    return new BuildTool(env).processRequest(request, null).getExitCondition();
+    return new BuildTool(env).processRequest(request, null, expr).getExitCondition();
   }
 }