subprocesses: customizable SubprocessFactory

    Individual SubprocessBuilder instances can now use
    a SubprocessFactory object other than the static
    SubprocessBuilder.factory.

    Also, WindowsSubprocessFactory is no longer a
    singleton because it now stores state: whether to
    use windows-style argument escaping or to use the
    (broken) Bash-style escaping (causing bazelbuild/bazel#7122).

    These two changes allow:
    - a safer way to mock out SubprocessFactory in
      tests, because it's no longer necessary to
      change global state (i.e. the static
      SubprocessBuilder.factory member)
    - testing old and new argument escaping semantics
      in WindowsSubprocessTest
    - adding a flag that switches between old and new
      semantics in the SubprocessFactory, and thus
      allows rolling out the bugfix with just a flag
      flip

    See bazelbuild/bazel#7122

    PiperOrigin-RevId: 234105692

dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

@@ -601,7 +601,7 @@ public Optional<Path> getServerLogPath() throws IOException {
       throw new IOException("Could not query server log location", e);
     }
 
-    return loggerFilePath.map((p) -> fileSystem.getPath(p.toAbsolutePath().toString()));
+    return loggerFilePath.map((p) -> fileSystem.getPath(p.toString()));
   }
 
   // Make sure we keep a strong reference to this logger, so that the

dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableMap;
 import java.io.File;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -88,8 +89,8 @@ public ImmutableList<String> getArgv() {
    * @throws IllegalArgumentException if argv is empty, or its first element (which becomes
    *     this.argv[0]) is neither an absolute path nor just a single file name
    */
-  public SubprocessBuilder setArgv(ImmutableList<String> argv) {
-    this.argv = Preconditions.checkNotNull(argv);
+  public SubprocessBuilder setArgv(Iterable<String> argv) {
+    this.argv = ImmutableList.copyOf(argv);
     Preconditions.checkArgument(!this.argv.isEmpty());
     File argv0 = new File(this.argv.get(0));
     Preconditions.checkArgument(
@@ -100,6 +101,11 @@ public SubprocessBuilder setArgv(ImmutableList<String> argv) {
     return this;
   }
 
+  public SubprocessBuilder setArgv(String... argv) {
+    this.setArgv(Arrays.asList(argv));
+    return this;
+  }
+
   public ImmutableMap<String, String> getEnv() {
     return env;
   }

dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java

@@ -14,48 +14,97 @@
 
 package com.google.devtools.build.lib.windows;
 
-import com.google.common.base.Charsets;
-import com.google.common.collect.Ordering;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
-
+import com.google.devtools.build.lib.shell.SubprocessFactory;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.windows.jni.WindowsProcesses;
 import java.io.File;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
 import java.util.Map;
+import java.util.TreeMap;
 
 /**
  * A subprocess factory that uses the Win32 API.
  */
-public class WindowsSubprocessFactory implements Subprocess.Factory {
-  public static final WindowsSubprocessFactory INSTANCE = new WindowsSubprocessFactory();
+public class WindowsSubprocessFactory implements SubprocessFactory {
+  private final boolean windowsStyleArgEscaping;
 
-  private WindowsSubprocessFactory() {
-    // Singleton
+  public WindowsSubprocessFactory(boolean windowsStyleArgEscaping) {
+    this.windowsStyleArgEscaping = windowsStyleArgEscaping;
   }
 
   @Override
   public Subprocess create(SubprocessBuilder builder) throws IOException {
-    WindowsJniLoader.loadJni();
-
-    String commandLine = WindowsProcesses.quoteCommandLine(builder.getArgv());
-    byte[] env = builder.getEnv() == null ? null : convertEnvToNative(builder.getEnv());
+    List<String> argv = builder.getArgv();
+
+    // DO NOT quote argv0, createProcess will do it for us.
+    String argv0 = processArgv0(argv.get(0));
+    String argvRest = argv.size() > 1 ? escapeArgvRest(argv.subList(1, argv.size())) : "";
+    byte[] env = convertEnvToNative(builder.getEnv());
 
     String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile());
     String stderrPath = getRedirectPath(builder.getStderr(), builder.getStderrFile());
 
-    long nativeProcess = WindowsProcesses.nativeCreateProcess(
-        commandLine, env, builder.getWorkingDirectory().getPath(), stdoutPath, stderrPath);
-    String error = WindowsProcesses.nativeGetLastError(nativeProcess);
+    long nativeProcess =
+        WindowsProcesses.createProcess(
+            argv0,
+            argvRest,
+            env,
+            builder.getWorkingDirectory().getPath(),
+            stdoutPath,
+            stderrPath,
+            builder.redirectErrorStream());
+    String error = WindowsProcesses.processGetLastError(nativeProcess);
     if (!error.isEmpty()) {
-      WindowsProcesses.nativeDelete(nativeProcess);
+      WindowsProcesses.deleteProcess(nativeProcess);
       throw new IOException(error);
     }
 
-    return new WindowsSubprocess(nativeProcess, stdoutPath != null, stderrPath != null);
+    return new WindowsSubprocess(
+        nativeProcess,
+        argv0 + " " + argvRest,
+        stdoutPath != null,
+        stderrPath != null,
+        builder.getTimeoutMillis());
   }
 
-  private String getRedirectPath(StreamAction action, File file) {
+  private String escapeArgvRest(List<String> argv) {
+    if (windowsStyleArgEscaping) {
+      StringBuilder result = new StringBuilder();
+      boolean first = true;
+      for (String arg : argv) {
+        if (first) {
+          first = false;
+        } else {
+          result.append(" ");
+        }
+        result.append(ShellUtils.windowsEscapeArg(arg));
+      }
+      return result.toString();
+    } else {
+      return WindowsProcesses.quoteCommandLine(argv);
+    }
+  }
+
+  public static String processArgv0(String argv0) {
+    // Normalize the path and make it Windows-style.
+    // If argv0 is at least MAX_PATH (260 chars) long, createNativeProcess calls GetShortPathNameW
+    // to obtain a 8dot3 name for it (thereby support long paths in CreateProcessA), but then argv0
+    // must be prefixed with "\\?\" for GetShortPathNameW to work, so it also must be an absolute,
+    // normalized, Windows-style path.
+    // Therefore if it's absolute, then normalize it also.
+    // If it's not absolute, then it cannot be longer than MAX_PATH, since MAX_PATH also limits the
+    // length of file names.
+    PathFragment argv0fragment = PathFragment.create(argv0);
+    return argv0fragment.isAbsolute() ? argv0fragment.getPathString().replace('/', '\\') : argv0;
+  }
+
+  private static String getRedirectPath(StreamAction action, File file) {
     switch (action) {
       case DISCARD:
         return "NUL";  // That's /dev/null on Windows
@@ -71,26 +120,48 @@ private String getRedirectPath(StreamAction action, File file) {
     }
   }
 
-  /**
-   * Converts an environment map to the format expected in lpEnvironment by CreateProcess().
-   */
-  private byte[] convertEnvToNative(Map<String, String> env) throws IOException {
-    if (env.isEmpty()) {
+  /** Converts an environment map to the format expected in lpEnvironment by CreateProcess(). */
+  private static byte[] convertEnvToNative(Map<String, String> envMap) throws IOException {
+    Map<String, String> realEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+    Map<String, String> systemEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+    if (envMap != null) {
+      realEnv.putAll(envMap);
+    }
+    // It is fine to use System.getenv to get default SYSTEMROOT and SYSTEMDRIVE, because they are
+    // very special system environment variables and Bazel's client and server are running on the
+    // same machine, so it should be the same in client environment.
+    systemEnv.putAll(System.getenv());
+    // Some versions of MSVCRT.DLL and tools require SYSTEMROOT and SYSTEMDRIVE to be set. They are
+    // very common environment variables on Windows, so we add these environment variables
+    // regardless of whether the caller requested it or not.
+    String[] systemEnvironmentVars = {"SYSTEMROOT", "SYSTEMDRIVE"};
+    for (String env : systemEnvironmentVars) {
+      if (realEnv.getOrDefault(env, null) == null) {
+        String value = systemEnv.getOrDefault(env, null);
+        if (value != null) {
+          realEnv.put(env, value);
+        }
+      }
+    }
+
+    if (realEnv.isEmpty()) {
       // Special case: CreateProcess() always expects the environment block to be terminated
       // with two zeros.
-      return new byte[] { 0, 0, };
+      return "\0".getBytes(StandardCharsets.UTF_16LE);
     }
 
     StringBuilder result = new StringBuilder();
-
-    for (String key : Ordering.natural().sortedCopy(env.keySet())) {
-      if (key.contains("=")) {
-        throw new IOException("Environment variable names must not contain '='");
+    for (Map.Entry<String, String> entry : realEnv.entrySet()) {
+      if (entry.getKey().contains("=")) {
+        // lpEnvironment requires no '=' in environment variable name, but on Windows,
+        // System.getenv() returns environment variables like '=C:' or '=ExitCode', so it can't
+        // be an error, we have ignore them here.
+        continue;
       }
-      result.append(key + "=" + env.get(key) + "\0");
+      result.append(entry.getKey() + "=" + entry.getValue() + "\0");
     }
 
     result.append("\0");
-    return result.toString().getBytes(Charsets.UTF_8);
+    return result.toString().getBytes(StandardCharsets.UTF_16LE);
   }
 }

dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java

@@ -39,7 +39,6 @@
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
-import com.google.devtools.build.lib.actions.cache.MetadataInjector;
 import com.google.devtools.build.lib.exec.BinTools;
 import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
@@ -259,11 +258,6 @@ public SortedMap<PathFragment, ActionInput> getInputMapping(
     public void report(ProgressStatus state, String name) {
       reportedStatus.add(state);
     }
-
-    @Override
-    public MetadataInjector getMetadataInjector() {
-      throw new UnsupportedOperationException();
-    }
   }
 
   private final MetadataProvider mockFileCache = mock(MetadataProvider.class);