Stop double-wrapping --run_under commandlines with bash

On non-windows platforms, `--run_under='cd /tmp &&' -- //foo:bar` would
become `/bin/bash -c '/bin/bash -c \'cd /tmp && /foo/bar\''`, now it's just
`/bin/bash -c 'cd /tmp && /foo/bar'`. This is a lot cleaner and less confusing
when it comes to trying to interpret or do anything novel with the command line.

The double-wrapping seems to have existed because that's the way the code
evolved, not necessity. See unknown commit for when the unconditional outer
/bin/bash was added.

This wound up being a lot more invasive than I had originally hoped, but I
think the restructuring is for the better. While all of the divergent ways of
doing things across and within linux and windows remain, at least they're
consolidated and better documented via tests.

RunCommandLine is moved to a builder to consolidate duplicate updates of args,
pretty args, and args-without-residue. Command-line production is implemented
via a new formatter interface, with a linux (aka non-windows) and windows
implementation.

Includes what I think is drive-by fix for the run executable not being escaped
with --run_under. That is, while I think the previous behavior was probably
functional, if we had thought about it I think we'd want to escape.

There are some bug-for-bug ports of existing issues, notably windows escaping
bugs. I tried to create github issues and linked TODOs for these.

There's probably more cleanup that can be done, but given the invasiveness I
tried to keep things focused.

PiperOrigin-RevId: 624974102
Change-Id: I973d473eb5afb2fd76a5d98cc624c236396af7fa

src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -86,14 +86,12 @@
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Interrupted;
 import com.google.devtools.build.lib.server.FailureDetails.RunCommand.Code;
-import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.InterruptedFailureDetails;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OptionsUtils;
 import com.google.devtools.build.lib.util.ScriptUtil;
-import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -266,7 +264,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
             .getStartupOptionsProvider()
             .getOptions(BlazeServerStartupOptions.class)
             .batch;
-    TreeMap<String, String> finalRunEnv = new TreeMap<>(runCommandLine.runEnvironment);
+    TreeMap<String, String> finalRunEnv = new TreeMap<>(runCommandLine.getEnvironment());
     if (batchMode) {
       // In --batch, prioritize original client env-var values over those added by the c++ launcher.
       // Only necessary in --batch since the command runs as a subprocess of the java server.
@@ -283,14 +281,13 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
                       .configuration
                       .getTestLogsDirectory(RepositoryName.MAIN)
                       .getExecPathString(),
-                  runCommandLine.isTestTarget)
+                  runCommandLine.isTestTarget())
               : ImmutableList.of();
 
       execRequest =
           execRequestBuilder(
               env,
-              runCommandLine.workingDir,
-              runCommandLine.args,
+              runCommandLine,
               ImmutableSortedMap.copyOf(finalRunEnv),
               ENV_VARIABLES_TO_CLEAR,
               builtTargets.configuration,
@@ -306,18 +303,10 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
     }
     if (runOptions.runBuiltTarget) {
       env.getReporter()
-          .handle(
-              Event.info(
-                  null,
-                  "Running command line: "
-                      + ShellEscaper.escapeJoinAll(runCommandLine.prettyPrintArgs)));
+          .handle(Event.info(null, "Running command line: " + runCommandLine.getPrettyArgs()));
     } else {
       env.getReporter()
-          .handle(
-              Event.info(
-                  null,
-                  "Runnable command line: "
-                      + ShellEscaper.escapeJoinAll(runCommandLine.prettyPrintArgs)));
+          .handle(Event.info(null, "Runnable command line: " + runCommandLine.getPrettyArgs()));
     }
 
     try {
@@ -328,11 +317,8 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
                   /* redactedArgv= */ options.getOptions(BuildEventProtocolOptions.class)
                           .includeResidueInRunBepEvent
                       ? ImmutableList.copyOf(execRequest.getArgvList())
-                      : transformArgvsForExecRequest(
-                          env,
-                          runCommandLine.argsWithoutResidue,
-                          builtTargets.configuration,
-                          builtTargets.stopTime)));
+                      : getArgvWithoutResidue(
+                          env, runCommandLine, builtTargets.configuration, builtTargets.stopTime)));
       return BlazeCommandResult.execute(execRequest.build());
     } catch (RunCommandException e) {
       return e.result;
@@ -512,8 +498,7 @@ private static BuiltTargets getBuiltTargets(
 
   private static ExecRequest.Builder execRequestBuilder(
       CommandEnvironment env,
-      Path workingDir,
-      ImmutableList<String> args,
+      RunCommandLine runCommandLine,
       ImmutableSortedMap<String, String> runEnv,
       ImmutableList<String> runEnvToClear,
       BuildConfigurationValue configuration,
@@ -523,8 +508,9 @@ private static ExecRequest.Builder execRequestBuilder(
       throws RunCommandException {
     ExecRequest.Builder execDescription =
         ExecRequest.newBuilder()
-            .setWorkingDirectory(ByteString.copyFrom(workingDir.getPathString(), ISO_8859_1));
-    execDescription.addAllArgv(transformArgvsForExecRequest(env, args, configuration, stopTime));
+            .setWorkingDirectory(
+                ByteString.copyFrom(runCommandLine.getWorkingDir().getPathString(), ISO_8859_1))
+            .addAllArgv(getArgvForExecRequest(env, runCommandLine, configuration, stopTime));
 
     for (Map.Entry<String, String> variable : runEnv.entrySet()) {
       execDescription.addEnvironmentVariable(
@@ -573,33 +559,40 @@ private static ImmutableList<PathToReplace> getPathsToReplace(
     return pathsToReplace.build();
   }
 
-  private static ImmutableList<ByteString> transformArgvsForExecRequest(
+  private static ImmutableList<ByteString> getArgvForExecRequest(
       CommandEnvironment env,
-      List<String> args,
+      RunCommandLine runCommandLine,
       BuildConfigurationValue configuration,
       long stopTime)
       throws RunCommandException {
-    ImmutableList.Builder<ByteString> execDescription = ImmutableList.builder();
-    if (OS.getCurrent() == OS.WINDOWS) {
-      boolean isBinary = true;
-      for (String arg : args) {
-        if (!isBinary) {
-          // All but the first element in `cmdLine` have to be escaped. The first element is the
-          // binary, which must not be escaped.
-          arg = ShellUtils.windowsEscapeArg(arg);
-        }
-        execDescription.add(ByteString.copyFrom(arg, ISO_8859_1));
-        isBinary = false;
-      }
-    } else {
-      String shExecutable =
-          getShellExecutableOrThrow(env, configuration, /* reason= */ "", stopTime);
-      String shellEscaped = ShellEscaper.escapeJoinAll(args);
-      for (String arg : ImmutableList.of(shExecutable, "-c", shellEscaped)) {
-        execDescription.add(ByteString.copyFrom(arg, ISO_8859_1));
-      }
+    return getArgv(env, runCommandLine, /* includeResidue= */ true, configuration, stopTime);
+  }
+
+  private static ImmutableList<ByteString> getArgvWithoutResidue(
+      CommandEnvironment env,
+      RunCommandLine runCommandLine,
+      BuildConfigurationValue configuration,
+      long stopTime)
+      throws RunCommandException {
+    return getArgv(env, runCommandLine, /* includeResidue= */ false, configuration, stopTime);
+  }
+
+  private static ImmutableList<ByteString> getArgv(
+      CommandEnvironment env,
+      RunCommandLine runCommandLine,
+      boolean includeResidue,
+      BuildConfigurationValue configuration,
+      long stopTime)
+      throws RunCommandException {
+    String shExecutable = null;
+    if (runCommandLine.requiresShExecutable()) {
+      shExecutable = getShellExecutableOrThrow(env, configuration, /* reason= */ "", stopTime);
     }
-    return execDescription.build();
+    ImmutableList<String> args =
+        includeResidue
+            ? runCommandLine.getArgs(shExecutable)
+            : runCommandLine.getArgsWithoutResidue(shExecutable);
+    return args.stream().map(s -> ByteString.copyFrom(s, ISO_8859_1)).collect(toImmutableList());
   }
 
   private BlazeCommandResult handleScriptPath(
@@ -620,9 +613,9 @@ private BlazeCommandResult handleScriptPath(
     String scriptContents =
         generateScriptContents(
             shExecutable,
-            runCommandLine.workingDir.getPathString(),
-            runCommandLine.runEnvironment,
-            runCommandLine.args);
+            runCommandLine.getWorkingDir().getPathString(),
+            runCommandLine.getEnvironment(),
+            runCommandLine.getScriptPathCommandLine());
     if (runOptions.emitScriptPathInExecRequest) {
       execRequest.setScriptPath(
           CommandProtos.ScriptPath.newBuilder()
@@ -641,32 +634,6 @@ private BlazeCommandResult handleScriptPath(
     }
   }
 
-  /** Encapsulates information for launching the command specified by a run invocation. */
-  private static class RunCommandLine {
-    private final ImmutableList<String> args;
-    private final ImmutableList<String> prettyPrintArgs;
-    private final ImmutableList<String> argsWithoutResidue;
-    private final ImmutableSortedMap<String, String> runEnvironment;
-    private final Path workingDir;
-
-    private final boolean isTestTarget;
-
-    private RunCommandLine(
-        ImmutableList<String> args,
-        ImmutableList<String> prettyPrintArgs,
-        ImmutableList<String> argsWithoutResidue,
-        ImmutableSortedMap<String, String> runEnvironment,
-        Path workingDir,
-        boolean isTestTarget) {
-      this.args = args;
-      this.prettyPrintArgs = prettyPrintArgs;
-      this.argsWithoutResidue = argsWithoutResidue;
-      this.runEnvironment = runEnvironment;
-      this.workingDir = workingDir;
-      this.isTestTarget = isTestTarget;
-    }
-  }
-
   private static RunCommandLine getCommandLineInfo(
       CommandEnvironment env,
       BuiltTargets builtTargets,
@@ -808,15 +775,11 @@ private static RunCommandLine getTestCommandLine(
           builtTargets.stopTime);
     }
 
-    ImmutableList<String> allArgs =
-        ImmutableList.<String>builder().addAll(testArgs).addAll(argsFromResidue).build();
-    return new RunCommandLine(
-        /* args= */ allArgs,
-        /* prettyPrintArgs= */ allArgs,
-        /* argsWithoutResidue= */ testArgs,
-        ImmutableSortedMap.copyOf(runEnvironment),
-        /* workingDir= */ env.getExecRoot(),
-        /* isTestTarget= */ true);
+    return new RunCommandLine.Builder(
+            ImmutableSortedMap.copyOf(runEnvironment), env.getExecRoot(), /* isTestTarget= */ true)
+        .addArgs(testArgs)
+        .addArgsFromResidue(argsFromResidue)
+        .build();
   }
 
   /**
@@ -837,110 +800,47 @@ private static RunCommandLine constructCommandLine(
       BuiltTargets builtTargets,
       ImmutableSortedMap<String, String> runEnvironment,
       ImmutableList<String> argsFromBinary,
-      ImmutableList<String> argsFromResidue)
-      throws RunCommandException {
-    PathFragment executablePath =
-        builtTargets
-            .targetToRun
-            .getProvider(FilesToRunProvider.class)
-            .getExecutable()
-            .getPath()
-            .asFragment();
-
+      ImmutableList<String> argsFromResidue) {
     BuildRequestOptions requestOptions = env.getOptions().getOptions(BuildRequestOptions.class);
     PathPrettyPrinter prettyPrinter =
         new PathPrettyPrinter(
             requestOptions.getSymlinkPrefix(env.getRuntime().getProductName()),
             builtTargets.convenienceSymlinks);
-    PathFragment prettyExecutablePath = prettyPrinter.getPrettyPath(executablePath);
 
-    ImmutableList.Builder<String> cmdLine = ImmutableList.builder();
-    ImmutableList.Builder<String> prettyCmdLine = ImmutableList.builder();
-    ImmutableList.Builder<String> redactedCmdLine = ImmutableList.builder();
+    RunCommandLine.Builder runCommandLine =
+        new RunCommandLine.Builder(
+            runEnvironment,
+            /* workingDir= */ builtTargets.targetToRunRunfilesDir != null
+                ? builtTargets.targetToRunRunfilesDir
+                : env.getWorkingDirectory(),
+            /* isTestTarget= */ false);
 
     RunUnder runUnder = env.getOptions().getOptions(CoreOptions.class).runUnder;
     // Insert the command prefix specified by the "--run_under=<command-prefix>" option
     // at the start of the command line.
     if (runUnder != null) {
-      String runUnderValue = runUnder.getValue();
-      String prettyRunUnderValue = runUnder.getValue();
       if (builtTargets.runUnderTarget != null) {
+        // --run_under specifies a target. Get the corresponding executable, this will be an
+        // absolute path because the run_under target is only in the runfiles of test targets
         Path runUnderPath =
             builtTargets
                 .runUnderTarget
                 .getProvider(FilesToRunProvider.class)
                 .getExecutable()
                 .getPath();
-        // --run_under specifies a target. Get the corresponding executable.
-        // This must be an absolute path, because the run_under target is only
-        // in the runfiles of test targets.
-        runUnderValue = runUnderPath.getPathString();
-        prettyRunUnderValue =
-            prettyPrinter.getPrettyPath(runUnderPath.asFragment()).getPathString();
-        // If the run_under command contains any options, make sure to add them
-        // to the command line as well.
-        List<String> opts = runUnder.getOptions();
-        if (!opts.isEmpty()) {
-          String escapedOpts = ShellEscaper.escapeJoinAll(opts);
-          runUnderValue += " " + escapedOpts;
-          prettyRunUnderValue += " " + escapedOpts;
-        }
+        runCommandLine.setRunUnderTarget(runUnderPath, runUnder.getOptions(), prettyPrinter);
+      } else {
+        runCommandLine.setRunUnderPrefix(runUnder.getValue());
       }
-
-      String shellExecutable =
-          getShellExecutableOrThrow(
-              env, builtTargets.configuration, "with \"--run_under\"", builtTargets.stopTime);
-
-      ImmutableList<String> allCommandLineArgs =
-          ImmutableList.<String>builder().addAll(argsFromBinary).addAll(argsFromResidue).build();
-      cmdLine
-          .add(shellExecutable)
-          .add("-c")
-          .add(
-              runUnderValue
-                  + " "
-                  + executablePath.getPathString()
-                  + " "
-                  + ShellEscaper.escapeJoinAll(allCommandLineArgs));
-
-      prettyCmdLine
-          .add(shellExecutable)
-          .add("-c")
-          .add(
-              prettyRunUnderValue
-                  + " "
-                  + prettyExecutablePath.getPathString()
-                  + " "
-                  + ShellEscaper.escapeJoinAll(allCommandLineArgs));
-
-      redactedCmdLine
-          .add(shellExecutable)
-          .add("-c")
-          .add(
-              runUnderValue
-                  + " "
-                  + executablePath.getPathString()
-                  + " "
-                  + ShellEscaper.escapeJoinAll(argsFromBinary));
-    } else {
-      cmdLine.add(executablePath.getPathString()).addAll(argsFromBinary).addAll(argsFromResidue);
-      prettyCmdLine
-          .add(prettyExecutablePath.getPathString())
-          .addAll(argsFromBinary)
-          .addAll(argsFromResidue);
-
-      redactedCmdLine.add(executablePath.getPathString()).addAll(argsFromBinary);
     }
 
-    return new RunCommandLine(
-        cmdLine.build(),
-        prettyCmdLine.build(),
-        redactedCmdLine.build(),
-        ImmutableSortedMap.copyOf(runEnvironment),
-        /* workingDir= */ builtTargets.targetToRunRunfilesDir != null
-            ? builtTargets.targetToRunRunfilesDir
-            : env.getWorkingDirectory(),
-        /* isTestTarget= */ false);
+    Artifact executable =
+        builtTargets.targetToRun.getProvider(FilesToRunProvider.class).getExecutable();
+    return runCommandLine
+        .addArg(executable.getPath(), prettyPrinter)
+        .addArgs(argsFromBinary)
+        .addArgsFromResidue(argsFromResidue)
+        .build();
   }
 
   private static String getShellExecutableOrThrow(
@@ -1279,7 +1179,7 @@ private FailureDetail createFailureDetail() {
   }
 
   private static String generateScriptContents(
-      String shExecutable, String workingDir, Map<String, String> environment, List<String> args) {
+      String shExecutable, String workingDir, Map<String, String> environment, String commandLine) {
     StringBuilder result = new StringBuilder();
     if (OS.getCurrent() == OS.WINDOWS) {
       result.append("@echo off\n");
@@ -1295,12 +1195,7 @@ private static String generateScriptContents(
     ScriptUtil.emitEnvPrefix(
         result, /* ignoreEnvironment= */ false, environment, ENV_VARIABLES_TO_CLEAR);
 
-    for (int i = 0; i < args.size(); i++) {
-      if (i != 0) {
-        result.append(" ");
-      }
-      ScriptUtil.emitCommandElement(result, args.get(i), i == 0);
-    }
+    result.append(commandLine);
 
     result.append(OS.getCurrent() == OS.WINDOWS ? " %*" : " \"$@\"");