Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: add --incompatible_windows_escape_jvm_flags #7490

Closed

Conversation

laszlocsomor
Copy link
Contributor

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

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).

- The result is that 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.

- The 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: 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.)

Change-Id: I531cc63cdfeccbe4b6d48876cb82870c1726a723
@laszlocsomor laszlocsomor requested review from lfpino and removed request for lberki February 21, 2019 12:51
@lfpino lfpino requested a review from iirina February 21, 2019 12:52
@lfpino
Copy link
Contributor

lfpino commented Feb 21, 2019

/CC @cushon

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift after fetching $1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

src/test/shell/integration/jvm_flags_escaping_test.sh Outdated Show resolved Hide resolved
src/test/shell/integration/jvm_flags_escaping_test.sh Outdated Show resolved Hide resolved
@laszlocsomor
Copy link
Contributor Author

@lfpino : PTAL

Copy link
Contributor

@lfpino lfpino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!

src/test/shell/integration/jvm_flags_escaping_test.sh Outdated Show resolved Hide resolved
# ----------------------------------------------------------------------

function test_no_windows_jvm_flags_escaping() {
local -r pkg="${FUNCNAME[0]}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Unless it's necessary, use concrete values for pkg, it removes one layer of indirection when looking at the lines of code using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep it like this. This generates a unique package name for every test.

In case the test cleanup logic were flawed (which would certainly be a bug that should be fixed) such that it didn't fully clean up after tests, we'd still run every test with its own package, reducing the chance that tests overwrite each others files.

src/test/shell/integration/jvm_flags_escaping_test.sh Outdated Show resolved Hide resolved
@laszlocsomor
Copy link
Contributor Author

All done.

@jin jin added the area-Windows Windows-specific issues and feature requests label Feb 22, 2019
@bazel-io bazel-io closed this in 8ac46c1 Feb 26, 2019
@laszlocsomor laszlocsomor deleted the jvm_flags_escaping branch February 27, 2019 08:03
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants