Windows, java launcher: clean up after flag flip

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

@@ -404,22 +404,14 @@ 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);
+      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(
@@ -428,8 +420,7 @@ public Artifact createStubAction(
           classpath,
           javaStartClass,
           jvmFlagsForLauncher,
-          executable,
-          windowsEscapeJvmFlags);
+          executable);
     }
 
     ruleContext.registerAction(new TemplateExpansionAction(
@@ -443,8 +434,7 @@ private static Artifact createWindowsExeLauncher(
       NestedSet<Artifact> classpath,
       String javaStartClass,
       List<String> jvmFlags,
-      Artifact javaLauncher,
-      boolean windowsEscapeJvmFlags) {
+      Artifact javaLauncher) {
     LaunchInfo launchInfo =
         LaunchInfo.builder()
             .addKeyValuePair("binary_type", "Java")
@@ -464,7 +454,6 @@ 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,7 +178,6 @@ 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;
@@ -216,7 +215,6 @@ 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) {
@@ -491,8 +489,4 @@ 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

@@ -613,27 +613,6 @@ 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 = "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 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;
-
   Label defaultJavaBase() {
     return Label.parseAbsoluteUnchecked(DEFAULT_JAVABASE);
   }
@@ -695,8 +674,6 @@ public FragmentOptions getHost() {
 
     host.disallowResourceJars = disallowResourceJars;
 
-    host.windowsEscapeJvmFlags = windowsEscapeJvmFlags;
-
     return host;
   }
 

src/test/shell/integration/jvm_flags_escaping_test.sh

@@ -255,78 +255,35 @@ function expect_program_cannot_run() {
 # TESTS
 # ----------------------------------------------------------------------
 
-function test_jvm_flags_escaping_when_it_is_disabled_on_windows() {
-  local -r pkg="${FUNCNAME[0]}"  # unique package name for this test
-
-  create_java_file_that_prints_jvm_args "$pkg"
-  create_build_file_with_many_jvm_flags "$pkg"
-
-  # On all platforms, Bazel can build the target.
-  bazel build --verbose_failures --noincompatible_windows_escape_jvm_flags \
-    "${pkg}:x" &>"$TEST_log" || fail "expected success"
-  if "$is_windows"; then
-    # On Windows, the target cannot run.
-    expect_program_cannot_run "bazel-bin/$pkg/x${EXE_EXT}"
-    expect_log "Could not find or load main class"
-  else
-    # On other platforms, the program can run fine.
-    expect_program_runs "bazel-bin/$pkg/x${EXE_EXT}"
-    assert_output_of_the_program_with_many_jvm_flags
-  fi
-}
-
 function test_jvm_flags_escaping() {
   local -r pkg="${FUNCNAME[0]}"  # unique package name for this test
 
   create_java_file_that_prints_jvm_args "$pkg"
   create_build_file_with_many_jvm_flags "$pkg"
 
   # On all platforms, Bazel can build and run the target.
-  bazel build --verbose_failures --incompatible_windows_escape_jvm_flags \
+  bazel build --verbose_failures \
     "${pkg}:x" &>"$TEST_log" || fail "expected success"
   expect_program_runs "bazel-bin/$pkg/x${EXE_EXT}"
   assert_output_of_the_program_with_many_jvm_flags
 }
 
-function test_untokenizable_jvm_flag_when_escaping_is_disabled() {
-  local -r pkg="${FUNCNAME[0]}"  # unique package name for this test
-
-  create_java_file_that_prints_jvm_args "$pkg"
-  create_build_file_for_untokenizable_flag "$pkg"
-
-  # On all platforms, Bazel can build the target.
-  bazel build --verbose_failures --noincompatible_windows_escape_jvm_flags \
-    "${pkg}:cannot_tokenize" 2>"$TEST_log" || fail "expected success"
-  if "$is_windows"; then
-    # On Windows, Bazel will not check the flag. It will just propagate the flag
-    # to the launcher, which also just passes it to the JVM.  This is bad, the
-    # flag should have been rejected because it cannot be Bash-tokenized.
-    expect_program_runs "bazel-bin/$pkg/cannot_tokenize${EXE_EXT}"
-    expect_log "arg0=('abc)"
-  else
-    # On other platforms, Bazel will build the target but it fails to run.
-    expect_program_cannot_run "bazel-bin/$pkg/cannot_tokenize${EXE_EXT}"
-    expect_log "syntax error"
-  fi
-}
-
 function test_untokenizable_jvm_flag_when_escaping_is_enabled() {
   local -r pkg="${FUNCNAME[0]}"  # unique package name for this test
 
   create_java_file_that_prints_jvm_args "$pkg"
   create_build_file_for_untokenizable_flag "$pkg"
 
-  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.
-    bazel build --verbose_failures "$flag" "${pkg}:cannot_tokenize" \
+    bazel build --verbose_failures "${pkg}:cannot_tokenize" \
       2>"$TEST_log" && fail "expected failure" || true
     expect_log "ERROR:.*in jvm_flags attribute of java_binary rule"
   else
     # On other platforms, Bazel will build the target but it fails to run.
-    bazel build --verbose_failures "$flag" "${pkg}:cannot_tokenize" \
+    bazel build --verbose_failures "${pkg}:cannot_tokenize" \
       2>"$TEST_log" || fail "expected success"
     expect_program_cannot_run "bazel-bin/$pkg/cannot_tokenize${EXE_EXT}"
     expect_log "syntax error"

src/tools/launcher/java_launcher.cc

@@ -43,7 +43,6 @@ static constexpr const char* JAR_BIN_PATH = "jar_bin_path";
 static constexpr const char* CLASSPATH = "classpath";
 static constexpr const char* JAVA_START_CLASS = "java_start_class";
 static constexpr const char* JVM_FLAGS = "jvm_flags";
-static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_jvmflags";
 
 // Check if a string start with a certain prefix.
 // If it's true, store the substring without the prefix in value.
@@ -409,15 +408,10 @@ ExitCode JavaBinaryLauncher::Launch() {
     arguments.push_back(arg);
   }
 
-  wstring (*const escape_arg_func)(const wstring&) =
-      this->GetLaunchInfoByKey(WINDOWS_STYLE_ESCAPE_JVM_FLAGS) == L"1"
-          ? WindowsEscapeArg2
-          : WindowsEscapeArg;
-
   vector<wstring> escaped_arguments;
   // Quote the arguments if having spaces
   for (const auto& arg : arguments) {
-    escaped_arguments.push_back(escape_arg_func(arg));
+    escaped_arguments.push_back(WindowsEscapeArg2(arg));
   }
 
   ExitCode exit_code = this->LaunchProcess(java_bin, escaped_arguments);