Windows: add --incompatible_windows_escape_jvm_flags

Add --incompatible_windows_escape_jvm_flags flag
(default: false). This flag has no effect on
platforms other than Windows.

This flag affects how Bazel builds the launcher of
java_binary and java_test targets. (The launcher
is the .exe file that sets up the environment for
the Java program and launches the JVM.)

In particular, the flag controls whether or not
Bazel will Bash-tokenize the jvm_flags that were
declared in the BUILD file, and whether or not
these jvm_flags are escaped by the launcher when
it invokes the JVM.

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 build fails if an entry cannot
  be Bash-tokenized, e.g. if it has unterminated
  quotes.

- The launcher will properly escape these flags
  when it runs the JVM as a subprocess (using
  launcher_util::WindowsEscapeArg2).

- Result: the jvm_flags declared in the BUILD file
  will arrive to the Java program as intended.
  However, due to the extra Bash-tokenization
  step, some ill-formed flags are no longer
  accepted, e.g.  `jvm_flags=["-Dfoo='a"]` now
  results in a build error.

When the flag is disabled:

- Bazel does not Bash-tokenize the jvm_flags
  before embedding them in the launcher. This
  preserves quoting meant to be stripped away, and
  it also means Bazel won't check whether the
  argument is properly quoted.

- The launcher escapes the flags with a Bash-like
  escaping logic (launcher_util::WindowsEscapeArg)
  which cannot properly quote and escape
  everything.

- Result: 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. However, due to the lack of
  Bash-tokenization, Bazel propagates some flags
  to the Java binary that it would no longer
  accept if the new
  `--incompatible_windows_escape_jvm_flags` were
  enabled, e.g. `jvm_flags=["'a"]` is fine.

Incompatible flag: #7486

Related: #7072

RELNOTES[NEW]: Added --incompatible_windows_escape_jvm_flags flag: enables correct java_binary.jvm_flags and java_test.jvm_flags tokenization and escaping on Windows. (No-op on other platforms.)

Change-Id: I531cc63cdfeccbe4b6d48876cb82870c1726a723

Closes #7490.

PiperOrigin-RevId: 235700143

.bazelci/postsubmit.yml

@@ -105,6 +105,7 @@ platforms:
     - "-//src/test/shell/integration:discard_analysis_cache_test"
     - "-//src/test/shell/integration:discard_graph_edges_test"
     - "-//src/test/shell/integration:java_integration_test"
+    - "-//src/test/shell/integration:jvm_flags_escaping_test"
     - "-//src/test/shell/integration:minimal_jdk_test"
     - "-//src/test/shell/integration:nonincremental_builds_test"
     - "-//src/test/shell/integration:output_filter_test"

.bazelci/presubmit.yml

@@ -127,6 +127,7 @@ platforms:
     - "-//src/test/shell/integration:discard_analysis_cache_test"
     - "-//src/test/shell/integration:discard_graph_edges_test"
     - "-//src/test/shell/integration:java_integration_test"
+    - "-//src/test/shell/integration:jvm_flags_escaping_test"
     - "-//src/test/shell/integration:minimal_jdk_test"
     - "-//src/test/shell/integration:nonincremental_builds_test"
     - "-//src/test/shell/integration:output_filter_test"

src/main/java/com/google/devtools/build/lib/BUILD

@@ -762,6 +762,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/rules/java:java-rules",
         "//src/main/java/com/google/devtools/build/lib/rules/objc",
         "//src/main/java/com/google/devtools/build/lib/rules/platform",
+        "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/apple",

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

@@ -66,6 +66,8 @@
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
 import com.google.devtools.build.lib.rules.java.JavaUtil;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
+import com.google.devtools.build.lib.shell.ShellUtils;
+import com.google.devtools.build.lib.shell.ShellUtils.TokenizationException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
@@ -407,13 +409,32 @@ public Artifact createStubAction(
     arguments.add(Substitution.ofSpaceSeparatedList("%jvm_flags%", jvmFlagsList));
 
     if (OS.getCurrent() == OS.WINDOWS) {
+      boolean windowsEscapeJvmFlags =
+          ruleContext
+              .getConfiguration()
+              .getFragment(JavaConfiguration.class)
+              .windowsEscapeJvmFlags();
+
+      List<String> jvmFlagsForLauncher = jvmFlagsList;
+      if (windowsEscapeJvmFlags) {
+        try {
+          jvmFlagsForLauncher = new ArrayList<>(jvmFlagsList.size());
+          for (String f : jvmFlagsList) {
+            ShellUtils.tokenize(jvmFlagsForLauncher, f);
+          }
+        } catch (TokenizationException e) {
+          ruleContext.attributeError("jvm_flags", "could not Bash-tokenize flag: " + e);
+        }
+      }
+
       return createWindowsExeLauncher(
           ruleContext,
           javaExecutable,
           classpath,
           javaStartClass,
-          jvmFlagsList,
-          executable);
+          jvmFlagsForLauncher,
+          executable,
+          windowsEscapeJvmFlags);
     }
 
     ruleContext.registerAction(new TemplateExpansionAction(
@@ -426,9 +447,9 @@ private static Artifact createWindowsExeLauncher(
       String javaExecutable,
       NestedSet<Artifact> classpath,
       String javaStartClass,
-      ImmutableList<String> jvmFlags,
-      Artifact javaLauncher) {
-
+      List<String> jvmFlags,
+      Artifact javaLauncher,
+      boolean windowsEscapeJvmFlags) {
     LaunchInfo launchInfo =
         LaunchInfo.builder()
             .addKeyValuePair("binary_type", "Java")
@@ -448,6 +469,7 @@ private static Artifact createWindowsExeLauncher(
                 "classpath",
                 ";",
                 Iterables.transform(classpath, Artifact.ROOT_RELATIVE_PATH_STRING))
+            .addKeyValuePair("escape_jvmflags", windowsEscapeJvmFlags ? "1" : "0")
             // TODO(laszlocsomor): Change the Launcher to accept multiple jvm_flags entries. As of
             // 2019-02-13 the Launcher accepts just one jvm_flags entry, which contains all the
             // flags, joined by TAB characters. The Launcher splits up the string to get the

src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java

@@ -178,6 +178,7 @@ public boolean alwaysGenerateOutputMapping() {
   private final ImmutableList<Label> pluginList;
   private final boolean requireJavaToolchainHeaderCompilerDirect;
   private final boolean disallowResourceJars;
+  private final boolean windowsEscapeJvmFlags;
 
   // TODO(dmarting): remove once we have a proper solution for #2539
   private final boolean useLegacyBazelJavaTest;
@@ -215,6 +216,7 @@ public boolean alwaysGenerateOutputMapping() {
     this.isJlplStrictDepsEnforced = javaOptions.isJlplStrictDepsEnforced;
     this.disallowResourceJars = javaOptions.disallowResourceJars;
     this.addTestSupportToCompileTimeDeps = javaOptions.addTestSupportToCompileTimeDeps;
+    this.windowsEscapeJvmFlags = javaOptions.windowsEscapeJvmFlags;
 
     ImmutableList.Builder<Label> translationsBuilder = ImmutableList.builder();
     for (String s : javaOptions.translationTargets) {
@@ -461,4 +463,8 @@ public boolean requireJavaToolchainHeaderCompilerDirect() {
   public boolean disallowResourceJars() {
     return disallowResourceJars;
   }
+
+  public boolean windowsEscapeJvmFlags() {
+    return windowsEscapeJvmFlags;
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java

@@ -641,6 +641,27 @@ public ImportDepsCheckingLevelConverter() {
           "Disables the resource_jars attribute; use java_import and deps or runtime_deps instead.")
   public boolean disallowResourceJars;
 
+  @Option(
+      name = "incompatible_windows_escape_jvm_flags",
+      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 java_binary and"
+              + " java_test targets are built; in particular, how the launcher of these targets"
+              + " escapes flags at the time of running the java_binary/java_test. When the flag is"
+              + " true, the launcher escapes the JVM 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/7072")
+  public boolean windowsEscapeJvmFlags;
+
   private Label getHostJavaBase() {
     if (hostJavaBase == null) {
       if (useJDK11AsHostJavaBase) {
@@ -715,6 +736,8 @@ public FragmentOptions getHost() {
 
     host.disallowResourceJars = disallowResourceJars;
 
+    host.windowsEscapeJvmFlags = windowsEscapeJvmFlags;
+
     return host;
   }
 

src/test/shell/integration/BUILD

@@ -583,6 +583,13 @@ sh_test(
     tags = ["no_windows"],
 )
 
+sh_test(
+    name = "jvm_flags_escaping_test",
+    srcs = ["jvm_flags_escaping_test.sh"],
+    data = [":test-deps"],
+    deps = ["@bazel_tools//tools/bash/runfiles"],
+)
+
 ########################################################################
 # Test suites.