Skip to content

Commit

Permalink
Windows: add --incompatible_windows_escape_jvm_flags
Browse files Browse the repository at this point in the history
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: bazelbuild#7486

Related: bazelbuild#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.)
  • Loading branch information
laszlocsomor committed Feb 21, 2019
1 parent d19d4ba commit 3bf82ed
Show file tree
Hide file tree
Showing 7 changed files with 304 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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")
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,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;
Expand Down Expand Up @@ -213,6 +214,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) {
Expand Down Expand Up @@ -455,4 +457,8 @@ public boolean requireJavaToolchainHeaderCompilerDirect() {
public boolean disallowResourceJars() {
return disallowResourceJars;
}

public boolean windowsEscapeJvmFlags() {
return windowsEscapeJvmFlags;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,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) {
Expand Down Expand Up @@ -698,6 +719,8 @@ public FragmentOptions getHost() {

host.disallowResourceJars = disallowResourceJars;

host.windowsEscapeJvmFlags = windowsEscapeJvmFlags;

return host;
}

Expand Down
7 changes: 7 additions & 0 deletions src/test/shell/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,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.

Expand Down
233 changes: 233 additions & 0 deletions src/test/shell/integration/jvm_flags_escaping_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
#!/bin/bash
#
# Copyright 2019 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# --- begin runfiles.bash initialization ---
# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash).
set -euo pipefail
if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
if [[ -f "$0.runfiles_manifest" ]]; then
export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
elif [[ -f "$0.runfiles/MANIFEST" ]]; then
export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST"
elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
export RUNFILES_DIR="$0.runfiles"
fi
fi
if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash"
elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \
"$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)"
else
echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash"
exit 1
fi
# --- end runfiles.bash initialization ---

source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
|| { echo "integration_test_setup.sh not found!" >&2; exit 1; }

# `uname` returns the current platform, e.g "MSYS_NT-10.0" or "Linux".
# `tr` converts all upper case letters to lower case.
# `case` matches the result if the `uname | tr` expression to string prefixes
# that use the same wildcards as names do in Bash, i.e. "msys*" matches strings
# starting with "msys", and "*" matches everything (it's the default case).
case "$(uname -s | tr [:upper:] [:lower:])" in
msys*)
# As of 2019-02-20, Bazel on Windows only supports MSYS Bash.
declare -r is_windows=true
;;
*)
declare -r is_windows=false
;;
esac

if "$is_windows"; then
# Disable MSYS path conversion that converts path-looking command arguments to
# Windows paths (even if they arguments are not in fact paths).
export MSYS_NO_PATHCONV=1
export MSYS2_ARG_CONV_EXCL="*"
declare -r EXE_EXT=".exe"
else
declare -r EXE_EXT=""
fi

function create_pkg() {
if [[ -d foo ]]; then
return
fi
mkdir foo || fail "mkdir foo"

cat >foo/BUILD <<'eof'
java_binary(
name = "x",
srcs = ["A.java"],
main_class = "A",
jvm_flags = [
"-Darg0=''",
"-Darg1=' '",
"-Darg2='\"'",
"-Darg3='\"\\'",
"-Darg4='\\'",
"-Darg5='\\\"'",
"-Darg6='with space'",
"-Darg7='with^caret'",
"-Darg8='space ^caret'",
"-Darg9='caret^ space'",
"-Darg10='with\"quote'",
"-Darg11='with\\backslash'",
"-Darg12='one\\ backslash and \\space'",
"-Darg13='two\\\\backslashes'",
"-Darg14='two\\\\ backslashes \\\\and space'",
"-Darg15='one\\\"x'",
"-Darg16='two\\\\\"x'",
"-Darg17='a \\ b'",
"-Darg18='a \\\" b'",
"-Darg19='A'",
"-Darg20='\"a\"'",
"-Darg21='B C'",
"-Darg22='\"b c\"'",
"-Darg23='D\"E'",
"-Darg24='\"d\"e\"'",
"-Darg25='C:\\F G'",
"-Darg26='\"C:\\f g\"'",
"-Darg27='C:\\H\"I'",
"-Darg28='\"C:\\h\"i\"'",
"-Darg29='C:\\J\\\"K'",
"-Darg30='\"C:\\j\\\"k\"'",
"-Darg31='C:\\L M '",
"-Darg32='\"C:\\l m \"'",
"-Darg33='C:\\N O\\'",
"-Darg34='\"C:\\n o\\\"'",
"-Darg35='C:\\P Q\\ '",
"-Darg36='\"C:\\p q\\ \"'",
"-Darg37='C:\\R\\S\\'",
"-Darg38='C:\\R x\\S\\'",
"-Darg39='\"C:\\r\\s\\\"'",
"-Darg40='\"C:\\r x\\s\\\"'",
"-Darg41='C:\\T U\\W\\'",
"-Darg42='\"C:\\t u\\w\\\"'",
],
)
eof

cat >foo/A.java <<'eof'
public class A {
public static void main(String[] args) {
for (int i = 0; ; ++i) {
String value = System.getProperty("arg" + i);
if (value == null) {
break;
} else {
System.out.printf("arg%d=(%s)%n", i, value);
}
}
}
}
eof
}

function assert_output() {
expect_log 'arg0=()'
expect_log 'arg1=( )'
expect_log 'arg2=(")'
expect_log 'arg3=("\\)'
expect_log 'arg4=(\\)'
expect_log 'arg5=(\\")'
expect_log 'arg6=(with space)'
expect_log 'arg7=(with^caret)'
expect_log 'arg8=(space ^caret)'
expect_log 'arg9=(caret^ space)'
expect_log 'arg10=(with"quote)'
expect_log 'arg11=(with\\backslash)'
expect_log 'arg12=(one\\ backslash and \\space)'
expect_log 'arg13=(two\\\\backslashes)'
expect_log 'arg14=(two\\\\ backslashes \\\\and space)'
expect_log 'arg15=(one\\"x)'
expect_log 'arg16=(two\\\\"x)'
expect_log 'arg17=(a \\ b)'
expect_log 'arg18=(a \\" b)'
expect_log 'arg19=(A)'
expect_log 'arg20=("a")'
expect_log 'arg21=(B C)'
expect_log 'arg22=("b c")'
expect_log 'arg23=(D"E)'
expect_log 'arg24=("d"e")'
expect_log 'arg25=(C:\\F G)'
expect_log 'arg26=("C:\\f g")'
expect_log 'arg27=(C:\\H"I)'
expect_log 'arg28=("C:\\h"i")'
expect_log 'arg29=(C:\\J\\"K)'
expect_log 'arg30=("C:\\j\\"k")'
expect_log 'arg31=(C:\\L M )'
expect_log 'arg32=("C:\\l m ")'
expect_log 'arg33=(C:\\N O\\)'
expect_log 'arg34=("C:\\n o\\")'
expect_log 'arg35=(C:\\P Q\\ )'
expect_log 'arg36=("C:\\p q\\ ")'
expect_log 'arg37=(C:\\R\\S\\)'
expect_log 'arg38=(C:\\R x\\S\\)'
expect_log 'arg39=("C:\\r\\s\\")'
expect_log 'arg40=("C:\\r x\\s\\")'
expect_log 'arg41=(C:\\T U\\W\\)'
expect_log 'arg42=("C:\\t u\\w\\")'
}

function assert_jvm_flags() {
local -r enable_windows_escaping=$1

create_pkg

if $enable_windows_escaping; then
local -r flag="--incompatible_windows_escape_jvm_flags"
local -r expect_run_fails=false
else
local -r flag="--noincompatible_windows_escape_jvm_flags"
local -r expect_run_fails=$is_windows
fi

bazel build $flag foo:x --verbose_failures 2>$TEST_log \
|| fail "expected success"

if $expect_run_fails; then
(RUNFILES_DIR= \
RUNFILES_MANIFEST_FILE= \
RUNFILES_MANIFEST_ONLY= \
bazel-bin/foo/x${EXE_EXT} &>$TEST_log ; ) \
&& fail "expected failure" || true
expect_not_log "LAUNCHER ERROR"
expect_log "Could not find or load main class"
else
(RUNFILES_DIR= \
RUNFILES_MANIFEST_FILE= \
RUNFILES_MANIFEST_ONLY= \
bazel-bin/foo/x${EXE_EXT} &>$TEST_log ; ) || fail "expected success"
expect_not_log "LAUNCHER ERROR"
assert_output
fi
}

function test_no_windows_jvm_flags_escaping() {
# $1=false: build with --noincompatible_windows_escape_jvm_flags
assert_jvm_flags false
}

function test_windows_jvm_flags_escaping() {
# $1=true: build with --incompatible_windows_escape_jvm_flags
assert_jvm_flags true
}

run_suite "Tests about how Bazel passes java_binary.jvm_flags to the binary"
Loading

0 comments on commit 3bf82ed

Please sign in to comment.