chipsenkbeil / distant

🚧 (Alpha stage software) Library and tooling that supports remote filesystem and process operations. 🚧
https://distant.dev
578 stars 12 forks source link

Support `shell` parameter on spawning a process #212

Closed chipsenkbeil closed 1 year ago

chipsenkbeil commented 1 year ago

If boolean, true would spawn within a shell such as sh -c "...". Would use a default shell detected.

If string, would spawn using the specified shell such as shell = "sh -c".

We also want to switch distant spawn to use the shell by default, with --no-shell available to disable its usage.

chipsenkbeil commented 1 year ago

Going to write down the approach. I was originally going to parse the command, determine how to escape and quote the arguments, and execute using something like cmd.exe /S /K "...", powershell -Command '...', csh -c '...', etc. but it becomes complicated and not scalable as if unknown shells appear it becomes more complicated.

Instead, I'm going to run an external process using the shell command where stdin is used to feed in the command.

  1. I need to determine if we're on windows or linux in order to provide either \n or \r\n to execute the command
  2. I need to still know how to exit the process. I think I can just trigger EOF (similar to Ctrl-D) and all shells should be expected to exit once the command and output is complete, but this means that spawning a process with a shell wouldn't let me continue to send stdin. What I think this means is that the protocol SHOULDN'T except a shell parameter. Instead, this is up to the CLI to provide and handle in its own way.
chipsenkbeil commented 1 year ago

An alternative approach that runs the command, including converting spawn --shell -- echo 'hello world' into spawn -- sh -c 'echo \'hello world\'' and equivalent.

chipsenkbeil commented 1 year ago

I think I'll actually do the above with the quoting because it's simpler than I first thought, but I'll be doing it at the CLI level and not the protocol level.

The idea is to have a CLI argument --shell that takes an optional argument that represents the program to run. We parse that like discussed earlier using typed-path to determine the program and handle accordingly. If the program is not known, there are two approaches:

  1. We blindly try to run shell <cmd> hoping that it will work
  2. We fail with an unsupported shell error

My thought is the latter as it's better to be clear about what is supported and not have unexpected failures. The user can always manually create the same effect by writing out the shell usage themselves.

chipsenkbeil commented 1 year ago

A few additional shells to support that are odd:

  1. elvish (because we support generating a config). The '\'' approach for single quotes doesn't work. We could check for the presence of single quotes and error in advance. Looking into how to escape these.
  2. rc (part of plan 9).
  3. nu shell, which does not support the '\'' or any other means to make single quotes work. For this, we can use the alternative backtick approach, but that still does not solve the problem of allowing any character. For this, we can just fail?

Zsh, rc, and elvish support RC_QUOTES where you can repeat a single quote to escape it (just like powershell).

chipsenkbeil commented 1 year ago

Implementation now appears to work when testing with a version of distant running locally

image

cmd.exe also works remotely, but powershell does not due to how we are splitting the command string. To result that, we need to update distant-local to detect powershell and split in a different way.

chipsenkbeil commented 1 year ago

@fabiomcosta this is almost done. Got to get powershell to work.

I don't plan to make shell execution the default for a couple of reasons, but it'll be an option you can specify in the plugin once this is rolled out.

chipsenkbeil commented 1 year ago

Resolved by https://github.com/chipsenkbeil/distant/commit/0efb5aee4c3a513012554b2fee048373101313a6.

github-actions[bot] commented 1 year ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.