Windows: remove incompatible_windows_style_arg_escaping

In this PR: - Removed the `--incompatible_windows_style_arg_escaping` flag and all code for its "false" case. The flag is flipped to true. - In WindowsProcessesTest, the testDoesNotQuoteArgWithDoubleQuote method is replaced by testQuotesArgWithDoubleQuote, i.e. the test logic is reversed. The new test shows the correct behavior, the old test was wrong. See bazelbuild#7122 See bazelbuild#7454 RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag. Closes bazelbuild#8003. PiperOrigin-RevId: 255929367
siberex · Jul 4, 2019 · 08b1204 · 08b1204
1 parent 05a1349
commit 08b1204
Show file tree

Hide file tree

Showing 11 changed files with 56 additions and 267 deletions.
diff --git a/src/main/cpp/bazel_startup_options.cc b/src/main/cpp/bazel_startup_options.cc
@@ -28,10 +28,8 @@ BazelStartupOptions::BazelStartupOptions(
       use_system_rc(true),
       use_workspace_rc(true),
       use_home_rc(true),
-      use_master_bazelrc_(true),
-      incompatible_windows_style_arg_escaping(true) {
+      use_master_bazelrc_(true) {
   RegisterNullaryStartupFlag("home_rc");
-  RegisterNullaryStartupFlag("incompatible_windows_style_arg_escaping");
   RegisterNullaryStartupFlag("master_bazelrc");
   RegisterNullaryStartupFlag("system_rc");
   RegisterNullaryStartupFlag("workspace_rc");
@@ -106,14 +104,6 @@ blaze_exit_code::ExitCode BazelStartupOptions::ProcessArgExtra(
     }
     use_master_bazelrc_ = false;
     option_sources["blazerc"] = rcfile;
-  } else if (GetNullaryOption(arg,
-                              "--incompatible_windows_style_arg_escaping")) {
-    incompatible_windows_style_arg_escaping = true;
-    option_sources["incompatible_windows_style_arg_escaping"] = rcfile;
-  } else if (GetNullaryOption(arg,
-                              "--noincompatible_windows_style_arg_escaping")) {
-    incompatible_windows_style_arg_escaping = false;
-    option_sources["incompatible_windows_style_arg_escaping"] = rcfile;
   } else {
     *is_processed = false;
     return blaze_exit_code::SUCCESS;
@@ -147,13 +137,4 @@ void BazelStartupOptions::MaybeLogStartupOptionWarnings() const {
   }
 }
 
-void BazelStartupOptions::AddExtraOptions(
-    std::vector<std::string> *result) const {
-  if (incompatible_windows_style_arg_escaping) {
-    result->push_back("--incompatible_windows_style_arg_escaping");
-  } else {
-    result->push_back("--noincompatible_windows_style_arg_escaping");
-  }
-}
-
 }  // namespace blaze
diff --git a/src/main/cpp/bazel_startup_options.h b/src/main/cpp/bazel_startup_options.h
@@ -26,8 +26,6 @@ class BazelStartupOptions : public StartupOptions {
  public:
   explicit BazelStartupOptions(const WorkspaceLayout *workspace_layout);
 
-  void AddExtraOptions(std::vector<std::string> *result) const override;
-
   blaze_exit_code::ExitCode ProcessArgExtra(
       const char *arg, const char *next_arg, const std::string &rcfile,
       const char **value, bool *is_processed, std::string *error) override;
@@ -41,13 +39,6 @@ class BazelStartupOptions : public StartupOptions {
   bool use_home_rc;
   // TODO(b/36168162): Remove the master rc flag.
   bool use_master_bazelrc_;
-
-  // Whether Windows-style subprocess argument escaping is enabled on Windows,
-  // or the (buggy) Bash-style is used.
-  // This flag only affects builds on Windows, and it's a no-op on other
-  // platforms.
-  // See https://github.com/bazelbuild/bazel/issues/7122
-  bool incompatible_windows_style_arg_escaping;
 };
 
 }  // namespace blaze

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -1125,10 +1125,9 @@ private static FileSystem defaultFileSystemImplementation()
     return OS.getCurrent() == OS.WINDOWS ? new WindowsFileSystem() : new UnixFileSystem();
   }
 
-  private static SubprocessFactory subprocessFactoryImplementation(
-      BlazeServerStartupOptions startupOptions) {
+  private static SubprocessFactory subprocessFactoryImplementation() {
     if (!"0".equals(System.getProperty("io.bazel.EnableJni")) && OS.getCurrent() == OS.WINDOWS) {
-      return new WindowsSubprocessFactory(startupOptions.windowsStyleArgEscaping);
+      return WindowsSubprocessFactory.INSTANCE;
     } else {
       return JavaSubprocessFactory.INSTANCE;
     }
@@ -1234,7 +1233,7 @@ private static BlazeRuntime newRuntime(Iterable<BlazeModule> blazeModules, List<
           "No module set the default hash function.", ExitCode.BLAZE_INTERNAL_ERROR, e);
     }
     Path.setFileSystemForSerialization(fs);
-    SubprocessBuilder.setDefaultSubprocessFactory(subprocessFactoryImplementation(startupOptions));
+    SubprocessBuilder.setDefaultSubprocessFactory(subprocessFactoryImplementation());
 
     Path outputUserRootPath = fs.getPath(outputUserRoot);
     Path installBasePath = fs.getPath(installBase);

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
@@ -465,25 +465,6 @@ public String getTypeDescription() {
           + " actually encounter a condition that triggers them.")
   public boolean unlimitCoredumps;
 
-  @Option(
-      name = "incompatible_windows_style_arg_escaping",
-      defaultValue = "true", // NOTE: purely decorative, rc files are read by the client.
-      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
-      effectTags = {
-        OptionEffectTag.ACTION_COMMAND_LINES,
-        OptionEffectTag.EXECUTION,
-      },
-      metadataTags = {
-        OptionMetadataTag.INCOMPATIBLE_CHANGE,
-        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
-      },
-      help =
-          "On Linux/macOS/non-Windows: no-op. On Windows: if true, then subprocess arguments are"
-              + " escaped Windows-style. When false, the arguments are escaped Bash-style. The"
-              + " Bash-style is buggy, the Windows-style is correct. See"
-              + " https://github.com/bazelbuild/bazel/issues/7122")
-  public boolean windowsStyleArgEscaping;
-
   @Option(
       name = "macos_qos_class",
       defaultValue = "default", // Only for documentation; value is set and used by the client.

diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java
@@ -32,11 +32,7 @@
  * A subprocess factory that uses the Win32 API.
  */
 public class WindowsSubprocessFactory implements SubprocessFactory {
-  private final boolean windowsStyleArgEscaping;
-
-  public WindowsSubprocessFactory(boolean windowsStyleArgEscaping) {
-    this.windowsStyleArgEscaping = windowsStyleArgEscaping;
-  }
+  public static final WindowsSubprocessFactory INSTANCE = new WindowsSubprocessFactory();
 
   @Override
   public Subprocess create(SubprocessBuilder builder) throws IOException {
@@ -74,21 +70,17 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
   }
 
   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));
+    StringBuilder result = new StringBuilder();
+    boolean first = true;
+    for (String arg : argv) {
+      if (first) {
+        first = false;
+      } else {
+        result.append(" ");
       }
-      return result.toString();
-    } else {
-      return WindowsProcesses.quoteCommandLine(argv);
+      result.append(ShellUtils.windowsEscapeArg(arg));
     }
+    return result.toString();
   }
 
   public static String processArgv0(String argv0) {

diff --git a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsProcesses.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.windows.jni;
 
-import java.util.List;
 
 /** Process management on Windows. */
 public class WindowsProcesses {
@@ -215,59 +214,4 @@ public static int getpid() {
   }
 
   private static native int nativeGetpid();
-
-  // TODO(laszlocsomor): Replace this method with ShellUtils.windowsEscapeArg in order to fix
-  // https://github.com/bazelbuild/bazel/issues/7122
-  public static String quoteCommandLine(List<String> argv) {
-    StringBuilder result = new StringBuilder();
-    for (int iArg = 0; iArg < argv.size(); iArg++) {
-      if (iArg != 0) {
-        result.append(" ");
-      }
-      String arg = argv.get(iArg);
-      if (arg.isEmpty()) {
-        result.append("\"\"");
-        continue;
-      }
-      boolean hasSpace = arg.contains(" ");
-      if (!arg.contains("\"") && !arg.contains("\\") && !hasSpace) {
-        // fast path. Just append the input string.
-        result.append(arg);
-      } else {
-        // We need to quote things if the argument contains a space.
-        if (hasSpace) {
-          result.append("\"");
-        }
-
-        for (int iChar = 0; iChar < arg.length(); iChar++) {
-          boolean lastChar = iChar == arg.length() - 1;
-          switch (arg.charAt(iChar)) {
-            case '"':
-              // Escape double quotes
-              result.append("\\\"");
-              break;
-            case '\\':
-              // Backslashes at the end of the string are quoted if we add quotes
-              if (lastChar) {
-                result.append(hasSpace ? "\\\\" : "\\");
-              } else {
-                // Backslashes everywhere else are quoted if they are followed by a
-                // quote or a backslash
-                result.append(
-                    arg.charAt(iChar + 1) == '"' || arg.charAt(iChar + 1) == '\\' ? "\\\\" : "\\");
-              }
-              break;
-            default:
-              result.append(arg.charAt(iChar));
-          }
-        }
-        // Add ending quotes if we added a quote at the beginning.
-        if (hasSpace) {
-          result.append("\"");
-        }
-      }
-    }
-
-    return result.toString();
-  }
 }
diff --git a/src/test/cpp/bazel_startup_options_test.cc b/src/test/cpp/bazel_startup_options_test.cc
@@ -87,7 +87,6 @@ TEST_F(BazelStartupOptionsTest, ValidStartupFlags) {
   ExpectIsNullaryOption(options, "fatal_event_bus_exceptions");
   ExpectIsNullaryOption(options, "home_rc");
   ExpectIsNullaryOption(options, "host_jvm_debug");
-  ExpectIsNullaryOption(options, "incompatible_windows_style_arg_escaping");
   ExpectIsNullaryOption(options, "shutdown_on_low_sys_mem");
   ExpectIsNullaryOption(options, "ignore_all_rc_files");
   ExpectIsNullaryOption(options, "master_bazelrc");

diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
@@ -18,7 +18,8 @@
 import static java.nio.charset.StandardCharsets.UTF_16LE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.base.Joiner;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.windows.jni.WindowsProcesses;
@@ -65,16 +66,26 @@ public void terminateProcess() throws Exception {
     }
   }
 
+  private static List<String> quoteArgs(List<String> argv, String... args) {
+    for (String arg : args) {
+      argv.add(ShellUtils.windowsEscapeArg(arg));
+    }
+    return argv;
+  }
+
+  private static List<String> quoteArgs(String... args) {
+    List<String> argv = new ArrayList<>();
+    return quoteArgs(argv, args);
+  }
+
   private String mockArgs(String... args) {
     List<String> argv = new ArrayList<>();
 
     argv.add("-jar");
     argv.add(mockSubprocess);
-    for (String arg : args) {
-      argv.add(arg);
-    }
+    quoteArgs(argv, args);
 
-    return WindowsProcesses.quoteCommandLine(argv);
+    return Joiner.on(" ").join(argv);
   }
 
   private void assertNoProcessError() throws Exception {
@@ -87,35 +98,32 @@ private void assertNoStreamError(long stream) throws Exception {
 
   @Test
   public void testDoesNotQuoteSimpleArg() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a"))).isEqualTo("x a");
+    assertThat(quoteArgs("x", "a")).containsExactly("x", "a").inOrder();
   }
 
   @Test
   public void testQuotesEmptyArg() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", ""))).isEqualTo("x \"\"");
+    assertThat(quoteArgs("x", "")).containsExactly("x", "\"\"").inOrder();
   }
 
   @Test
   public void testQuotesArgWithSpace() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a b")))
-        .isEqualTo("x \"a b\"");
+    assertThat(quoteArgs("x", "a b")).containsExactly("x", "\"a b\"").inOrder();
   }
 
   @Test
   public void testDoesNotQuoteArgWithBackslash() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\\b")))
-        .isEqualTo("x a\\b");
+    assertThat(quoteArgs("x", "a\\b")).containsExactly("x", "a\\b").inOrder();
   }
 
   @Test
   public void testDoesNotQuoteArgWithSingleQuote() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a'b"))).isEqualTo("x a'b");
+    assertThat(quoteArgs("x", "a'b")).containsExactly("x", "a'b").inOrder();
   }
 
   @Test
-  public void testDoesNotQuoteArgWithDoubleQuote() throws Exception {
-    assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\"b")))
-        .isEqualTo("x a\\\"b");
+  public void testQuotesArgWithDoubleQuote() throws Exception {
+    assertThat(quoteArgs("x", "a\"b", "y")).containsExactly("x", "\"a\\\"b\"", "y").inOrder();
   }
 
   @Test