From 3bf82ed829fd2d87dde8c546cd170bcda84b92ac Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 20 Feb 2019 15:57:33 +0100 Subject: [PATCH] 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 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. Incompatible flag: https://github.com/bazelbuild/bazel/issues/7486 Related: https://github.com/bazelbuild/bazel/issues/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.) --- .../java/com/google/devtools/build/lib/BUILD | 1 + .../bazel/rules/java/BazelJavaSemantics.java | 32 ++- .../lib/rules/java/JavaConfiguration.java | 6 + .../build/lib/rules/java/JavaOptions.java | 23 ++ src/test/shell/integration/BUILD | 7 + .../integration/jvm_flags_escaping_test.sh | 233 ++++++++++++++++++ src/tools/launcher/java_launcher.cc | 8 +- 7 files changed, 304 insertions(+), 6 deletions(-) create mode 100644 src/test/shell/integration/jvm_flags_escaping_test.sh diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 211fa48dd9485c..e950a9408fbc1e 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -768,6 +768,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/java", "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/proto", "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/python", + "//src/main/java/com/google/devtools/build/lib/shell", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java index 399f7afb5c2a3e..d2cf3a15bbdd3a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java +++ b/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 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 classpath, String javaStartClass, - ImmutableList jvmFlags, - Artifact javaLauncher) { - + List 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 diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java index 2f4e6172df366c..8b847e05f8796a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java @@ -177,6 +177,7 @@ public boolean alwaysGenerateOutputMapping() { private final ImmutableList