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_style_arg_escaping: enables correct subprocess argument escaping on Windows #7454

Closed
laszlocsomor opened this Issue Feb 18, 2019 · 5 comments

Comments

Projects
None yet
2 participants
@laszlocsomor
Copy link
Contributor

commented Feb 18, 2019

Description

The option --incompatible_windows_style_arg_escaping enables correct subprocess argument escaping on Windows. This flag has NO effect on other platforms.

When enabled, WindowsSubprocessFactory will use ShellUtils.windowsEscapeArg to escape command line arguments. This is correct, as verified by tests.

When disabled, WindowsSubprocessFactory will use ShellUtils.quoteCommandLine. This is buggy, as shown by #7122.

Migration recipe

None, as of 2019-02-18.

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

Rollout plan

  • Bazel 0.23.0 will not support this flag.
  • Bazel 0.24.0 is expected to support this flag, with default value being false.
  • Bazel 0.25.0 is expected to flip this flag to true.

@laszlocsomor laszlocsomor self-assigned this Feb 18, 2019

bazel-io pushed a commit that referenced this issue Feb 19, 2019

Windows: add --incompatible_windows_style_arg_escaping
Add the --incompatible_windows_style_arg_escaping
flag (default: false). This flag has no effect on
platforms other than Windows.

Semantics:
- When true: subprocess arguments are escaped with
  ShellUtils.windowsEscapeArg. This is the correct
  behavior.
- When false: subprocess arguments are escaped
  with ShellUtils.quoteCommandLine. This is the
  default behavior on all platforms, but it is
  incorrect on Windows.

Incompatible flag: #7454

Related: #7122

RELNOTES[NEW]: Added --incompatible_windows_style_arg_escaping flag: enables correct subprocess argument escaping on Windows. (No-op on other platforms.)

PiperOrigin-RevId: 234581069

irengrig added a commit to irengrig/bazel that referenced this issue Feb 26, 2019

Windows: add --incompatible_windows_style_arg_escaping
Add the --incompatible_windows_style_arg_escaping
flag (default: false). This flag has no effect on
platforms other than Windows.

Semantics:
- When true: subprocess arguments are escaped with
  ShellUtils.windowsEscapeArg. This is the correct
  behavior.
- When false: subprocess arguments are escaped
  with ShellUtils.quoteCommandLine. This is the
  default behavior on all platforms, but it is
  incorrect on Windows.

Incompatible flag: bazelbuild#7454

Related: bazelbuild#7122

RELNOTES[NEW]: Added --incompatible_windows_style_arg_escaping flag: enables correct subprocess argument escaping on Windows. (No-op on other platforms.)

PiperOrigin-RevId: 234581069

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Mar 6, 2019

Flip --incompatible_windows_style_arg_escaping
Fixes: bazelbuild#7454

RELNOTES[INC]: Flip --incompatible_windows_style_arg_escaping to true.  See bazelbuild#7454
@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Fixed by laszlocsomor@363016e. Closing issue.

@dkelmer dkelmer closed this Apr 3, 2019

@laszlocsomor

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

No, laszlocsomor@363016e was not merged! This flag is not a breaking change in 0.25

@laszlocsomor laszlocsomor reopened this Apr 4, 2019

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 5, 2019

test_wrapper_test: fix for incompatible flag
--incompatible_windows_style_arg_escaping will no
longer break this test.

see bazelbuild#7454
@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Sorry about that, thanks for updating the labels

@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I re-added breaking change 26 because that will put this bug in the queue for the next release manager to check up on it. No pressure if it doesn't get flipped, the protocol is to extend the window in that case. But I think if it doesn't have a breaking change label it will fall through the cracks eventually.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Good to know. Thank you!

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 5, 2019

test_wrapper_test: fix for incompatible flag
--incompatible_windows_style_arg_escaping will no
longer break this test.

see bazelbuild#7454

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 8, 2019

test_wrapper_test: fix for incompatible flag
--incompatible_windows_style_arg_escaping will no
longer break this test.

see bazelbuild#7454

bazel-io pushed a commit that referenced this issue Apr 8, 2019

test_wrapper_test: fix for incompatible flag
--incompatible_windows_style_arg_escaping will no
longer break this test.

see #7454

Closes #7968.

PiperOrigin-RevId: 242459218

@bazel-io bazel-io closed this in 20a78a8 Apr 8, 2019

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 11, 2019

Windows: remove incompatible_windows_style_arg_escaping
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Windows: add --incompatible_windows_style_arg_escaping
Add the --incompatible_windows_style_arg_escaping
flag (default: false). This flag has no effect on
platforms other than Windows.

Semantics:
- When true: subprocess arguments are escaped with
  ShellUtils.windowsEscapeArg. This is the correct
  behavior.
- When false: subprocess arguments are escaped
  with ShellUtils.quoteCommandLine. This is the
  default behavior on all platforms, but it is
  incorrect on Windows.

Incompatible flag: bazelbuild#7454

Related: bazelbuild#7122

RELNOTES[NEW]: Added --incompatible_windows_style_arg_escaping flag: enables correct subprocess argument escaping on Windows. (No-op on other platforms.)

PiperOrigin-RevId: 234581069

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

test_wrapper_test: fix for incompatible flag
--incompatible_windows_style_arg_escaping will no
longer break this test.

see bazelbuild#7454

Closes bazelbuild#7968.

PiperOrigin-RevId: 242459218

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Flip --incompatible_windows_style_arg_escaping
Fixes: bazelbuild#7454

RELNOTES[INC]: Flip --incompatible_windows_style_arg_escaping to true.  See bazelbuild#7454

Closes bazelbuild#7645.

Change-Id: I068de62c2d2daf92b664a1d45d80d7b2c1ffa916
PiperOrigin-RevId: 242467818

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 18, 2019

Windows: remove incompatible_windows_style_arg_escaping
In this PR:

- Removed the
  `--incompatible_windows_style_arg_escaping` flag
  and all code for its "false" case.  The flag is
  flipped to true.

- In WindowsProcessesTest, the
  testDoesNotQuoteArgWithDoubleQuote method is
  replaced by testQuotesArgWithDoubleQuote, i.e.
  the test logic is reversed. The new test shows
  the correct behavior, the old test was wrong.

See bazelbuild#7122
See bazelbuild#7454

RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.