CommandBuilder: remove useShell and setWorkingDir

src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java

@@ -109,14 +109,12 @@ static class BazelWorkspaceStatusAction extends WorkspaceStatusAction {
       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;
     }

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java

@@ -17,7 +17,6 @@
 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.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 void createSymlinksUsingCommand(
       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 @@ Spawn createSpawn(
     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, "",
@@ -162,8 +159,8 @@ Spawn createSpawn(
   /**
    * 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();
+  private ArrayList<String> getSpawnArgumentList(Path execRoot, PathFragment buildRunfiles) {
+    ArrayList<String> args = new ArrayList<>(filesetTree ? 5 : 3);
     args.add(buildRunfiles.getPathString());
 
     if (filesetTree) {
@@ -174,6 +171,6 @@ private ImmutableList<String> getSpawnArgumentList(Path execRoot, PathFragment b
     args.add(inputManifest.relativeTo(execRoot).getPathString());
     args.add(symlinkTreeRoot.relativeTo(execRoot).getPathString());
 
-    return ImmutableList.copyOf(args);
+    return args;
   }
 }

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

@@ -194,10 +194,10 @@ private static void asyncClean(CommandEnvironment env, Path path, String pathIte
     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();
   }

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

@@ -237,8 +237,8 @@ private BlazeCommandResult runTargetUnderServer(CommandEnvironment env,
         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.

src/main/java/com/google/devtools/build/lib/runtime/mobileinstall/MobileInstallCommand.java

@@ -285,11 +285,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsProvider options)
 
     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

src/main/java/com/google/devtools/build/lib/util/CommandBuilder.java

@@ -14,14 +14,8 @@
 
 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;
@@ -30,49 +24,36 @@
 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.
  */
 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 +79,8 @@ public CommandBuilder setEnv(Map<String, String> env) {
     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);
   }
 }

src/test/java/com/google/devtools/build/lib/util/CommandBuilderTest.java

@@ -17,6 +17,7 @@
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
+import java.io.File;
 import java.util.Arrays;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -28,12 +29,8 @@
 @RunWith(JUnit4.class)
 public class CommandBuilderTest {
 
-  private CommandBuilder linuxBuilder() {
-    return new CommandBuilder(OS.LINUX).useTempDir();
-  }
-
-  private CommandBuilder winBuilder() {
-    return new CommandBuilder(OS.WINDOWS).useTempDir();
+  private CommandBuilder builder() {
+    return new CommandBuilder(new File("dummy-workdir"));
   }
 
   private void assertArgv(CommandBuilder builder, String... expected) {
@@ -43,10 +40,6 @@ private void assertArgv(CommandBuilder builder, String... expected) {
         .inOrder();
   }
 
-  private void assertWinCmdArgv(CommandBuilder builder, String expected) {
-    assertArgv(builder, "CMD.EXE", "/S", "/E:ON", "/V:ON", "/D", "/C", expected);
-  }
-
   private void assertFailure(CommandBuilder builder, String expected) {
     try {
       builder.build();
@@ -57,47 +50,18 @@ private void assertFailure(CommandBuilder builder, String expected) {
   }
 
   @Test
-  public void linuxBuilderTest() {
-    assertArgv(linuxBuilder().addArg("abc"), "abc");
-    assertArgv(linuxBuilder().addArg("abc def"), "abc def");
-    assertArgv(linuxBuilder().addArgs("abc", "def"), "abc", "def");
-    assertArgv(linuxBuilder().addArgs(ImmutableList.of("abc", "def")), "abc", "def");
-    assertArgv(linuxBuilder().addArg("abc").useShell(true), "/bin/sh", "-c", "abc");
-    assertArgv(linuxBuilder().addArg("abc def").useShell(true), "/bin/sh", "-c", "abc def");
-    assertArgv(linuxBuilder().addArgs("abc", "def").useShell(true), "/bin/sh", "-c", "abc def");
-    assertArgv(linuxBuilder().addArgs("/bin/sh", "-c", "abc").useShell(true),
-        "/bin/sh", "-c", "abc");
-    assertArgv(linuxBuilder().addArgs("/bin/sh", "-c"), "/bin/sh", "-c");
-    assertArgv(linuxBuilder().addArgs("/bin/bash", "-c"), "/bin/bash", "-c");
-    assertArgv(linuxBuilder().addArgs("/bin/sh", "-c").useShell(true), "/bin/sh", "-c");
-    assertArgv(linuxBuilder().addArgs("/bin/bash", "-c").useShell(true), "/bin/bash", "-c");
-  }
-
-  @Test
-  public void windowsBuilderTest() {
-    assertArgv(winBuilder().addArg("abc.exe"), "abc.exe");
-    assertArgv(winBuilder().addArg("abc.exe -o"), "abc.exe -o");
-    assertArgv(winBuilder().addArg("ABC.EXE"), "ABC.EXE");
-    assertWinCmdArgv(winBuilder().addArg("abc def.exe"), "abc def.exe");
-    assertArgv(winBuilder().addArgs("abc.exe", "def"), "abc.exe", "def");
-    assertArgv(winBuilder().addArgs(ImmutableList.of("abc.exe", "def")), "abc.exe", "def");
-    assertWinCmdArgv(winBuilder().addArgs("abc.exe", "def").useShell(true), "abc.exe def");
-    assertWinCmdArgv(winBuilder().addArg("abc"), "abc");
-    assertWinCmdArgv(winBuilder().addArgs("abc", "def"), "abc def");
-    assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c", "abc", "def"), "abc def");
-    assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c"), "");
-    assertWinCmdArgv(winBuilder().addArgs("/bin/bash", "-c"), "");
-    assertWinCmdArgv(winBuilder().addArgs("/bin/sh", "-c").useShell(true), "");
-    assertWinCmdArgv(winBuilder().addArgs("/bin/bash", "-c").useShell(true), "");
+  public void builderTest() {
+    assertArgv(builder().addArg("abc"), "abc");
+    assertArgv(builder().addArg("abc def"), "abc def");
+    assertArgv(builder().addArgs("abc", "def"), "abc", "def");
+    assertArgv(builder().addArgs(ImmutableList.of("abc", "def")), "abc", "def");
+    assertArgv(builder().addArgs("/bin/sh", "-c", "abc"), "/bin/sh", "-c", "abc");
+    assertArgv(builder().addArgs("/bin/sh", "-c"), "/bin/sh", "-c");
+    assertArgv(builder().addArgs("/bin/bash", "-c"), "/bin/bash", "-c");
   }
 
   @Test
   public void failureScenarios() {
-    assertFailure(linuxBuilder(), "At least one argument is expected");
-    assertFailure(new CommandBuilder(OS.UNKNOWN).useTempDir().addArg("a"),
-        "Unidentified operating system");
-    assertFailure(new CommandBuilder(OS.LINUX).addArg("a"),
-        "Working directory must be set");
+    assertFailure(builder(), "At least one argument is expected");
   }
-
 }

src/test/shell/bazel/bazel_workspace_status_test.sh

@@ -129,7 +129,7 @@ EOF
 
   bazel build --workspace_status_command=$TEST_TMPDIR/wscmissing.sh --stamp //:a &> $TEST_log \
     && fail "build succeeded"
-  expect_log "wscmissing.sh: No such file or directory\|wscmissing.sh: not found"
+  expect_log "Cannot run program.*/wscmissing.sh.* No such file or directory"
 }