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

Strings are not passed as they are to externals #4601

Closed
elferherrera opened this issue Feb 22, 2022 · 13 comments · Fixed by #5846
Closed

Strings are not passed as they are to externals #4601

elferherrera opened this issue Feb 22, 2022 · 13 comments · Fixed by #5846
Labels
upstream problem with upstream dependency
Milestone

Comments

@elferherrera
Copy link
Contributor

Describe the bug

The next command errors because the string after the filter is not passed as it is

gcloud dataproc jobs list --region europe-west1 --limit=50 --filter="labels.table_name=vagabond_tranches" --sort-by="statusHistory[0].stateStartTime"

The error received from the external is this one

ERROR: (gcloud.dataproc.jobs.list) INVALID_ARGUMENT: Could not parse the filter: Unexpected property ''. Expected one of [labels, clusterName, placement.clusterName, status.state, operationType]

How to reproduce

run external command that requires an argument with string

Expected behavior

The string is passed correctly to the argument

Screenshots

No response

Configuration

No response

Additional context

No response

@elferherrera
Copy link
Contributor Author

It seems this is not related to nushell but to rust itself

This issue describes it and there is a nightly feature waiting for approval.

I'm closing this issue but leaving this for future reference

@sophiajt
Copy link
Contributor

Let's leave this open and tag it upstream. We may have to work with the Rust teams to help finish it.

@sophiajt sophiajt reopened this Feb 23, 2022
@sophiajt sophiajt added the upstream problem with upstream dependency label Feb 23, 2022
@sophiajt
Copy link
Contributor

@elferherrera - the issue you linked to is marked Windows-specific. Seems like it would fix the issue you saw on Windows, but you originally saw it on macOS too right?

@elferherrera
Copy link
Contributor Author

I see the issue with externals in MacOS and Windows operating system. I linked to that issue for reference with scaped quotes

@JCallicoat
Copy link

I just found in a another issue that nu seems to be passing the double quotes literally to external commands that have flags with an = (#4609 (comment)) which seems like it could be related. Could you try your command with spaces on those flags instead of equals, i.e., --filter "labels.table_name=vagabond_tranches" --sort-by "statusHistory[0].stateStartTime" and see if it works?

@JCallicoat
Copy link

Simple reproducer to show the quotes being passed through to the command:

// foo.c
#include <stdio.h>
int main(int argc, char **argv) {
    for (int i=1; i < argc; i++) {
        puts(argv[i]);
    }
}
gcc -o foo foo.c

./foo --filter="labels.table_name=vagabond_tranches"
--filter="labels.table_name=vagabond_tranches"

Compared to zsh:

./foo --filter="labels.table_name=vagabond_tranches"
--filter=labels.table_name=vagabond_tranches

@elferherrera
Copy link
Contributor Author

@JCallicoat I can confirm that by adding a space the external command works

@elferherrera
Copy link
Contributor Author

@jntrnr @fdncred Should we add a config flag that removes quotes from arguments to externals to behave like bash?

@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2022

Should we add a config flag that removes quotes from arguments to externals to behave like bash?

imo, passing arguments to externals should "just work". we continually have issues with it. I'd like to resolve it once and for all. I'm not sure what that resolution looks like though.

@elferherrera
Copy link
Contributor Author

The thing is that bash and zsh does thing differently. Bash removes all quotes before passing it to the external, zsh does it sometimes

@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2022

I'm willing to try just about anything in order to quit having complaints about external parameters not working.

@elferherrera
Copy link
Contributor Author

elferherrera commented Jun 21, 2022

In that case I would suggest to go the bash route. It removes all quotes from the arguments to externals. For example, if I call this external like this in bash

arg_tester dataproc jobs list --region europe-west1 --limit=50 --filter="labels.table_name=vagabond_tranches && labels=something" --sort-by="statusHistory[0].stateStartTime"

it is received by the external like this

["./target/debug/arg_tester", "dataproc", "jobs", "list", "--region", "europe-west1", "--limit=50", "--filter=labels.table_name=vagabond_tranches && labels=something", "--sort-by=statusHistory[0].stateStartTime"]

Notice that the quotes for --filter and --sort-by are removed after the equal sign

@fdncred
Copy link
Collaborator

fdncred commented Jun 21, 2022

Sounds good to me. I'm willing to try that and see how it goes.

@hustcer hustcer added this to the v0.65 milestone Jun 21, 2022
IanManske pushed a commit that referenced this issue May 23, 2024
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:

```nu
let foo = "'bar'"
^touch $foo            # This creates a file named `bar`, but it should be `'bar'`
^touch ...[ "'bar'" ]  # Same
```

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-ins

On Windows, `.bat` files and `.cmd` files are considered executable, but
they need `CMD.exe` as the interpreter. The Rust standard library
supports running `.bat` files directly and will spawn `CMD.exe` under
the hood (see
[documentation](https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting)).
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()](https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.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` feature

The `which` crate is now a hard dependency of `nu-command`, making the
`which-support` feature essentially useless. The `which` crate is
already a hard dependency of `nu-cli`, and we should consider removing
the `which-support` feature entirely.

## Appendix

Here's a list of quoting-related issues and PRs in rough chronological
order.

* #4609
* #4631
* #4601
  * #5846
* #5978
  * #6014
* #6154
  * #6161
* #6399
  * #6420
  * #6426
* #6465
* #6559
  * #6560

[^1]: The idea that backtick-quoted strings act like bare strings was
introduced by Kubouch and briefly mentioned in [the language
reference](https://www.nushell.sh/lang-guide/chapters/strings_and_text.html#backtick-quotes).

[^2]: 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.

[^3]: If anyone wants to try, the best resource I found on the topic is
[this](https://daviddeley.com/autohotkey/parameters/parameters.htm).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream problem with upstream dependency
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants