Repository: bazelbuild/bazel
Issue: incompatible_windows_escape_jvm_flags: on Windows, enables tokenization and escaping of jvm_flags
Labels: area-Windows
incompatible-change
team-OSS

# Description

Add --incompatible_windows_escape_jvm_flags flag (default: false). This flag only affects builds on Windows.

This flag has no effect on Linux/macOS/non-Windows.

## Background

When you build a java_binary or java_test on Windows, one of the outputs is an .exe file. This is called the launcher, and this is what you run with "bazel run" and "bazel test". The launcher's purpose is to set up the environment, compute the JVM command line (classpath, JVM flags, etc.) and launch the JVM.

The target-specific data (such as the classpath, main class name, but also the `jvm_flags` attribute from the BUILD-file) are embedded into the launcher as binary data. When you run the launcher, it will compute the command line to run the JVM as a subprocess (using the CreateProcessW system call), and that involves escaping the JVM flags so they can safely be passed to the subprcoess.

## The new flag

The --incompatible_windows_escape_jvm_flags flag affects how Bazel builds the launcher:
- whether or not Bazel will Bash-tokenize the jvm_flags before embedding them in the launcher, and
- whether the launcher will escape these flags using the [correct escaping logic](https://github.com/bazelbuild/bazel/blob/d19d4bae21d8706ae35a151b3135a6220189baa7/src/tools/launcher/util/launcher_util.cc#L182) or the [buggy one](https://github.com/bazelbuild/bazel/blob/d19d4bae21d8706ae35a151b3135a6220189baa7/src/tools/launcher/util/launcher_util.cc#L263).

When the flag is enabled:

- Bazel will Bash-tokenize java_binary.jvm_flags and java_test.jvm_flags (as documented by the Build Encyclopedia) before embedding them into the launcher.

- The launcher will properly escape these flags when it runs the JVM as a subprocess (using launcher_util::WindowsEscapeArg2).

- The result is that the jvm_flags declared in the BUILD file will arrive to the Java program as intended.

When the flag is disabled:

- Bazel does not Bash-tokenize the jvm_flags before embedding them in the launcher.

- The launcher escapes the flags with a Bash-like escaping logic (launcher_util::WindowsEscapeArg)
  which cannot properly quote and escape everything.

- The result is that the jvm_flags declared in the BUILD file might get messed up as they are passed to the JVM, or the launcher may not even be able to run the JVM.

Related bug: https://github.com/bazelbuild/bazel/issues/7072

# Example

BUILD file:
```
java_binary(
    name = "x",
    srcs = ["A.java"],
    main_class = "A",
    jvm_flags = [
        "-Darg0='hello world'",
        r"-Darg1=\"C:\\Program\ Files\\\"",
    ],
)
```

A.java:
```
public class A {
  public static void main(String[] args) {
    System.out.printf(
        "arg0=(%s)%narg1=(%s)%n",
        System.getProperty("arg0"),
        System.getProperty("arg1"));
  }
}
```

The expected output of running the binary is:
```
arg0=(hello world)
arg1=("C:\Program Files\")
```

Currently the output is something like:
```
Error: Could not find or load main class
```

# Migration recipe

None, as of 2019-02-21.

We don't expect any breakages when this flag is enabled. However if it breaks your build or **if it breaks your java_binaries or java_tests**, please let us know so we can help fixing it and provide a migration recipe.

# Rollout plan

* Bazel 0.23.0 will not support this flag.
* Bazel 0.24.0 is expected to support this flag, with default value being `false`.
* Bazel 0.25.0 is expected to flip this flag to true.

Issue comments:
Seems like the flag has been flipped to true in https://github.com/bazelbuild/bazel/commit/2029b1a2332b336f64ee8d9f6433b205aecefc91. I'm closing the issue. 
The flag is not yet flipped: https://github.com/bazelbuild/bazel/blob/b137071ea03537ac00592a572dc0140fab985c53/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java#L618

https://github.com/bazelbuild/bazel/commit/2029b1a2332b336f64ee8d9f6433b205aecefc91 was merged a testing branch, not master, to run the "Bazel / Bazel@HEAD + Downstream" pipeline on CI.
Ah, I did think the PR looked weird :)
Thanks for clarifying
I think it was released in 0.26 (https://blog.bazel.build/2019/05/28/bazel-0.26.0.html).

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
index 53420f5ad18524..7d1d28590db9d9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
@@ -643,7 +643,7 @@ public ImportDepsCheckingLevelConverter() {
 
   @Option(
       name = "incompatible_windows_escape_jvm_flags",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {
         OptionEffectTag.ACTION_COMMAND_LINES,


Test patch: