Skip to content

Conversation

chksome
Copy link
Contributor

@chksome chksome commented Dec 5, 2023

This adds a new argument -shell and the option to print fish shell compatible output. The default is still bash/zsh compatible output.

Documentation updated to match.

Copy link
Owner

@mame mame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think it is a good feature, but I am unsure if --shell fish is a good option. Do you know of any other examples?

I checked how ssh-agent does.

  • ssh-agent -s outputs Bourne shell commands: SSH_AUTH_SOCK=/path/to/sock; export SSH_AUTH_SOCK;.
  • ssh-agent -c outputs a C-shell command: setenv SSH_AUTH_SOCK /path/to/sock.
  • If not specified, ssh-agent automatically checks if the environment variable SHELL ends with "csh".

And fish users seem to use eval (ssh-agent -c).

If we follow ssh-agent, it might be good to print setenv when --csh is given.

### 2. Modify `.bashrc` (or `.zshrc` if you are using `zsh`)
### 2. Modify your shell's rc file

Bash or Zsh
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Bash or Zsh
#### Bash or Zsh

eval $($HOME/wsl2-ssh-agent)
```

Fish
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Fish
#### Fish


```
if status is-login
$HOME/wsl2-ssh-agent | source
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be $HOME/wsl2-ssh-agent --shell fish | source ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, good catch. Will fix.


* Open the "Services" app and check that "OpenSSH Authentication Agent" service is installed and running.
* Check that `ssh your-machine` works perfect on cmd.exe or PowerShell, not on WSL2.
- Open the "Services" app and check that "OpenSSH Authentication Agent" service is installed and running.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no particular reason, please do not change these.

Copy link
Contributor Author

@chksome chksome Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, yeah, I'll fix.
EDIT: So it seems Prettier is a fan of using - for unordered lists instead of *, hence how this change sneaked in on me. I'll change it back if you want, or keep it to be "more standards compliant". I don't really care either way, but if it stays this way you'll be good to go if you decide to start using it for formatting.

@mame
Copy link
Owner

mame commented Dec 6, 2023

I have reconsidered. I don't think it's a good idea to include the --csh option in modern times. If you show me that there are already major commands that implements --shell fish option, I'll be OK.

@Hill-98
Copy link
Contributor

Hill-98 commented Dec 7, 2023

Maybe this is a good choice?

command -v export &>/dev/null && export A=1 || set A 1;:

Edit: I think we may need an option like -only-output-socket-path so that we can use export SSH_AUTH_SOCK="$(wsl2-ssh-agent -only-output-socket-path)" to be compatible with any Shell.

@chksome
Copy link
Contributor Author

chksome commented Dec 7, 2023

I have reconsidered. I don't think it's a good idea to include the --csh option in modern times. If you show me that there are already major commands that implements --shell fish option, I'll be OK.

I agree that a switch for each shell seems antiquated. I was going to do that first then changed to the current -shell argument so the argument list doesn't get really polluted. I'll be honest though, I don't know of any other application with a -shell argument. rbenv handles this with a shell name as an argument to init. Starship does the same thing. What's here now is in the same spirit, but of course with a different name. I'm open to changing it as I just kind of picked the second thing that came to mind.

@mame
Copy link
Owner

mame commented Dec 8, 2023

How about this.

  • Add --format {auto,bash,zsh,csh,fish}
  • Default is --format auto, which changes the output depending on the SHELL environment variable
    • If SHELL ends with fish, print set -x SSH_AUTH_SOCK /path/to/sock
    • if SHELL ends with csh, print setenv SSH_AUTH_SOCK /path/to/sock
    • Otherwise, print SSH_AUTH_SOCK=/path/to/sock; export SSH_AUTH_SOCK;

Just in case, I want to leave room for future extensions, such as --format bare-path to show just the path as @Hill-98 said, or --format custom:"export SSH_AUTH_SOCK=%s" to use a custom format.

@Hill-98 I like polyglot too, but I am afraid that ordinary users might be suspicious when they see the output.

mame added a commit that referenced this pull request Dec 9, 2023
Fixes #10

Co-Authored-by: chksome <chksome@protonmail.com>
@mame mame mentioned this pull request Dec 9, 2023
@mame
Copy link
Owner

mame commented Dec 9, 2023

@chksome I have created another pull request #11 with you as Co-Author. If you agree with it, I will merge it.

@chksome
Copy link
Contributor Author

chksome commented Dec 19, 2023

Better solution in #11. Closing this one.

@chksome chksome closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants