Add shell: sh

[husjon]

The sh reference in Shell_Bash.ts#L28 unfortunately does not work, in particular on NixOS which does not use traditional FHS, as such the only way to spawn a shell without knowing the path is to use /usr/bin/env bash or /usr/bin/env zsh etc.
Since this was not possible to add quickly, I instead decided to add a separate Shell for /bin/sh which is available (also on most if not all Linux distros and MacOS).

In short, this PR moves the sh reference to a separate Shell (Shell_Sh.ts) to allow it to be selected separately.

[Taitava]

Hi. Thank you for your suggestion!

The sh reference in Shell_Bash.ts#L28 unfortunately does not work

What do you mean by does not work? Does it give an error message you could share?

Does the shell work if you select Use system default? The sh reference was added in issue #281 and it aimed to make the system default choice work, but I can't really remember how shortly or extensively I tested it. Plus there was some refactoring done afterwards that might have affected it.

---

I'll need to consider this a bit. It might make sense to make sh an explicit option like you are suggesting. Or I could create a Shell_OperatingSystemDefault class and make it work for any path that comes from the SHELL (Linux, macOS) or ComSpec (Windows) environment variables. Or do both. I'll think about this.

As it's already possible to define custom shells that allow you to specify the shell's path freely, I don't see this as an urgent thing, but I do see that it would be an easier option, especially for new users.

---

For this to be merged, I'd like all the commit messages to be prefixed with an issue number. And as this doesn't have an issue yet, it would be good to go the normal feature request route and create a discussion for this, after which I can create an issue if I'll decide to merge this code. I haven't received many pull requests yet, so I haven't really polished up the PR process yet. I guess I'll need to write some instructions to recommend creating a discussion before a pull request 🙂.

[husjon]

Hi I might have been a bit too quick when writing the initial PR comment but I'll answer your questions below. :)

---

What do you mean by does not work? Does it give an error message you could share?

The error I get is the following: Shell command failed to execute. Error: spawn /bin/bash ENOENT (this is with the original released plugin)

Does the shell work if you select Use system default?

I've always used the Use System Default option so that unfortunately didn't change anything.

---

I'll need to consider this a bit. It might make sense to make sh an explicit option like you are suggesting. Or I could create a Shell_OperatingSystemDefault class and make it work for any path that comes from the SHELL (Linux, macOS) or ComSpec (Windows) environment variables. Or do both. I'll think about this.

I think having the possibility for selecting the shell explicitly, especially the usual system defaults (as in shells that usually comes with systems) like sh, bash, zsh and dash is quite a safe choice for at least Linux and Mac (I haven't used Windows in many years so I can't testify for it).
My current shell of choice is Fish, but due to the non-POSIX compliance it's hard to argue for it in a case like this :)

In the case of Bash and the Shell_Bash.ownedShellBinaries array, it seem to only be referenced in BuiltinShell.matchesIdentifier, this seem to unfortunately only checks if a Shell owns a binary from within the array, i.e sh, it would then still be using /bin/bash as the shell executable as defined in Shell_Bash.getBinaryPath().

As it's already possible to define custom shells that allow you to specify the shell's path freely, I don't see this as an urgent thing, but I do see that it would be an easier option, especially for new users.

I did see that this was possible but didn't look further into it, I could take a look at how to define one.

One really nice way of handling this would be to allow the shell to be run using /usr/bin/env <shell of choice> (e.g: /usr/bin/env bash).
This way as long as the shell is part of the environment (specified / overridden by the PATH environment variable), it will choose that executable.

This is for example used in Python Virtual Environments.
The typical shebang of a executable python file usually looks like this:

#!/usr/bin/env python

Then when you're using the virtual environment, PATH is prepended with that virtual environments python executable, this has the upside of not touching the System-level python packages.

---

For this to be merged, I'd like all the commit messages to be prefixed with an issue number.

I'll happily rebase and correct the commit messages when/if we get to a conclusion :)

[husjon]

While I was writing the comment above I was also experimenting a bit more using the main branch just to familiarize myself with the flow of the plugin.
In particular how it uses and matches on the ownedShellBinaries array.
What I found was that during the shell matching in getShell (ShellFunctions.ts), the path it identifies with is /bin/sh but the executable it's using while spawning it is still /bin/bash. (see image below)

The ChildProsess log is in ShellCommandExecutor.executeShellCommand, just after the shell has been spawned.

I might just make this PR into a fix for that instead, thoughts?

[husjon]

So I've played around a bit more, the issue I described in the above comment is still relevant, as in even though sh is matched with the Shell_Bash implementation, it is still trying to execute the shell as /bin/bash which in my case does not work.
I'm not entirely sure how to most easily split up getBinaryPath based on the identified shell, you might have some ideas if it comes to it.

I can however as you mentioned use Custom Shells and point it to /bin/sh, this does work as intended.

I'm also able to create a custom shell using /usr/bin/env bash using the following Shell arguments -S bash -c {{!shell_command_content}} (note the -S to tell /usr/bin/env to split the following parts as shebang lines)

If you do decide to accept the PR as is (to add sh as a separate explicit shell), just let me know and I'll correct the commit messages according to how you want them as you mentioned in your original comment. :)

[Taitava]

Thank you for your replies! 🙂 I didn't read it all yet as I'm in a bit of hurry now, but I'll come back to this in the coming weeks, if not this week.

In the case of Bash and the Shell_Bash.ownedShellBinaries array, it seem to only be referenced in BuiltinShell.matchesIdentifier, this seem to unfortunately only checks if a Shell owns a binary from within the array, i.e sh, it would then still be using /bin/bash as the shell executable as defined in Shell_Bash.getBinaryPath().

Yes, this is the case. The shell paths are hardcoded, and even the "Use system default" option does not really use the system's default shell path, instead it just tries to match the shell binary's name with the hardcoded paths, and then uses one of them. It's definitely confusing. Right now I can't remember how I came up with this (I could look into this), but it's definitely something that can be improved to really use the system default shell path.

[Taitava]

Hi again @husjon , and sorry for the delay! I'll answer the rest of your messages now.

I have thought about this addition of /bin/sh as a separate shell selection for Linux and macOS, and yes, let's do it 👍 . And I will probably fix the "Use system default" shell option to actually use the correct path. I haven't decided yet, will these changes happen in the same release or not, but at least the former (using your PR) could be prioritized here and planned for the next version, SC 0.24.0.

About /usr/bin/env <shell of choice>

One really nice way of handling this would be to allow the shell to be run using /usr/bin/env <shell of choice> (e.g: /usr/bin/env bash). This way as long as the shell is part of the environment (specified / overridden by the PATH environment variable), it will choose that executable.

This is for example used in Python Virtual Environments. The typical shebang of a executable python file usually looks like this:

#!/usr/bin/env python

Then when you're using the virtual environment, PATH is prepended with that virtual environments python executable, this has the upside of not touching the System-level python packages.

Hmm, this concept of /usr/bin/env <shell of choice> is new to me, but if I understood correctly, this could be a broader way to support different Linux distros that may have Bash, Dash or Zsh installed somewhere else than in /bin. Do you know if it works on macOS, too, or only on Linux?

I do like the idea. If I was now deciding what paths to use for the default shells, I would definitely try /usr/bin/env <bash|dash|zsh> before (the current) /bin/<bash|dash|zsh>. If /usr/bin/env would work for all Linux users (= I don't know this now), I'd like to use it instead of the current one. But I assume your suggestion was to add it as an alternative option? Maybe yes, I'll need to think about how to do that without cluttering up the UI.

(Btw. Do you know if using /usr/bin/env affects the performance in any way? I was just thinking that it's one step more before launching the actual shell. But maybe it's unnoticeable.)

Maybe the simplest would be to just duplicate the current Linux shell choices (and macOS, if applicable):

• Rename Bash -> Bash (/bin/bash)
• Rename Dash -> Dash (/bin/dash)
• Rename Zsh (Z shell) -> Z shell (/bin/zsh)
• Add Bash (/usr/bin/env bash)
• Add Dash (/usr/bin/env dash)
• Add Z shell (/usr/bin/env zsh)

This is not a decision yet, just thinking.

Possibly fixing the Use system default choice

So I've played around a bit more, the issue I described in the above comment is still relevant, as in even though sh is matched with the Shell_Bash implementation, it is still trying to execute the shell as /bin/bash which in my case does not work.
I'm not entirely sure how to most easily split up getBinaryPath based on the identified shell, you might have some ideas if it comes to it.

I'll probably look into this myself at some point. Maybe I'll create a class named like Shell_PlatformDefault whose getBinaryPath() will return whatever happens to be in the SHELL or ComSpec environment variable.

(I guess one idea I might originally have had with recognizing the shell (the current strange way) is to determine how special characters in {{variable}} values should be escaped, i.e. what character to use for escaping (currently backslash \ or backtick `). Now I think that maybe I don't have to know each and all shells out there, I can just assume that on Linux and macOS, the escaping can use backlash \ , and on Windows PowerShell, it can use backtick `. If there's any shell that uses different escaping, I can always add special cases later if someone reports problems. To determine the escaping system, it was needed to pick a Shell_* class that generally suits the user's shell, but ending up also using the hardcoded binary path seems to be an accident, and the real default path should have been used right from the beginning.)

• I need to create an issue about this for myself. Like I mentioned in the beginning of this comment, I haven't decided a milestone (SC version) for this yet.

Finishing up the addition of /bin/sh shell

• I have created issue Linux & macOS: Add shell /bin/sh #440 for this. (I didn't create a discussion like features would normally have, as this PR can exceptionally serve as a discussion. 🙂)
• Now you can edit the commit messages of this PR and add the #440: prefix to include the issue reference in them.
• If you want, you can also edit AUTHORS.md in this pull request and add your details as a contributor. This is completely optional.

Thank you very much for your time and effort on describing this problem and coming up with a solution! 🙏😎

[husjon]

Hi and no worries about the delay, I was able to get unblocked by creating a custom shell. :)

Hmm, this concept of /usr/bin/env is new to me, but if I understood correctly, this could be a broader way to support different Linux distros that may have Bash, Dash or Zsh installed somewhere else than in /bin. Do you know if it works on macOS, too, or only on Linux?

I did a quick search yesterday and found a few posts saying that it is available on MacOS too (one from 2017 https://apple.stackexchange.com/questions/297533/does-macos-have-usr-bin-env which also links the manpage for it), it is a different version (BSD vs GNU) but the end result is the same, which in this context is to look up the shell to run the commands as.

I can't speak for all non-FHS systems, but in in my case with NixOS it tries to bridge the gap with at least providing some bare minimum known paths, like /bin/sh and /usr/bin/env.
Everything else is handled with environment variables.
The 2 paths mentioned above would also be needed to be exposed on other non-FHS systems if they should have any chance of running "normal" apps that are not build explicitly for said system. :)

(Btw. Do you know if using /usr/bin/env affects the performance in any way? I was just thinking that it's one step more before launching the actual shell. But maybe it's unnoticeable.)

As for performance, it would call it negligible.

Single execution

• /bin/bash: 0.003 seconds
• /usr/bin/env: 0.004 seconds
  

1000 runs

• /bin/bash: 2.914 seconds
• /usr/bin/env: 3.483 seconds
  

Maybe the simplest would be to just duplicate the current Linux shell choices (and macOS, if applicable)

To reduce the need for duplication, it would be nice if for example the Bash shell (and the others) tried /usr/bin/env bash first, then /bin/bash as a fallback.
This way it would let the executables in the users environment take precedence over system executables and only use those as fallbacks.
I am however not sure how this would work on Windows since I haven't used it in quite a few years (it should work fine on MacOS).
If it is too hard to implement, adding duplicate entries as you showed also works. :)

I guess one idea I might originally have had with recognizing the shell (the current strange way) is to determine how special characters in {{variable}} values should be escaped

I found this out the other day as I was trying to use the provided variables ({{vault_path}}) and I found the values to be pre-escaped, I'm used to always wrapping variables which expands (especially ones that expands to paths) in quotation marks (") so that spaces etc are kepts as-is while keeping it human-readable.
As in, instead of /home/user/my\ special\ file\ with\ spaces.md,
it would be "/home/user/my special file with spaces.md".

I think this would also work for Windows systems as in if you wrap the variable in normal double quotation marks it should keep the string as is, but I might be wrong. :)

On Linux "$MY_VAR" would expand the MY_VAR variable, while '$MY_VAR' would not.
This would also be the case on MacOS.

Finishing up the addition of /bin/sh shell

I'll go through and rebase and update the commit messages with the issue number shortly after finishing this message.
I also added myself to AUTHORS.md.

Happy to help and thanks to you too. :)