Remove --[no]incompatible_windows_escape_python_args

The flag was flipped in Bazel 0.27.0.

Removing the flag, the code path for "false"
value, and tests.

Fixes bazelbuild#7974

RELNOTES[INC]: Python, Windows: the --[no]incompatible_windows_escape_python_args is no longer supported. (It was flipped to true in Bazel 0.27.0)

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java

@@ -256,8 +256,7 @@ public void createExecutable(
           // unix. See also https://github.com/bazelbuild/bazel/issues/7947#issuecomment-491385802.
           pythonBinary,
           executable,
-          /*useZipFile=*/ buildPythonZip,
-          /*windowsEscapePythonArgs=*/ config.windowsEscapePythonArgs());
+          /*useZipFile=*/ buildPythonZip);
     }
   }
 
@@ -266,8 +265,7 @@ private static void createWindowsExeLauncher(
       RuleContext ruleContext,
       String pythonBinary,
       Artifact pythonLauncher,
-      boolean useZipFile,
-      boolean windowsEscapePythonArgs)
+      boolean useZipFile)
       throws InterruptedException {
     LaunchInfo launchInfo =
         LaunchInfo.builder()
@@ -278,7 +276,6 @@ private static void createWindowsExeLauncher(
                 ruleContext.getConfiguration().runfilesEnabled() ? "1" : "0")
             .addKeyValuePair("python_bin_path", pythonBinary)
             .addKeyValuePair("use_zip_file", useZipFile ? "1" : "0")
-            .addKeyValuePair("escape_args", windowsEscapePythonArgs ? "1" : "0")
             .build();
     LauncherFileWriteAction.createAndRegister(ruleContext, pythonLauncher, launchInfo);
   }

src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java

@@ -58,8 +58,6 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
   // TODO(brandjon): Remove this once migration for native rule access is complete.
   private final boolean loadPythonRulesFromBzl;
 
-  private final boolean windowsEscapePythonArgs;
-
   PythonConfiguration(
       PythonVersion version,
       PythonVersion defaultVersion,
@@ -70,8 +68,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
       boolean py2OutputsAreSuffixed,
       boolean disallowLegacyPyProvider,
       boolean useToolchains,
-      boolean loadPythonRulesFromBzl,
-      boolean windowsEscapePythonArgs) {
+      boolean loadPythonRulesFromBzl) {
     this.version = version;
     this.defaultVersion = defaultVersion;
     this.buildPythonZip = buildPythonZip;
@@ -82,7 +79,6 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
     this.disallowLegacyPyProvider = disallowLegacyPyProvider;
     this.useToolchains = useToolchains;
     this.loadPythonRulesFromBzl = loadPythonRulesFromBzl;
-    this.windowsEscapePythonArgs = windowsEscapePythonArgs;
   }
 
   /**
@@ -208,8 +204,4 @@ public boolean useToolchains() {
   public boolean loadPythonRulesFromBzl() {
     return loadPythonRulesFromBzl;
   }
-
-  public boolean windowsEscapePythonArgs() {
-    return windowsEscapePythonArgs;
-  }
 }

src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java

@@ -44,8 +44,7 @@ public PythonConfiguration create(BuildOptions buildOptions)
         /*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed,
         /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider,
         /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains,
-        /*loadPythonRulesFromBzl=*/ pythonOptions.loadPythonRulesFromBzl,
-        pythonOptions.windowsEscapePythonArgs);
+        /*loadPythonRulesFromBzl=*/ pythonOptions.loadPythonRulesFromBzl);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java

@@ -269,26 +269,6 @@ public String getTypeDescription() {
               + "data runfiles of another binary.")
   public boolean buildTransitiveRunfilesTrees;
 
-  @Option(
-      name = "incompatible_windows_escape_python_args",
-      defaultValue = "true",
-      documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
-      effectTags = {
-        OptionEffectTag.ACTION_COMMAND_LINES,
-        OptionEffectTag.AFFECTS_OUTPUTS,
-      },
-      metadataTags = {
-        OptionMetadataTag.INCOMPATIBLE_CHANGE,
-        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
-      },
-      help =
-          "On Linux/macOS/non-Windows: no-op. On Windows: this flag affects how py_binary and"
-              + " py_test targets are built: how their launcher escapes command line flags. When"
-              + " this flag is true, the launcher escapes command line flags using Windows-style"
-              + " escaping (correct behavior). When the flag is false, the launcher uses Bash-style"
-              + " escaping (buggy behavior). See https://github.com/bazelbuild/bazel/issues/7958")
-  public boolean windowsEscapePythonArgs;
-
   @Override
   public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
     // TODO(brandjon): Instead of referencing the python_version target, whose path depends on the
@@ -408,7 +388,6 @@ public FragmentOptions getHost() {
     hostPythonOptions.incompatibleDisallowLegacyPyProvider = incompatibleDisallowLegacyPyProvider;
     hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains;
     hostPythonOptions.loadPythonRulesFromBzl = loadPythonRulesFromBzl;
-    hostPythonOptions.windowsEscapePythonArgs = windowsEscapePythonArgs;
 
     // Save host options in case of a further exec->host transition.
     hostPythonOptions.hostForcePython = hostForcePython;

src/test/shell/integration/py_args_escaping_test.sh

@@ -167,25 +167,6 @@ py_binary(
 eof
 }
 
-# Asserts that the $TEST_log contains bad py_binary args.
-#
-# This assertion guards (and demonstrates) the status quo.
-#
-# See create_build_file_with_many_args() and create_py_file_that_prints_args().
-function assert_bad_output_of_the_program_with_many_args() {
-  expect_log 'arg1=()'
-  expect_log 'arg2=( )'
-  expect_log 'arg3=(")'
-  expect_log 'arg4=("\\)'
-  # arg5 and arg6 already stumble. No need to assert more.
-  expect_log 'arg5=(\\)'
-  expect_log 'arg6=(\\ with)'
-  # To illustrate the bug again, these args match those in the bug report:
-  # https://github.com/bazelbuild/bazel/issues/7958
-  expect_log 'arg40=(a)'
-  expect_log 'arg41=(b\\ c)'
-}
-
 # Asserts that the $TEST_log contains all py_binary args as argN=(VALUE).
 #
 # See create_build_file_with_many_args() and create_py_file_that_prints_args().
@@ -242,27 +223,6 @@ function assert_good_output_of_the_program_with_many_args() {
 # TESTS
 # ----------------------------------------------------------------------
 
-function test_args_escaping_disabled_on_windows() {
-  local -r ws="$TEST_TMPDIR/${FUNCNAME[0]}"  # unique workspace for this test
-  mkdir -p "$ws"
-  create_workspace_with_default_repos "$ws/WORKSPACE"
-
-  create_py_file_that_prints_args "$ws"
-  create_build_file_with_many_args "$ws"
-
-  ( cd "$ws"
-    bazel run --verbose_failures --noincompatible_windows_escape_python_args \
-      :x &>"$TEST_log" || fail "expected success"
-  )
-  if "$is_windows"; then
-    # On Windows, the target runs but prints bad output.
-    assert_bad_output_of_the_program_with_many_args
-  else
-    # On other platforms, the program runs fine and prints correct output.
-    assert_good_output_of_the_program_with_many_args
-  fi
-}
-
 function test_args_escaping() {
   local -r ws="$TEST_TMPDIR/${FUNCNAME[0]}"  # unique workspace for this test
   mkdir -p "$ws"
@@ -273,41 +233,21 @@ function test_args_escaping() {
 
   # On all platforms, the target prints good output.
   ( cd "$ws"
-    bazel run --verbose_failures --incompatible_windows_escape_python_args \
-      :x &>"$TEST_log" || fail "expected success"
+    bazel run --verbose_failures :x &>"$TEST_log" || fail "expected success"
   )
   assert_good_output_of_the_program_with_many_args
 }
 
-function test_untokenizable_args_when_escaping_is_disabled() {
+function test_untokenizable_args() {
   local -r ws="$TEST_TMPDIR/${FUNCNAME[0]}"  # unique workspace for this test
   mkdir -p "$ws"
   create_workspace_with_default_repos "$ws/WORKSPACE"
 
   create_py_file_that_prints_args "$ws"
   create_build_file_for_untokenizable_args "$ws"
 
-  # On all platforms, Bazel can build the target.
   ( cd "$ws"
-    if bazel build --verbose_failures --noincompatible_windows_escape_python_args \
-        :cannot_tokenize 2>"$TEST_log"; then
-      fail "expected failure"
-    fi
-  )
-  expect_log "unterminated quotation"
-}
-
-function test_untokenizable_args_when_escaping_is_enabled() {
-  local -r ws="$TEST_TMPDIR/${FUNCNAME[0]}"  # unique workspace for this test
-  mkdir -p "$ws"
-  create_workspace_with_default_repos "$ws/WORKSPACE"
-
-  create_py_file_that_prints_args "$ws"
-  create_build_file_for_untokenizable_args "$ws"
-
-  local -r flag="--incompatible_windows_escape_python_args"
-  ( cd "$ws"
-    bazel run --verbose_failures "$flag" :cannot_tokenize \
+    bazel run --verbose_failures :cannot_tokenize \
       2>"$TEST_log" && fail "expected failure" || true
   )
   expect_log "ERROR:.*in args attribute of py_binary rule.*unterminated quotation"
@@ -363,27 +303,7 @@ run_host_configured = rule(
 eof
 
   ( cd "$ws"
-    bazel build --verbose_failures --noincompatible_windows_escape_python_args \
-      :x &>"$TEST_log" || fail "expected success"
-    cat bazel-bin/x.out >> "$TEST_log"
-  )
-  if "$is_windows"; then
-    # This output is wrong, but expected on Windows with
-    # --noincompatible_windows_escape_python_args.
-    expect_log 'arg2=(a)'
-    expect_log 'arg3=()'
-    expect_log 'arg4=("b \\c z)'
-  else
-    # This output is right.
-    expect_log 'arg2=(a)'
-    expect_log 'arg3=()'
-    expect_log 'arg4=("b \\"c)'
-    expect_log 'arg5=(z)'
-  fi
-
-  ( cd "$ws"
-    bazel build --verbose_failures --incompatible_windows_escape_python_args \
-      :x &>"$TEST_log" || fail "expected success"
+    bazel build --verbose_failures :x &>"$TEST_log" || fail "expected success"
     cat bazel-bin/x.out >> "$TEST_log"
   )
   # This output is right.

src/tools/launcher/python_launcher.cc

@@ -26,7 +26,6 @@ using std::wstring;
 
 static constexpr const char* PYTHON_BIN_PATH = "python_bin_path";
 static constexpr const char* USE_ZIP_FILE = "use_zip_file";
-static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args";
 
 ExitCode PythonBinaryLauncher::Launch() {
   wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH);
@@ -64,13 +63,10 @@ ExitCode PythonBinaryLauncher::Launch() {
   // Replace the first argument with python file path
   args[0] = python_file;
 
-  wstring (*const escape_arg_func)(const wstring&) =
-      this->GetLaunchInfoByKey(WINDOWS_STYLE_ESCAPE_JVM_FLAGS) == L"1"
-          ? WindowsEscapeArg2
-          : WindowsEscapeArg;
+  wstring (*const escape_arg_func)(const wstring&) = WindowsEscapeArg2;
 
   for (int i = 1; i < args.size(); i++) {
-    args[i] = escape_arg_func(args[i]);
+    args[i] = WindowsEscapeArg2(args[i]);
   }
 
   return this->LaunchProcess(python_binary, args);

src/tools/launcher/util/launcher_util.cc

@@ -133,8 +133,7 @@ wstring GetBinaryPathWithExtension(const wstring& binary) {
   return GetBinaryPathWithoutExtension(binary) + L".exe";
 }
 
-static wstring GetEscapedArgument(const wstring& argument,
-                                  bool escape_backslash) {
+std::wstring BashEscapeArg(const std::wstring& argument) {
   wstring escaped_arg;
   // escaped_arg will be at least this long
   escaped_arg.reserve(argument.size());
@@ -156,8 +155,8 @@ static wstring GetEscapedArgument(const wstring& argument,
         break;
 
       case L'\\':
-        // Escape back slashes if escape_backslash is true
-        escaped_arg += (escape_backslash ? L"\\\\" : L"\\");
+        // Escape back slashes.
+        escaped_arg += L"\\\\";
         break;
 
       default:
@@ -171,10 +170,6 @@ static wstring GetEscapedArgument(const wstring& argument,
   return escaped_arg;
 }
 
-std::wstring BashEscapeArg(const std::wstring& arg) {
-  return GetEscapedArgument(arg, /* escape_backslash */ true);
-}
-
 // Escape arguments for CreateProcessW.
 //
 // This algorithm is based on information found in
@@ -265,11 +260,6 @@ std::wstring WindowsEscapeArg2(const std::wstring& s) {
   return result.str();
 }
 
-std::wstring WindowsEscapeArg(const std::wstring& arg) {
-  // TODO(laszlocsomor): use WindowsEscapeArg2 instead.
-  return GetEscapedArgument(arg, /* escape_backslash */ false);
-}
-
 // An environment variable has a maximum size limit of 32,767 characters
 // https://msdn.microsoft.com/en-us/library/ms683188.aspx
 static const int BUFFER_SIZE = 32767;

src/tools/launcher/util/launcher_util.h

@@ -53,11 +53,6 @@ std::wstring BashEscapeArg(const std::wstring& arg);
 // This escaping lets us safely pass arguments to subprocesses created with
 // CreateProcessW. (The escaping rules are a bit complex, look at the function
 // implementation.)
-std::wstring WindowsEscapeArg(const std::wstring& arg);
-
-// TODO(laszlocsomor): Delete WindowsEscapeArg and use WindowsEscapeArg2.
-// WindowsEscapeArg escapes incorrectly while WindowsEscapeArg2 escapes
-// correctly.
 std::wstring WindowsEscapeArg2(const std::wstring& arg);
 
 // Convert a path to an absolute Windows path with \\?\ prefix.

src/tools/launcher/util/launcher_util_test.cc

@@ -256,27 +256,6 @@ void AssertSubprocessReceivesArgsAsIntended(
   }
 }
 
-TEST_F(LaunchUtilTest, WindowsEscapeArgTest) {
-  // List of arguments with their expected WindowsEscapeArg-encoded version.
-  AssertSubprocessReceivesArgsAsIntended(
-      WindowsEscapeArg,
-      {
-          // Each pair is:
-          // - first: argument to pass (and expected output from subprocess)
-          // - second: expected WindowsEscapeArg-encoded string
-          {L"foo", L"foo"},
-          {L"", L"\"\""},
-          {L" ", L"\" \""},
-          {L"foo\\bar", L"foo\\bar"},
-          {L"C:\\foo\\bar\\", L"C:\\foo\\bar\\"},
-          // TODO(laszlocsomor): fix WindowsEscapeArg to use correct escaping
-          // semantics (not Bash semantics) and add more tests. The example
-          // below is
-          // escaped incorrectly.
-          // {L"C:\\foo bar\\", L"\"C:\\foo bar\\\""},
-      });
-}
-
 TEST_F(LaunchUtilTest, WindowsEscapeArg2Test) {
   AssertSubprocessReceivesArgsAsIntended(
       WindowsEscapeArg2,