Rollback #5007

It breaks downstream rules_nodejs. See #5028 for details.

RELNOTES: None.
PiperOrigin-RevId: 193074798

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

@@ -109,12 +109,14 @@ static class BazelWorkspaceStatusAction extends WorkspaceStatusAction {
       this.getWorkspaceStatusCommand =
           options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT)
               ? null
-              : new CommandBuilder(workspace)
-                  .addArg(options.workspaceStatusCommand.toString())
+              : new CommandBuilder()
+                  .addArgs(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,6 +17,7 @@
 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;
@@ -36,7 +37,6 @@
 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,7 +90,10 @@ public void createSymlinksUsingCommand(
       Path execRoot, BuildConfiguration config, BinTools binTools)
           throws CommandException {
     List<String> argv = getSpawnArgumentList(execRoot, binTools.getExecPath(BUILD_RUNFILES));
-    new CommandBuilder(execRoot).addArgs(argv).build().execute();
+    CommandBuilder builder = new CommandBuilder();
+    builder.addArgs(argv);
+    builder.setWorkingDir(execRoot);
+    builder.build().execute();
   }
 
   /**
@@ -145,7 +148,7 @@ Spawn createSpawn(
     ActionInput buildRunfiles = binTools.getActionInput(BUILD_RUNFILES);
     return new SimpleSpawn(
         owner,
-        ImmutableList.copyOf(getSpawnArgumentList(execRoot, buildRunfiles.getExecPath())),
+        getSpawnArgumentList(execRoot, buildRunfiles.getExecPath()),
         environment,
         ImmutableMap.of(
             ExecutionRequirements.LOCAL, "",
@@ -156,9 +159,11 @@ Spawn createSpawn(
         RESOURCE_SET);
   }
 
-  /** 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);
+  /**
+   * 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();
     args.add(buildRunfiles.getPathString());
 
     if (filesetTree) {
@@ -169,6 +174,6 @@ private ArrayList<String> getSpawnArgumentList(Path execRoot, PathFragment build
     args.add(inputManifest.relativeTo(execRoot).getPathString());
     args.add(symlinkTreeRoot.relativeTo(execRoot).getPathString());
 
-    return args;
+    return ImmutableList.copyOf(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(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)
+    new CommandBuilder()
+        .addArg(command)
+        .useShell(true)
+        .setWorkingDir(tempPath.getParentDirectory())
         .build()
         .execute();
   }

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

@@ -246,8 +246,8 @@ private BlazeCommandResult runTargetUnderServer(CommandEnvironment env,
         null, "Running command line: " + ShellEscaper.escapeJoinAll(prettyCmdLine)));
 
     try {
-      com.google.devtools.build.lib.shell.Command command =
-          new CommandBuilder(workingDir).addArgs(cmdLine).setEnv(env.getClientEnv()).build();
+      com.google.devtools.build.lib.shell.Command command = new CommandBuilder()
+          .addArgs(cmdLine).setEnv(env.getClientEnv()).setWorkingDir(workingDir).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,7 +285,11 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsProvider options)
 
     Path workingDir = env.getBlazeWorkspace().getOutputPath().getParentDirectory();
     com.google.devtools.build.lib.shell.Command command =
-        new CommandBuilder(workingDir).addArgs(cmdLine).setEnv(env.getClientEnv()).build();
+        new CommandBuilder()
+            .addArgs(cmdLine)
+            .setEnv(env.getClientEnv())
+            .setWorkingDir(workingDir)
+            .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,44 +14,65 @@
 
 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 {@link Command} builder.
+ * 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.
  *
- * <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 final ArrayList<String> argv = new ArrayList<>();
-  private final HashMap<String, String> env = new HashMap<>();
-  private final File workingDir;
+  private static final ImmutableList<String> SHELLS = ImmutableList.of("/bin/sh", "/bin/bash");
+
+  private static final Splitter ARGV_SPLITTER = Splitter.on(CharMatcher.anyOf(" \t"));
 
-  public CommandBuilder(Path workingDir) {
-    this(workingDir.getPathFile());
+  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(File workingDir) {
-    this.workingDir = Preconditions.checkNotNull(workingDir);
+  @VisibleForTesting
+  CommandBuilder(OS system) {
+    this.system = system;
   }
 
   public CommandBuilder addArg(String arg) {
-    Preconditions.checkNotNull(arg);
+    Preconditions.checkNotNull(arg, "Argument must not be null");
     argv.add(arg);
     return this;
   }
 
   public CommandBuilder addArgs(Iterable<String> args) {
-    Preconditions.checkArgument(!Iterables.contains(args, null));
+    Preconditions.checkArgument(!Iterables.contains(args, null), "Arguments must not be null");
     Iterables.addAll(argv, args);
     return this;
   }
@@ -77,8 +98,82 @@ 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(argv.toArray(new String[0]), env, workingDir);
+
+    return new Command(
+        system == OS.WINDOWS ? transformArgvForWindows() : transformArgvForLinux(),
+        env, workingDir);
   }
 }

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

@@ -17,7 +17,6 @@
 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;
@@ -29,8 +28,12 @@
 @RunWith(JUnit4.class)
 public class CommandBuilderTest {
 
-  private CommandBuilder builder() {
-    return new CommandBuilder(new File("dummy-workdir"));
+  private CommandBuilder linuxBuilder() {
+    return new CommandBuilder(OS.LINUX).useTempDir();
+  }
+
+  private CommandBuilder winBuilder() {
+    return new CommandBuilder(OS.WINDOWS).useTempDir();
   }
 
   private void assertArgv(CommandBuilder builder, String... expected) {
@@ -40,6 +43,10 @@ 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();
@@ -50,18 +57,47 @@ private void assertFailure(CommandBuilder builder, String expected) {
   }
 
   @Test
-  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");
+  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), "");
   }
 
   @Test
   public void failureScenarios() {
-    assertFailure(builder(), "At least one argument is expected");
+    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");
   }
+
 }

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 "Cannot run program.*/wscmissing.sh.* No such file or directory"
+  expect_log "wscmissing.sh: No such file or directory\|wscmissing.sh: not found"
 }