Expose force_quotes on Windows. #77728

lygstate · 2020-10-08T22:30:23Z

On Windows, the arg quotes and not quotes have different effect
for the program it called, if the program called are msys2/cygwin program.
Refer to
msys2/MSYS2-packages#2176

This also solve the issues comes from

https://internals.rust-lang.org/t/std-process-on-windows-is-escaping-raw-literals-which-causes-problems-with-chaining-commands/8163

Tracking issue:
#82227

rust-highfive · 2020-10-08T22:30:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

library/std/src/sys/windows/ext/process.rs

Dylan-DPC-zz · 2020-10-31T02:48:05Z

not sure who is a good candidate to review this

r? @Mark-Simulacrum

Mark-Simulacrum · 2020-11-05T20:53:00Z

r? @Amanieu perhaps, though not sure. May be worth pinging the windows group.

jyn514 · 2020-11-24T03:28:38Z

@retep998 what do you think?

sivadeilra · 2020-12-18T19:19:45Z

I'll take a look.

sivadeilra

In principle, the PR makes sense. There are two things that concern me, though:

Since the feature is exposed by defining a new function on an existing trait, this could be a breaking change. I know it's an obscure case, but still, the Rust Project aims to provide an objective definition of non-breaking changes.
This feature works on an entire command-line, rather than individual args. So this solves your current need, but it's easy to imagine a case in the future where you wanted to pass an arg string like "foo*.c" bar*, which would not be possible with this PR.

Perhaps a better approach would be to allow the caller to provide a verbatim String that provides exactly the arg that is passed to CreateProcess. That way, callers who know they're on Windows and know that they need control over argument parsing can provide exactly the string that they want.

sivadeilra · 2020-12-18T19:25:25Z

library/std/src/sys/windows/ext/process.rs

+    /// In msvcrt based program, the commandline with and without quotes have
+    /// the same effect.
+    /// Refer to https://msdn.microsoft.com/en-us/library/17w5ykft.aspx
+    #[unstable(feature = "windows_process_extensions_force_quotes", issue = "77728")]


It's unlikely but possible that this could be a breaking change. If someone implemented CommandExt in a separate crate, then adding this new associated function will break that implementation. I know, it's really unlikely, but still.

This could be prevented by either 1) moving force_quotes to a new trait, or 2) providing a default implementation of force_quotes which did nothing, but returned self. I'm not sure that's possible, though, given how the function signature is defined.

Does trait are possible to implement defaut implementation?

@lygstate Traits can have default implementations for methods, if that's what you mean.

@lygstate For example, you can do this:

pub trait Foo { fn foo(&self); // <-- has no default fn bar(&self) { println!("I am the default behavior"); } }

It's impossible to implement default implementation

#[unstable(feature = "windows_process_extensions_force_quotes", issue = "77728")] fn force_quotes(&mut self, enabled: bool) -> &mut process::Command { }

Well, if there's no reasonable default implementation that satisfies the expected behavior, then it doesn't make sense to have a default implementation.

However in this case you can probably just return self as @sivadeilra suggested. Having an empty body is definitely not going to work though.

@camelid I am sorry for that, return self are also not working and I've tried.
It's trait and not inherit from process::Command, so return self not working

Seal the CommandExt, OsStrExt and OsStringExt traits A crater run (rust-lang#81213 (comment)) has shown that this does not break any existing code. This also unblocks rust-lang#77728. Based on rust-lang#81213. r? ``@m-ou-se`` cc ``@lygstate``

Amanieu

Now that #81975 is merged, we can move forward with this.

library/std/src/sys/windows/ext/process.rs

Amanieu · 2021-02-17T16:41:48Z

@bors r+

bors · 2021-02-17T16:41:49Z

📌 Commit c38648afe61e6f84d7e3859a53c67e84bcb505fa has been approved by Amanieu

Amanieu · 2021-02-17T16:42:14Z

@bors r-

Amanieu · 2021-02-17T16:54:34Z

You need to create a tracking issue for this feature and update the unstable attribute to point to it.

lygstate · 2021-02-17T17:08:37Z

You need to create a tracking issue for this feature and update the unstable attribute to point to it.

Hi, any example?

m-ou-se · 2021-02-17T17:25:19Z

You can use this template for the tracking issue.

Quotes the arg and not quotes the arg have different effect on Windows when the program called are msys2/cygwin program. Refer to msys2/MSYS2-packages#2176 Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>

Amanieu · 2021-02-17T18:20:30Z

@bors r+

bors · 2021-02-17T18:20:31Z

📌 Commit fa23ddf has been approved by Amanieu

Rollup of 8 pull requests Successful merges: - rust-lang#77728 (Expose force_quotes on Windows.) - rust-lang#80572 (Add a `Result::into_ok_or_err` method to extract a `T` from `Result<T, T>`) - rust-lang#81860 (Fix SourceMap::start_point) - rust-lang#81869 (Simplify pattern grammar, improve or-pattern diagnostics) - rust-lang#81898 (Fix debug information for function arguments of type &str or slice.) - rust-lang#81972 (Placeholder lifetime error cleanup) - rust-lang#82007 (Implement reborrow for closure captures) - rust-lang#82021 (Spell out nested Self type in lint message) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned withoutboats Oct 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020

camelid added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 8, 2020

camelid reviewed Oct 8, 2020

View reviewed changes

library/std/src/sys/windows/ext/process.rs Outdated Show resolved Hide resolved

lygstate force-pushed the master branch from 7e74bf1 to 2a9c916 Compare October 9, 2020 03:54

camelid mentioned this pull request Oct 9, 2020

On windows, the process need a option to quote all parameter. #77734

Closed

lygstate force-pushed the master branch 8 times, most recently from 92c8614 to 88e3da1 Compare October 9, 2020 13:24

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2020

rust-highfive assigned Mark-Simulacrum and unassigned withoutboats Oct 31, 2020

rust-highfive assigned Amanieu and unassigned Mark-Simulacrum Nov 5, 2020

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2020

sivadeilra reviewed Dec 18, 2020

View reviewed changes

lygstate force-pushed the master branch from 88e3da1 to 88e39b0 Compare December 23, 2020 06:06

Amanieu mentioned this pull request Feb 10, 2021

Seal the CommandExt, OsStrExt and OsStringExt traits #81975

Merged

Amanieu reviewed Feb 16, 2021

View reviewed changes

library/std/src/sys/windows/ext/process.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

lygstate force-pushed the master branch from d2e30c9 to c38648a Compare February 16, 2021 17:00

m-ou-se removed I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 17, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 17, 2021

lygstate changed the title ~~Expose force_quotes,~~ Expose force_quotes on Windows. Feb 17, 2021

lygstate mentioned this pull request Feb 17, 2021

Tracking Issue for expose force_quotes in CommandExt on Win32 #82227

Open

3 tasks

lygstate force-pushed the master branch from c38648a to 8dc90d0 Compare February 17, 2021 17:48

Expose force_quotes on Windows.

fa23ddf

Quotes the arg and not quotes the arg have different effect on Windows when the program called are msys2/cygwin program. Refer to msys2/MSYS2-packages#2176 Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>

lygstate force-pushed the master branch from 8dc90d0 to fa23ddf Compare February 17, 2021 17:54

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 17, 2021

Dylan-DPC-zz mentioned this pull request Feb 17, 2021

Rollup of 8 pull requests #82241

Merged

bors merged commit db59950 into rust-lang:master Feb 18, 2021

rustbot added this to the 1.52.0 milestone Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose force_quotes on Windows. #77728

Expose force_quotes on Windows. #77728

lygstate commented Oct 8, 2020 •

edited

Loading

rust-highfive commented Oct 8, 2020

Dylan-DPC-zz commented Oct 31, 2020

Mark-Simulacrum commented Nov 5, 2020

jyn514 commented Nov 24, 2020

sivadeilra commented Dec 18, 2020

sivadeilra left a comment

sivadeilra Dec 18, 2020

lygstate Dec 19, 2020

camelid Dec 19, 2020 •

edited

Loading

sivadeilra Dec 21, 2020

lygstate Dec 23, 2020

camelid Dec 23, 2020

camelid Dec 23, 2020

lygstate Dec 24, 2020

Amanieu left a comment

This comment has been minimized.

Amanieu commented Feb 17, 2021

bors commented Feb 17, 2021

Amanieu commented Feb 17, 2021

Amanieu commented Feb 17, 2021

lygstate commented Feb 17, 2021

m-ou-se commented Feb 17, 2021

Amanieu commented Feb 17, 2021

bors commented Feb 17, 2021

Expose force_quotes on Windows. #77728

Expose force_quotes on Windows. #77728

Conversation

lygstate commented Oct 8, 2020 • edited Loading

rust-highfive commented Oct 8, 2020

Dylan-DPC-zz commented Oct 31, 2020

Mark-Simulacrum commented Nov 5, 2020

jyn514 commented Nov 24, 2020

sivadeilra commented Dec 18, 2020

sivadeilra left a comment

Choose a reason for hiding this comment

sivadeilra Dec 18, 2020

Choose a reason for hiding this comment

lygstate Dec 19, 2020

Choose a reason for hiding this comment

camelid Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

sivadeilra Dec 21, 2020

Choose a reason for hiding this comment

lygstate Dec 23, 2020

Choose a reason for hiding this comment

camelid Dec 23, 2020

Choose a reason for hiding this comment

camelid Dec 23, 2020

Choose a reason for hiding this comment

lygstate Dec 24, 2020

Choose a reason for hiding this comment

Amanieu left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Amanieu commented Feb 17, 2021

bors commented Feb 17, 2021

Amanieu commented Feb 17, 2021

Amanieu commented Feb 17, 2021

lygstate commented Feb 17, 2021

m-ou-se commented Feb 17, 2021

Amanieu commented Feb 17, 2021

bors commented Feb 17, 2021

lygstate commented Oct 8, 2020 •

edited

Loading

camelid Dec 19, 2020 •

edited

Loading