Windows, python: add inc. flag to fix esacping

See bazelbuild#7958 RELNOTES[NEW]: Windows, Python: the --incompatible_windows_escape_python_args flag (false by default) builds py_binary and py_test targets with correct command line argument escaping. Change-Id: I789f9370e2cf59fa1a179716ca1c6ad80e1d583e
laszlocsomor · Apr 8, 2019 · 1cf17b1 · 1cf17b1
1 parent 20a78a8
commit 1cf17b1
Show file tree

Hide file tree

Showing 8 changed files with 365 additions and 10 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
@@ -145,8 +145,15 @@ public Artifact createExecutable(
       if (OS.getCurrent() == OS.WINDOWS) {
         // On Windows, use a Windows native binary to launch the python launcher script (stub file).
         stubOutput = common.getPythonLauncherArtifact(executable);
+
+        boolean windowsEscapePythonArgs =
+            ruleContext
+                .getConfiguration()
+                .getFragment(PythonConfiguration.class)
+                .windowsEscapePythonArgs();
         executable =
-            createWindowsExeLauncher(ruleContext, pythonBinary, executable, /*useZipFile*/ false);
+            createWindowsExeLauncher(ruleContext, pythonBinary, executable, /*useZipFile*/ false,
+                windowsEscapePythonArgs);
       }
 
       ruleContext.registerAction(
@@ -198,15 +205,23 @@ public Artifact createExecutable(
                 .setMnemonic("BuildBinary")
                 .build(ruleContext));
       } else {
-        return createWindowsExeLauncher(ruleContext, pythonBinary, executable, true);
+        boolean windowsEscapePythonArgs =
+            ruleContext
+                .getConfiguration()
+                .getFragment(PythonConfiguration.class)
+                .windowsEscapePythonArgs();
+
+        return createWindowsExeLauncher(ruleContext, pythonBinary, executable, true,
+            windowsEscapePythonArgs);
       }
     }
 
     return executable;
   }
 
   private static Artifact createWindowsExeLauncher(
-      RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher, boolean useZipFile)
+      RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher, boolean useZipFile,
+      boolean windowsEscapePythonArgs)
       throws InterruptedException {
     LaunchInfo launchInfo =
         LaunchInfo.builder()
@@ -217,6 +232,7 @@ private static Artifact 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);
     return pythonLauncher;

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java
@@ -55,6 +55,8 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
   // TODO(brandjon): Remove this once migration to Python toolchains is complete.
   private final boolean useToolchains;
 
+  private final boolean windowsEscapePythonArgs;
+
   PythonConfiguration(
       PythonVersion version,
       PythonVersion defaultVersion,
@@ -64,7 +66,8 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
       boolean useNewPyVersionSemantics,
       boolean py2OutputsAreSuffixed,
       boolean disallowLegacyPyProvider,
-      boolean useToolchains) {
+      boolean useToolchains,
+      boolean windowsEscapePythonArgs) {
     this.version = version;
     this.defaultVersion = defaultVersion;
     this.buildPythonZip = buildPythonZip;
@@ -74,6 +77,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
     this.py2OutputsAreSuffixed = py2OutputsAreSuffixed;
     this.disallowLegacyPyProvider = disallowLegacyPyProvider;
     this.useToolchains = useToolchains;
+    this.windowsEscapePythonArgs = windowsEscapePythonArgs;
   }
 
   /**
@@ -188,4 +192,8 @@ public boolean disallowLegacyPyProvider() {
   public boolean useToolchains() {
     return useToolchains;
   }
+
+  public boolean windowsEscapePythonArgs() {
+    return windowsEscapePythonArgs;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java
@@ -43,7 +43,8 @@ public PythonConfiguration create(BuildOptions buildOptions)
         /*useNewPyVersionSemantics=*/ pythonOptions.incompatibleAllowPythonVersionTransitions,
         /*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed,
         /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider,
-        /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains);
+        /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains,
+        pythonOptions.windowsEscapePythonArgs);
   }
 
   @Override

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
@@ -254,6 +254,26 @@ public String getTypeDescription() {
               + "data runfiles of another binary.")
   public boolean buildTransitiveRunfilesTrees;
 
+  @Option(
+      name = "incompatible_windows_escape_python_args",
+      defaultValue = "false",
+      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

diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
@@ -600,6 +600,13 @@ sh_test(
     deps = ["@bazel_tools//tools/bash/runfiles"],
 )
 
+sh_test(
+    name = "py_args_escaping_test",
+    srcs = ["py_args_escaping_test.sh"],
+    data = [":test-deps"],
+    deps = ["@bazel_tools//tools/bash/runfiles"],
+)
+
 ########################################################################
 # Test suites.
 

diff --git a/src/test/shell/integration/jvm_flags_escaping_test.sh b/src/test/shell/integration/jvm_flags_escaping_test.sh
@@ -318,9 +318,7 @@ function test_untokenizable_jvm_flag_when_escaping_is_enabled() {
 
   local -r flag="--incompatible_windows_escape_jvm_flags"
   if "$is_windows"; then
-    # On Windows, Bazel will check the flag. It will just propagate the flag
-    # to the launcher, which also just passes the flag to the JVM. This is bad,
-    # the flag should have been rejected because it cannot be Bash-tokenized.
+    # On Windows, Bazel will check the flag.
     bazel build --verbose_failures "$flag" "${pkg}:cannot_tokenize" \
       2>"$TEST_log" && fail "expected failure" || true
     expect_log "ERROR:.*in jvm_flags attribute of java_binary rule"