Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upShell escape process arguments that are printed out #1673
Conversation
added some commits
May 2, 2015
rust-highfive
assigned
huonw
Jun 2, 2015
This comment has been minimized.
This comment has been minimized.
rust-highfive
commented
Jun 2, 2015
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (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. 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. |
This comment has been minimized.
This comment has been minimized.
|
Successor of #1572. I think Windows support is still WIP. Ok, it was explained that no detection is needed. Just escape " and spaces on windows by quoting the arguments, also replace all backslashes with slashes. Unfortunately the old fail log is gone and I can't find where it failed on windows the last time, but there must be some test that needs to be updated. |
bluss
reviewed
Jun 2, 2015
| r#""--features=\"default\"""#); | ||
| assert_eq!(shell_escape(r#"\path\to\my documents\"#.into()), | ||
| r#""/path/to/my documents/""#); | ||
| } |
This comment has been minimized.
This comment has been minimized.
bluss
Jun 2, 2015
Author
Contributor
(See above) @retep998 is this the right way to escape? It adds double quotes if it finds double quotes or spaces.
This comment has been minimized.
This comment has been minimized.
retep998
Jun 2, 2015
Member
If it has spaces in it then you add double quotes around it. If it has double quotes in it, then you add backslashes. You do not need to add double quotes around it if it has double quotes in it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors: try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jun 2, 2015
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
I'd personally recommend not using Also in theory if you're using a MSYS shell on Windows you should use the same escaping as the unix shell, but that can always be implemented later. |
This comment has been minimized.
This comment has been minimized.
|
Oh that's a mistake. The intention was for both windows and other to both compile and test always -- which they do on linux. |
alexcrichton
reviewed
Jun 4, 2015
| pub use self::windows::shell_escape; | ||
|
|
||
|
|
||
| #[cfg(any(test, target_os = "windows"))] |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 4, 2015
Member
er actually I was thinking that this would remove the #[cfg] attributes in this module entirely, and instead have a top-level function that used if cfg!(...)
This comment has been minimized.
This comment has been minimized.
bluss
Jun 24, 2015
Author
Contributor
I would do this change, it makes sense, but the way I had it now, I tested both versions on any platform.
This comment has been minimized.
This comment has been minimized.
|
I'm sorry, I can't really work on something that I can't run & test (i.e. Windows support). |
This comment has been minimized.
This comment has been minimized.
|
It would be better to merge this with single platform support only, and then have another contributor pick up the windows support -- in a PR that they can develop & test on that platform. |
alexcrichton
referenced this pull request
Jun 29, 2015
Merged
Shell escape arguments in output #1770
bors
added a commit
that referenced
this pull request
Jun 29, 2015
This comment has been minimized.
This comment has been minimized.
|
The Windows bots are failing because backslashes are being translated to forward slashes, so tests need to be updated, but I'm also not 100% convinced that we can do this kind of translation as it's changing the input. Do you have a link to the documentation for Windows on how the string may be escaped? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks, I didn't use any docs, I just talked with @retep998 :) |
bluss commentedJun 2, 2015
Shell escape process arguments that are printed out
cargo build --verbose does output some command lines that cannot be
simply copy and pasted into the shell again. The problem is the
arguments which are output exactly like this: --feature="foo"
When pasted back into the shell, the shell will parse and remove the
double quotes. To counteract this, escape special shell characters when
printing commandlines. Cargo will print --feature="foo" instead, which
can be pasted back into the shell.