CommandBuilder: remove useShell and setWorkingDir

Remove the .useShell method, expect callers to
just pass the shell interpreter if they need it.
This removes the argument vector transformation
heuristic, and stops shelling out to cmd.exe on
Windows.

Also remove the .setWorkingDir method because
callers always had to set the working directory.
Instead, the CommandBuilder constructor takes the
working directory.

Closes #2190. (Test code still shells out to
cmd.exe though.)

Closes #5007.

Change-Id: I545e01c811daaf34913cb585492923da81aa02ee
PiperOrigin-RevId: 192611370

6 files changed, 29 insertions, 135 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index 2434bff695..ef7ecdf4cb 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -109,14 +109,12 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
       this.getWorkspaceStatusCommand =
           options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT)
               ? null
-              : new CommandBuilder()
-                  .addArgs(options.workspaceStatusCommand.toString())
+              : new CommandBuilder(workspace)
+                  .addArg(options.workspaceStatusCommand.toString())
                   // Pass client env, because certain SCM client(like
                   // perforce, git) relies on environment variables to work
                   // correctly.
                   .setEnv(clientEnv)
-                  .setWorkingDir(workspace)
-                  .useShell(true)
                   .build();
       this.workspace = workspace;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index 5f231e4fd0..2eaaf499c5 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -17,7 +17,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
@@ -37,6 +36,7 @@ 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 java.io.IOException;
+import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -90,10 +90,7 @@ public final class SymlinkTreeHelper {
       Path execRoot, BuildConfiguration config, BinTools binTools)
           throws CommandException {
     List<String> argv = getSpawnArgumentList(execRoot, binTools.getExecPath(BUILD_RUNFILES));
-    CommandBuilder builder = new CommandBuilder();
-    builder.addArgs(argv);
-    builder.setWorkingDir(execRoot);
-    builder.build().execute();
+    new CommandBuilder(execRoot).addArgs(argv).build().execute();
   }
 
   /**
@@ -148,7 +145,7 @@ public final class SymlinkTreeHelper {
     ActionInput buildRunfiles = binTools.getActionInput(BUILD_RUNFILES);
     return new SimpleSpawn(
         owner,
-        getSpawnArgumentList(execRoot, buildRunfiles.getExecPath()),
+        ImmutableList.copyOf(getSpawnArgumentList(execRoot, buildRunfiles.getExecPath())),
         environment,
         ImmutableMap.of(
             ExecutionRequirements.LOCAL, "",
@@ -159,11 +156,9 @@ public final class SymlinkTreeHelper {
         RESOURCE_SET);
   }
 
-  /**
-   * Returns the complete argument list build-runfiles has to be called with.
-   */
-  private ImmutableList<String> getSpawnArgumentList(Path execRoot, PathFragment buildRunfiles) {
-    List<String> args = Lists.newArrayList();
+  /** Returns the complete argument list build-runfiles has to be called with. */
+  private ArrayList<String> getSpawnArgumentList(Path execRoot, PathFragment buildRunfiles) {
+    ArrayList<String> args = new ArrayList<>(filesetTree ? 5 : 3);
     args.add(buildRunfiles.getPathString());
 
     if (filesetTree) {
@@ -174,6 +169,6 @@ public final class SymlinkTreeHelper {
     args.add(inputManifest.relativeTo(execRoot).getPathString());
     args.add(symlinkTreeRoot.relativeTo(execRoot).getPathString());
 
-    return ImmutableList.copyOf(args);
+    return args;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
index d848d47469..c9c91f3f48 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
@@ -194,10 +194,10 @@ public final class CleanCommand implements BlazeCommand {
     logger.info("Executing shell commmand " + ShellEscaper.escapeString(command));
 
     // Doesn't throw iff command exited and was successful.
-    new CommandBuilder()
-        .addArg(command)
-        .useShell(true)
-        .setWorkingDir(tempPath.getParentDirectory())
+    new CommandBuilder(tempPath.getParentDirectory())
+        // TODO(laszlocsomor): implement the async directory tree deleter as a native, embedded
+        // binary, and stop relying on the shell. See GitHub issue #4319.
+        .addArgs("/bin/sh", "-c", command)
         .build()
         .execute();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 8aa3566041..adcae97ff5 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -237,8 +237,8 @@ public class RunCommand implements BlazeCommand  {
         null, "Running command line: " + ShellEscaper.escapeJoinAll(prettyCmdLine)));
 
     try {
-      com.google.devtools.build.lib.shell.Command command = new CommandBuilder()
-          .addArgs(cmdLine).setEnv(env.getClientEnv()).setWorkingDir(workingDir).build();
+      com.google.devtools.build.lib.shell.Command command =
+          new CommandBuilder(workingDir).addArgs(cmdLine).setEnv(env.getClientEnv()).build();
 
       // Restore a raw EventHandler if it is registered. This allows for blaze run to produce the
       // actual output of the command being run even if --color=no is specified.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java
index 6484bbe3f2..0d43d8f95a 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java
@@ -285,11 +285,7 @@ public class MobileInstallCommand implements BlazeCommand {
 
     Path workingDir = env.getBlazeWorkspace().getOutputPath().getParentDirectory();
     com.google.devtools.build.lib.shell.Command command =
-        new CommandBuilder()
-            .addArgs(cmdLine)
-            .setEnv(env.getClientEnv())
-            .setWorkingDir(workingDir)
-            .build();
+        new CommandBuilder(workingDir).addArgs(cmdLine).setEnv(env.getClientEnv()).build();
 
     try {
       // Restore a raw EventHandler if it is registered. This allows for blaze run to produce the
diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java b/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java
index d60fd94dc4..9bff531521 100644
--- a/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java
@@ -14,65 +14,44 @@
 
 package com.google.devtools.build.lib.util;
 
-import static com.google.common.base.StandardSystemProperty.JAVA_IO_TMPDIR;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.CharMatcher;
-import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Splitter;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.File;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 
 /**
- * Implements OS aware {@link Command} builder. At this point only Linux, Mac
- * and Windows XP are supported.
- *
- * <p>Builder will also apply heuristic to identify trivial cases where
- * unix-like command lines could be automatically converted into the
- * Windows-compatible form.
+ * Implements {@link Command} builder.
  *
- * <p>TODO(bazel-team): (2010) Some of the code here is very similar to the
- * {@link com.google.devtools.build.lib.shell.Shell} class. This should be looked at.
+ * <p>TODO(bazel-team): (2010) Some of the code here is very similar to the {@link
+ * com.google.devtools.build.lib.shell.Shell} class. This should be looked at.
  */
 public final class CommandBuilder {
 
-  private static final ImmutableList<String> SHELLS = ImmutableList.of("/bin/sh", "/bin/bash");
-
-  private static final Splitter ARGV_SPLITTER = Splitter.on(CharMatcher.anyOf(" \t"));
+  private final ArrayList<String> argv = new ArrayList<>();
+  private final HashMap<String, String> env = new HashMap<>();
+  private final File workingDir;
 
-  private final OS system;
-  private final List<String> argv = new ArrayList<>();
-  private final Map<String, String> env = new HashMap<>();
-  private File workingDir = null;
-  private boolean useShell = false;
-
-  public CommandBuilder() {
-    this(OS.getCurrent());
+  public CommandBuilder(Path workingDir) {
+    this(workingDir.getPathFile());
   }
 
-  @VisibleForTesting
-  CommandBuilder(OS system) {
-    this.system = system;
+  public CommandBuilder(File workingDir) {
+    this.workingDir = Preconditions.checkNotNull(workingDir);
   }
 
   public CommandBuilder addArg(String arg) {
-    Preconditions.checkNotNull(arg, "Argument must not be null");
+    Preconditions.checkNotNull(arg);
     argv.add(arg);
     return this;
   }
 
   public CommandBuilder addArgs(Iterable<String> args) {
-    Preconditions.checkArgument(!Iterables.contains(args, null), "Arguments must not be null");
+    Preconditions.checkArgument(!Iterables.contains(args, null));
     Iterables.addAll(argv, args);
     return this;
   }
@@ -98,82 +77,8 @@ public final class CommandBuilder {
     return this;
   }
 
-  public CommandBuilder setWorkingDir(Path path) {
-    Preconditions.checkNotNull(path);
-    workingDir = path.getPathFile();
-    return this;
-  }
-
-  public CommandBuilder useTempDir() {
-    workingDir = new File(JAVA_IO_TMPDIR.value());
-    return this;
-  }
-
-  public CommandBuilder useShell(boolean useShell) {
-    this.useShell = useShell;
-    return this;
-  }
-
-  private boolean argvStartsWithSh() {
-    return argv.size() >= 2 && SHELLS.contains(argv.get(0)) && "-c".equals(argv.get(1));
-  }
-
-  private String[] transformArgvForLinux() {
-    // If command line already starts with "/bin/sh -c", ignore useShell attribute.
-    if (useShell && !argvStartsWithSh()) {
-      // c.g.io.base.shell.Shell.shellify() actually concatenates argv into the space-separated
-      // string here. Not sure why, but we will do the same.
-      return new String[] { "/bin/sh", "-c", Joiner.on(' ').join(argv) };
-    }
-    return argv.toArray(new String[argv.size()]);
-  }
-
-  private String[] transformArgvForWindows() {
-    List<String> modifiedArgv;
-    // Heuristic: replace "/bin/sh -c" with something more appropriate for Windows.
-    if (argvStartsWithSh()) {
-      useShell = true;
-      modifiedArgv = Lists.newArrayList(argv.subList(2, argv.size()));
-    } else {
-      modifiedArgv = Lists.newArrayList(argv);
-    }
-
-    if (!modifiedArgv.isEmpty()) {
-      // args can contain whitespace, so figure out the first word
-      String argv0 = modifiedArgv.get(0);
-      String command = ARGV_SPLITTER.split(argv0).iterator().next();
-
-      // Automatically enable CMD.EXE use if we are executing something else besides "*.exe" file.
-      // When use CMD.EXE to invoke a bat/cmd file, the file path must have '\' instead of '/'
-      if (!command.toLowerCase().endsWith(".exe")) {
-        useShell = true;
-        modifiedArgv.set(0, argv0.replace('/', '\\'));
-      }
-    } else {
-      // This is degenerate "/bin/sh -c" case. We ensure that Windows behavior is identical
-      // to the Linux - call shell that will do nothing.
-      useShell = true;
-    }
-    if (useShell) {
-      // /S - strip first and last quotes and execute everything else as is.
-      // /E:ON - enable extended command set.
-      // /V:ON - enable delayed variable expansion
-      // /D - ignore AutoRun registry entries.
-      // /C - execute command. This must be the last option before the command itself.
-      return new String[] { "CMD.EXE", "/S", "/E:ON", "/V:ON", "/D", "/C",
-          Joiner.on(' ').join(modifiedArgv) };
-    } else {
-      return modifiedArgv.toArray(new String[argv.size()]);
-    }
-  }
-
   public Command build() {
-    Preconditions.checkState(system != OS.UNKNOWN, "Unidentified operating system");
-    Preconditions.checkNotNull(workingDir, "Working directory must be set");
     Preconditions.checkState(!argv.isEmpty(), "At least one argument is expected");
-
-    return new Command(
-        system == OS.WINDOWS ? transformArgvForWindows() : transformArgvForLinux(),
-        env, workingDir);
+    return new Command(argv.toArray(new String[0]), env, workingDir);
   }
 }