BlazeCommandDispatcher: all options parsing and editing in one place

The invocation policy must be the last step in determining the active options
for the current command. Take precautions to prevent accidental options editing
of options after the policy is applied by declaring the options as an
OptionsProvider (interface has no mutating methods); technically, this could
be circumvented by casting to OptionsParser, but it's at least safer than
before.

As part of this, I'm moving the BlazeCommand.editOptions call to before the
invocation policy, and also making minor changes to the CommandEnvironment.
Commands that expect to have the last word on options could therefore see
options changes after this commit.

There are three commands that override editOptions:
1. TestCommand
Current behavior: if test_output=streamed is set, also sets
--test_sharding_strategy=disabled and --test_strategy=exclusive.

Overriding --test_sharding_strategy is not a concern; in fact, maybe we
shouldn't be setting that in the first place, since it can cause tests to
timeout (timeout is usually applied per shard, so a 10-way sharded test will
very likely timeout). If you override the test_strategy to local or standalone,
then you may get interleaved stdout / stderr output from tests that run in
parallel - a bit surprising, but no showstopper. If you override the
test_strategy to remote, you won't get streamed output, because no remote
strategy currently supports that. You may get interleaved output, if multiple
tests finish very close to each other.

There are no correctness concerns, it's just a slightly worse user experience.
The code is safe in all cases, AFAICT.

We could consider changing streamed to not require serialized execution, and
instead use something like a lock - the first test to produce output gets
streamed, and all others get delayed until the lock is released, but could
still execute concurrently. However, it's unlikely that that's the desired
behavior for most users.

2. CoverageCommand
Current behavior: sets --collect_code_coverage, increases default
--test_timeout.

bazel coverage --nocollect_code_coverage is effectively bazel test. You can
actually now run bazel test --collect_code_coverage --test_timeout and it will
behave exactly the same way as bazel coverage. It's mostly a convenience thing.

Note that CoverageCommand inherits TestCommand, so the case above also applies.

3. MobileInstallCommand
Current behavior: sets --aspects, and --output_groups.

If you override the aspect or output_groups, then the command will fail or run
an old binary from a previous build. Not what you'd expect, but no correctness
concerns.

Summary:
IMO, the impact on the existing commands is minor. The advantage of this change
is that it's more secure, and gives us an escape hatch if a command ever
overrides options in a way that turns out to be wrong or broken in some way.

RELNOTES: None.
PiperOrigin-RevId: 160114902

2 files changed, 55 insertions, 58 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index b860bda7dc..330a69d7f8 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -52,6 +52,7 @@ import com.google.devtools.common.options.OpaqueOptionsData;
 import com.google.devtools.common.options.OptionPriority;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
+import com.google.devtools.common.options.OptionsProvider;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.PrintStream;
@@ -197,11 +198,10 @@ public class BlazeCommandDispatcher {
     return ExitCode.SUCCESS;
   }
 
-  private ExitCode parseArgsAndConfigs(CommandEnvironment env, OptionsParser optionsParser,
+  private void parseArgsAndConfigs(CommandEnvironment env, OptionsParser optionsParser,
       Command commandAnnotation, List<String> args, List<String> rcfileNotes,
       EventHandler eventHandler)
           throws OptionsParsingException {
-
     Function<String, String> commandOptionSourceFunction =
         new Function<String, String>() {
           @Override
@@ -252,15 +252,11 @@ public class BlazeCommandDispatcher {
                 "Config values are not defined in any .rc file: "
                     + Joiner.on(", ").join(unknownConfigs)));
       } else {
-        eventHandler.handle(
-            Event.error(
-                "Config values are not defined in any .rc file: "
-                    + Joiner.on(", ").join(unknownConfigs)));
-        return ExitCode.COMMAND_LINE_ERROR;
+        throw new OptionsParsingException(
+            "Config values are not defined in any .rc file: "
+                + Joiner.on(", ").join(unknownConfigs));
       }
     }
-
-    return ExitCode.SUCCESS;
   }
 
   /**
@@ -396,25 +392,29 @@ public class BlazeCommandDispatcher {
       return result.getNumericExitCode();
     }
 
-    OptionsParser optionsParser;
+    // Declare options as OptionsProvider so the options can't be easily modified after we've
+    // applied the invocation policy.
+    OptionsProvider options;
     // Delay output of notes regarding the parsed rc file, so it's possible to disable this in the
     // rc file.
     List<String> rcfileNotes = new ArrayList<>();
     try {
-      optionsParser = createOptionsParser(command);
-    } catch (OptionsParser.ConstructionException e) {
-      eventHandler.handle(
-          Event.error("Internal error while constructing options parser: " + e.getMessage()));
-      return ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode();
-    }
-    try {
+      OptionsParser optionsParser = createOptionsParser(command);
       // TODO(ulfjack): parseArgsAndConfigs calls env.getWorkingDirectory, which isn't set correctly
       // at this point in the code - it's initialized to the workspace root, which usually works.
-      ExitCode parseResult =
-          parseArgsAndConfigs(
-              env, optionsParser, commandAnnotation, args, rcfileNotes, eventHandler);
-      if (!parseResult.equals(ExitCode.SUCCESS)) {
-        return parseResult.getNumericExitCode();
+      parseArgsAndConfigs(
+          env, optionsParser, commandAnnotation, args, rcfileNotes, eventHandler);
+      // Allow the command to edit the options.
+      command.editOptions(optionsParser);
+      // Migration of --watchfs to a command option.
+      // TODO(ulfjack): Get rid of the startup option and drop this code.
+      if (runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).watchFS) {
+        try {
+          optionsParser.parse("--watchfs");
+        } catch (OptionsParsingException e) {
+          // This should never happen.
+          throw new IllegalStateException(e);
+        }
       }
       // Merge the invocation policy that is user-supplied, from the command line, and any
       // invocation policy that was added by a module. The module one goes 'first,' so the user
@@ -425,7 +425,22 @@ public class BlazeCommandDispatcher {
               .mergeFrom(invocationPolicy)
               .build();
       InvocationPolicyEnforcer optionsPolicyEnforcer = new InvocationPolicyEnforcer(combinedPolicy);
+      // Enforce the invocation policy. It is intentional that this is the last step in preparing
+      // the options. The invocation policy is used in security-critical contexts, and may be used
+      // as a last resort to override flags. That means that the policy can override flags set in
+      // BlazeCommand.editOptions, so the code needs to be safe regardless of the actual flag
+      // values. At the time of this writing, editOptions was only used as a convenience feature or
+      // to improve the user experience, but not required for safety or correctness.
       optionsPolicyEnforcer.enforce(optionsParser, commandName);
+      // Print warnings for odd options usage
+      for (String warning : optionsParser.getWarnings()) {
+        eventHandler.handle(Event.warn(warning));
+      }
+      options = optionsParser;
+    } catch (OptionsParser.ConstructionException e) {
+      eventHandler.handle(
+          Event.error("Internal error while constructing options parser: " + e.getMessage()));
+      return ExitCode.BLAZE_INTERNAL_ERROR.getNumericExitCode();
     } catch (OptionsParsingException e) {
       for (String note : rcfileNotes) {
         eventHandler.handle(Event.info(note));
@@ -436,7 +451,7 @@ public class BlazeCommandDispatcher {
 
     // Setup log filtering
     BlazeCommandEventHandler.Options eventHandlerOptions =
-        optionsParser.getOptions(BlazeCommandEventHandler.Options.class);
+        options.getOptions(BlazeCommandEventHandler.Options.class);
     OutErr colorfulOutErr = outErr;
 
     if (!eventHandlerOptions.useColor()) {
@@ -457,7 +472,7 @@ public class BlazeCommandDispatcher {
       outErr = lineBufferErr(outErr);
     }
 
-    CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class);
+    CommonCommandOptions commonOptions = options.getOptions(CommonCommandOptions.class);
     if (!commonOptions.verbosity.equals(lastLogVerbosityLevel)) {
       BlazeRuntime.setupLogging(commonOptions.verbosity);
       lastLogVerbosityLevel = commonOptions.verbosity;
@@ -511,24 +526,17 @@ public class BlazeCommandDispatcher {
       try {
         // Notify the BlazeRuntime, so it can do some initial setup.
         env.beforeCommand(
-            optionsParser,
+            options,
             commonOptions,
             execStartTimeNanos,
             waitTimeInMs,
             invocationPolicy);
-        // Allow the command to edit options after parsing:
-        command.editOptions(optionsParser);
       } catch (AbruptExitException e) {
         reporter.handle(Event.error(e.getMessage()));
         return e.getExitCode().getNumericExitCode();
       }
       for (BlazeModule module : runtime.getBlazeModules()) {
-        module.handleOptions(optionsParser);
-      }
-
-      // Print warnings for odd options usage
-      for (String warning : optionsParser.getWarnings()) {
-        reporter.handle(Event.warn(warning));
+        module.handleOptions(options);
       }
 
       env.getEventBus().post(originalCommandLine);
@@ -537,7 +545,7 @@ public class BlazeCommandDispatcher {
         env.getSkyframeExecutor().injectExtraPrecomputedValues(module.getPrecomputedValues());
       }
 
-      ExitCode outcome = command.exec(env, optionsParser);
+      ExitCode outcome = command.exec(env, options);
       outcome = env.precompleteCommand(outcome);
       numericExitCode = outcome.getNumericExitCode();
       return numericExitCode;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index d1411c70bd..48092a1a70 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -51,8 +51,6 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.OptionsClassProvider;
-import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
 import java.io.IOException;
 import java.util.Collection;
@@ -553,30 +551,21 @@ public final class CommandEnvironment {
   /**
    * Hook method called by the BlazeCommandDispatcher prior to the dispatch of each command.
    *
-   * @param options The CommonCommandOptions used by every command.
+   * @param commonOptions The CommonCommandOptions used by every command.
    * @throws AbruptExitException if this command is unsuitable to be run as specified
    */
   void beforeCommand(
-      OptionsParser optionsParser,
-      CommonCommandOptions options,
+      OptionsProvider options,
+      CommonCommandOptions commonOptions,
       long execStartTimeNanos,
       long waitTimeInMs,
       InvocationPolicy invocationPolicy)
       throws AbruptExitException {
-    commandStartTime -= options.startupTime;
-    if (runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).watchFS) {
-      try {
-        // TODO(ulfjack): Get rid of the startup option and drop this code.
-        optionsParser.parse("--watchfs");
-      } catch (OptionsParsingException e) {
-        // This should never happen.
-        throw new IllegalStateException(e);
-      }
-    }
-    this.options = optionsParser;
+    commandStartTime -= commonOptions.startupTime;
+    this.options = options;
 
     eventBus.post(
-        new GotOptionsEvent(runtime.getStartupOptionsProvider(), optionsParser, invocationPolicy));
+        new GotOptionsEvent(runtime.getStartupOptionsProvider(), options, invocationPolicy));
     throwPendingException();
 
     outputService = null;
@@ -604,7 +593,7 @@ public final class CommandEnvironment {
     Path workspace = getWorkspace();
     Path workingDirectory;
     if (inWorkspace()) {
-      workingDirectory = workspace.getRelative(options.clientCwd);
+      workingDirectory = workspace.getRelative(commonOptions.clientCwd);
     } else {
       workspace = FileSystemUtils.getWorkingDirectory(getDirectories().getFileSystem());
       workingDirectory = workspace;
@@ -612,17 +601,17 @@ public final class CommandEnvironment {
     this.relativeWorkingDirectory = workingDirectory.relativeTo(workspace);
     this.workingDirectory = workingDirectory;
 
-    updateClientEnv(options.clientEnv);
+    updateClientEnv(commonOptions.clientEnv);
 
     // Fail fast in the case where a Blaze command forgets to install the package path correctly.
     skyframeExecutor.setActive(false);
     // Let skyframe figure out if it needs to store graph edges for this build.
     skyframeExecutor.decideKeepIncrementalState(
         runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).batch,
-        optionsParser.getOptions(BuildView.Options.class));
+        options.getOptions(BuildView.Options.class));
 
     // Start the performance and memory profilers.
-    runtime.beforeCommand(this, options, execStartTimeNanos);
+    runtime.beforeCommand(this, commonOptions, execStartTimeNanos);
 
     // actionClientEnv contains the environment where values from actionEnvironment are overridden.
     actionClientEnv.putAll(clientEnv);
@@ -631,7 +620,7 @@ public final class CommandEnvironment {
       // Compute the set of environment variables that are whitelisted on the commandline
       // for inheritance.
       for (Map.Entry<String, String> entry :
-          optionsParser.getOptions(BuildConfiguration.Options.class).actionEnvironment) {
+          options.getOptions(BuildConfiguration.Options.class).actionEnvironment) {
         if (entry.getValue() == null) {
           visibleActionEnv.add(entry.getKey());
         } else {
@@ -640,7 +629,7 @@ public final class CommandEnvironment {
         }
       }
       for (Map.Entry<String, String> entry :
-          optionsParser.getOptions(BuildConfiguration.Options.class).testEnvironment) {
+          options.getOptions(BuildConfiguration.Options.class).testEnvironment) {
         if (entry.getValue() == null) {
           visibleTestEnv.add(entry.getKey());
         }
@@ -649,7 +638,7 @@ public final class CommandEnvironment {
 
     eventBus.post(new CommandStartEvent(
         command.name(), getCommandId(), getClientEnv(), workingDirectory, getDirectories(),
-        waitTimeInMs + options.waitTime));
+        waitTimeInMs + commonOptions.waitTime));
   }
 
   /** Returns the name of the file system we are writing output to. */