-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Rewrite run_external.rs #12921
Rewrite run_external.rs #12921
Conversation
wow, complete rewrite indeed. thanks for this. i'm anxious to try it on windows. i only had one real point of concern listed above, others may see other interesting changes. i really appreciate all the comments. i'm wondering if it's a good idea to add some tests that specifically exercise the windows built-in commands, not that they run right but that we pass the arguments right to them. I do see some tests in ./crates/nu-command/tests/commands/run_external.rs but they're pretty basic. |
I'm also thinking it may be good to keep these comments somewhere I went through the supported cmd.exe built-ins and seems like these are the "have-to-have" supported syntax.
|
The original implementation was broken beyond repair. Let me show you just how broken it was. # All of these behaviors are wrong
^echo '^foo' # foo
^echo 'foo bar' # "foo bar"
^echo 'foo | bar' # "foo ^| bar"
^echo '"foo' # \"foo
^echo '\"foo' # \\\"foo
^echo '&shutdown/s' # (shuts down the PC) |
|
I meant that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up this code! Most of it looks good. Some comments:
Arguments to CMD internal commands are now "properly" escaped. It should work 99% of the time, and when it doesn't work ( |
The part about "fallback path" is no longer accurate. One of the main goal of the rewrite is getting rid of all of fallback logic (#6011 (comment)). I've added the bits that are still relavant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The general implementation looks good to me, just left a comment
So far, the only remaining concern is quote-removal, discussed here. How much should we follow Bash's behavior? Should we require the existence of |
I'd vote for whatever is most consistent. By that, I mean that, if possible, whatever we do here works the same on MacOS, Linux, and Windows. The hard part about doing what is "least surprising" is that sometimes that is in the eye of the beholder. A hard-core Linux person may interpret "least surprising" in a way that a hard-core Windows user would've never considered. So, this is always a tricky question for cross platform software. If we're consistent, we can at least say, "it always does this, on every platform", and it's an easier rule to remember. |
Maybe for now we should just match what we do for internal commands. Namely, we only do quote removal for long flags ( |
What does "not in quotes" mean? Does the first |
Here's what I've implemented based on @IanManske's description. Does this look like what you want? /// Transforms `--option="value"` into `--option=value`. `value` can be quoted
/// with double quotes, single quotes, or backticks. Only removes the outermost
/// pair of quotes after the equal sign.
fn remove_inner_quotes(arg: &str) -> Cow<'_, str> {
// Check that `arg` is a long option.
if !arg.starts_with("--") {
return Cow::Borrowed(arg);
}
// Split `arg` on the first `=`.
let splits: Vec<&str> = arg.splitn(2, '=').collect();
let [option, value] = splits.as_slice() else {
return Cow::Borrowed(arg);
};
// Remove the outermost pair of quotes from `value`.
let value = remove_quotes(value);
Cow::Owned(format!("{option}={value}"))
} |
Yes thanks, that looks good.
I was thinking of something like One last thing, you can use |
Thanks everyone for the feedback! I believe we can merge this PR now. |
Thanks! Let's give this a try 🚀 |
# Description Instead of returning an error, this PR changes `expand_glob` in `run_external.rs` to return the original string arg if glob creation failed. This makes it so that, e.g., ```nushell ^echo `[` ^echo `***` ``` no longer fail with a shell error. (This follows from #12921.)
@YizhePKU I'm not asking for changes with this comment but wanted to put this "found code" here where Microsoft talks about parsing command line options. https://github.com/microsoft/sudo/blob/90edcaa04d9f58fa4702a75c034105bae2e49aa9/sudo/src/helpers.rs#L358 and https://github.com/microsoft/sudo/blob/90edcaa04d9f58fa4702a75c034105bae2e49aa9/sudo/src/run_handler.rs#L60 This is from their just published sudo command, which is written in rust. There are lots of interesting windows bits in this helper.rs file. |
@YizhePKU I just did a git bisect and found that this PR is what caused |
To clarify, we like the new behavior where ~/some_exe
`~/some_exe`
^~/some_exe
^`~/some_exe` (And again, thanks for all of your work so far!) |
I've found that I can restore the behaviour we used to have where tilde is expanded for the command name, but the bare word detection and quote removal just feels a bit strange to me. I think this is the fault of how > let s = "\"foo\""
> ^echo $s
"foo"
> run-external echo $s
"foo"
> run-external echo "foo"
foo
> run-external echo "\"foo\""
foo And note that that doesn't currently apply to the command. I think to be consistent for now I'll end up doing that |
commented on the quoting stuff here #13001 (comment) |
# Description Fix a regression introduced by #12921, where tilde expansion was no longer done on the external command name, breaking things like ```nushell > ~/.cargo/bin/exa ``` This properly handles quoted strings, so they don't expand: ```nushell > ^"~/.cargo/bin/exa" Error: nu::shell::external_command × External command failed ╭─[entry #1:1:2] 1 │ ^"~/.cargo/bin/exa" · ─────────┬──────── · ╰── Command `~/.cargo/bin/exa` not found ╰──── help: `~/.cargo/bin/exa` is neither a Nushell built-in or a known external command ``` This required a change to the parser, so the command name is also parsed in the same way the arguments are - i.e. the quotes on the outside remain in the expression. Hopefully that doesn't break anything else. 🤞 Fixes #13000. Should include in patch release 0.94.1 cc @YizhePKU # User-Facing Changes - Tilde expansion now works again for external commands - The `command` of `run-external` will now have its quotes removed like the other arguments if it is a literal string - The parser is changed to include quotes in the command expression of `ExternalCall` if they were present # Tests + Formatting I would like to add a regression test for this, but it's complicated because we need a well-known binary within the home directory, which just isn't a thing. We could drop one there, but that's kind of a bad behavior for a test to do. I also considered changing the home directory for the test, but that's so platform-specific - potentially could get it working on specific platforms though. Changing `HOME` env on Linux definitely works as far as tilde expansion works. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib`
Another thing potentially broken by this PR #13020 |
This PR also caused #13021 |
…he parser (#13089) # Description We've had a lot of different issues and PRs related to arg handling with externals since the rewrite of `run-external` in #12921: - #12950 - #12955 - #13000 - #13001 - #13021 - #13027 - #13028 - #13073 Many of these are caused by the argument handling of external calls and `run-external` being very special and involving the parser handing quoted strings over to `run-external` so that it knows whether to expand tildes and globs and so on. This is really unusual and also makes it harder to use `run-external`, and also harder to understand it (and probably is part of the reason why it was rewritten in the first place). This PR moves a lot more of that work over to the parser, so that by the time `run-external` gets it, it's dealing with much more normal Nushell values. In particular: - Unquoted strings are handled as globs with no expand - The unescaped-but-quoted handling of strings was removed, and the parser constructs normal looking strings instead, removing internal quotes so that `run-external` doesn't have to do it - Bare word interpolation is now supported and expansion is done in this case - Expressions typed as `Glob` containing `Expr::StringInterpolation` now produce `Value::Glob` instead, with the quoted status from the expr passed through so we know if it was a bare word - Bare word interpolation for values typed as `glob` now possible, but not implemented - Because expansion is now triggered by `Value::Glob(_, false)` instead of looking at the expr, externals now support glob types # User-Facing Changes - Bare word interpolation works for external command options, and otherwise embedded in other strings: ```nushell ^echo --foo=(2 + 2) # prints --foo=4 ^echo -foo=$"(2 + 2)" # prints -foo=4 ^echo foo="(2 + 2)" # prints (no interpolation!) foo=(2 + 2) ^echo foo,(2 + 2),bar # prints foo,4,bar ``` - Bare word interpolation expands for external command head/args: ```nushell let name = "exa" ~/.cargo/bin/($name) # this works, and expands the tilde ^$"~/.cargo/bin/($name)" # this doesn't expand the tilde ^echo ~/($name)/* # this glob is expanded ^echo $"~/($name)/*" # this isn't expanded ``` - Ndots are now supported for the head of an external command (`^.../foo` works) - Glob values are now supported for head/args of an external command, and expanded appropriately: ```nushell ^("~/.cargo/bin/exa" | into glob) # the tilde is expanded ^echo ("*.txt" | into glob) # this glob is expanded ``` - `run-external` now works more like any other command, without expecting a special call convention for its args: ```nushell run-external echo "'foo'" # before PR: 'foo' # after PR: foo run-external echo "*.txt" # before PR: (glob is expanded) # after PR: *.txt ``` # Tests + Formatting Lots of tests added and cleaned up. Some tests that weren't active on Windows changed to use `nu --testbin cococo` so that they can work. Added a test for Linux only to make sure tilde expansion of commands works, because changing `HOME` there causes `~` to reliably change. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting - [ ] release notes: make sure to mention the new syntaxes that are supported
This PR is a complete rewrite of
run_external.rs
. The main goal of the rewrite is improving readability, but it also fixes some bugs related to argument handling and the PATH variable (fixes #6011).I'll discuss some technical details to make reviewing easier.
Argument handling
Quoting arguments for external commands is hard. Like, really hard. We've had more than a dozen issues and PRs dedicated to quoting arguments (see Appendix) but the current implementation is still buggy.
Here's a demonstration of the buggy behavior:
I'll describe how this PR deals with argument handling.
First, we'll introduce the concept of bare strings. Bare strings are string literals that are either unquoted or quoted by backticks 1. Strings within a list literal are NOT considered bare strings, even if they are unquoted or quoted by backticks.
When a bare string is used as an argument to external process, we need to perform tilde-expansion, glob-expansion, and inner-quotes-removal, in that order. "Inner-quotes-removal" means transforming from
--option="value"
into--option=value
..bat
files and CMD built-insOn Windows,
.bat
files and.cmd
files are considered executable, but they needCMD.exe
as the interpreter. The Rust standard library supports running.bat
files directly and will spawnCMD.exe
under the hood (see documentation). However, other extensions are not supported 2.Nushell also supports a selected number of CMD built-ins. The problem with CMD is that it uses a different set of quoting rules. Correctly quoting for CMD requires using Command::raw_arg() and manually quoting CMD special characters, on top of quoting from the Nushell side.
I decided that this is too complex and chose to reject special characters in CMD built-ins instead 3. Hopefully this will not affact real-world use cases.I've implemented escaping that works reasonably well.which-support
featureThe
which
crate is now a hard dependency ofnu-command
, making thewhich-support
feature essentially useless. Thewhich
crate is already a hard dependency ofnu-cli
, and we should consider removing thewhich-support
feature entirely.Appendix
Here's a list of quoting-related issues and PRs in rough chronological order.
Footnotes
The idea that backtick-quoted strings act like bare strings was introduced by Kubouch and briefly mentioned in the language reference. ↩
The documentation also said "running .bat scripts in this way may be removed in the future and so should not be relied upon", which is another reason to move away from this. But again, quoting for CMD is hard. ↩
If anyone wants to try, the best resource I found on the topic is this. ↩