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

incompatible_windows_escape_python_args: on Windows, enables correct escaping of py_binary.args and py_test.args #7974

Closed
laszlocsomor opened this issue Apr 8, 2019 · 0 comments
Assignees
Labels
area-Windows Windows-specific issues and feature requests incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website

Comments

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Apr 8, 2019

Description

Add --incompatible_windows_escape_python_args flag (default: false).

This is a no-op on Linux/macOS.

On Linux/macOS/non-Windows: the flag is a no-op.

On Windows: This flag affects how py_binary and py_test targets are built, how their launcher escapes command line flags. When this flag is true, the launcher escapes command line flags using Windows-style escaping (correct behavior). When the flag is false, the launcher uses Bash-style escaping (buggy behavior).

Background

The output of a py_binary or py_test is an .exe file of two parts: a stub (launcher) and a payload (zip file). The payload is the actual Python program with all its runfiles..

To build py_binary/py_test rules, Bazel appends the zip file and some metadata to the launcher. The launcher reads the data from its tail, then sets up the environment and runs Python, passing the command line arguments along.

However the launcher escapes arguments badly. This is the same bug for Python as #7486 was for Java. The solution is similar: use WindowsEscapeArg2 in the launcher:

args[i] = WindowsEscapeArg(args[i]);

// Escape arguments for CreateProcessW.
//
// This algorithm is based on information found in
// http://daviddeley.com/autohotkey/parameters/parameters.htm
//
// The following source specifies a similar algorithm:
// https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
// unfortunately I found this algorithm only after creating the one below, but
// fortunately they seem to do the same.
std::wstring WindowsEscapeArg2(const std::wstring& s) {

Related bug: #7958

The new flag

The --incompatible_windows_escape_python_args flag affects the metadata Bazel embeds in the launcher, which governs whether the launcher uses bad or good escaping.

Migration recipe

None, as of 2019-04-08.

We don't expect any breakages when this flag is enabled. However if it breaks your build or if it breaks your py_binaries or py_tests, please let us know so we can help fixing it and provide a migration recipe.

Rollout plan

  • Bazel 0.25.0 will not support this flag.
  • Bazel 0.26.0 is expected to support this flag, with default value being false.
  • Bazel 0.27.0 is expected to flip this flag to true, but keep the flag.
@laszlocsomor laszlocsomor self-assigned this Apr 8, 2019
@laszlocsomor laszlocsomor added bazel 1.0 incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) area-Windows Windows-specific issues and feature requests type: bug and removed type: bug labels Apr 8, 2019
bazel-io pushed a commit that referenced this issue Apr 9, 2019
Incompatible flag: #7974
See #7958

RELNOTES[NEW]: Windows, Python: the --incompatible_windows_escape_python_args flag (false by default) builds py_binary and py_test targets with correct command line argument escaping.

Change-Id: I789f9370e2cf59fa1a179716ca1c6ad80e1d583e

Closes #7973.

Change-Id: I789f9370e2cf59fa1a179716ca1c6ad80e1d583e
PiperOrigin-RevId: 242628695
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue May 10, 2019
Follow-up to PR bazelbuild#7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: bazelbuild#7974
See bazelbuild#7958
bazel-io pushed a commit that referenced this issue May 10, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 13, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 17, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 17, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 20, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 21, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 22, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 23, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 23, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
aehlig pushed a commit that referenced this issue May 24, 2019
Follow-up to PR #7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: #7974
See #7958

Closes #8289.

PiperOrigin-RevId: 247596101
laszlocsomor added a commit that referenced this issue May 28, 2019
See #7974

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue May 28, 2019
CI test results (Bazel@HEAD + Downstream):
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1003

The flag affects Windows only, and on Windows only
two projects are broken: rules_foreign_cc and
rules_nodejs.

These are also broken on other platforms, and in
the previous build:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1002

Therefore I'm reasonably sure they are not broken
by this PR. That said, we can't completely be
sure because the tests didn't run.

See bazelbuild#7974

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
bazel-io pushed a commit that referenced this issue May 29, 2019
CI test results (Bazel@HEAD + Downstream):
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1003

The flag affects Windows only, and on Windows only
two projects are broken: rules_foreign_cc and
rules_nodejs.

These are also broken on other platforms, and in
the previous build:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1002

Therefore I'm reasonably sure they are not broken
by this PR. That said, we can't completely be
sure because the tests didn't run.

See #7974

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7

Closes #8478.

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
PiperOrigin-RevId: 250437116
irengrig pushed a commit to irengrig/bazel that referenced this issue Jun 18, 2019
Follow-up to PR bazelbuild#7973

Copy the --incompatible_windows_escape_python_args
value also to the host config options.

Incompatible flag: bazelbuild#7974
See bazelbuild#7958

Closes bazelbuild#8289.

PiperOrigin-RevId: 247596101
irengrig pushed a commit to irengrig/bazel that referenced this issue Jun 18, 2019
CI test results (Bazel@HEAD + Downstream):
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1003

The flag affects Windows only, and on Windows only
two projects are broken: rules_foreign_cc and
rules_nodejs.

These are also broken on other platforms, and in
the previous build:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1002

Therefore I'm reasonably sure they are not broken
by this PR. That said, we can't completely be
sure because the tests didn't run.

See bazelbuild#7974

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7

Closes bazelbuild#8478.

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
PiperOrigin-RevId: 250437116
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
CI test results (Bazel@HEAD + Downstream):
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1003

The flag affects Windows only, and on Windows only
two projects are broken: rules_foreign_cc and
rules_nodejs.

These are also broken on other platforms, and in
the previous build:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1002

Therefore I'm reasonably sure they are not broken
by this PR. That said, we can't completely be
sure because the tests didn't run.

See bazelbuild#7974

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7

Closes bazelbuild#8478.

Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
PiperOrigin-RevId: 250437116
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Aug 6, 2019
The flag was flipped in Bazel 0.27.0.

Removing the flag, the code path for "false"
value, and tests.

Fixes bazelbuild#7974

RELNOTES[INC]: Python, Windows: the --[no]incompatible_windows_escape_python_args is no longer supported. (It was flipped to true in Bazel 0.27.0)
bazel-io pushed a commit that referenced this issue Aug 6, 2019
The flag was flipped in Bazel 0.27.0.

Removing the flag, the code path for "false"
value, and tests.

Fixes #7974

RELNOTES[INC]: Python, Windows: the --[no]incompatible_windows_escape_python_args is no longer supported. (It was flipped to true in Bazel 0.27.0)

Closes #9088.

PiperOrigin-RevId: 261880550
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    CI test results (Bazel@HEAD + Downstream):
    https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1003

    The flag affects Windows only, and on Windows only
    two projects are broken: rules_foreign_cc and
    rules_nodejs.

    These are also broken on other platforms, and in
    the previous build:
    https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1002

    Therefore I'm reasonably sure they are not broken
    by this PR. That said, we can't completely be
    sure because the tests didn't run.

    See bazelbuild/bazel#7974

    Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7

    Closes #8478.

    Change-Id: I2cc09a4e9295495062dc4ac79b859dec6512b1b7
    PiperOrigin-RevId: 250437116
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 incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants