-
Notifications
You must be signed in to change notification settings - Fork 137
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
Allow using the default login shell for remote connections #706
base: master
Are you sure you want to change the base?
Conversation
I changed the wildcard from "*" to t, as it feels more idiomatic. The last commit (c11336a) sets the default value of vterm-tramp-shells to |
This fixes #567 |
vterm.el
Outdated
(if (or (eq first 'login-shell) | ||
(and (consp first)(eq (cadr first) 'login-shell))) | ||
(let* ((shell (ignore-errors | ||
(with-output-to-string | ||
(with-current-buffer standard-output | ||
;; Use a shell command here to get $LOGNAME. | ||
;; Using the Tramp user does not always work | ||
;; as it can be nil, e.g., with ssh host configs | ||
(process-file-shell-command | ||
"getent passwd $LOGNAME" | ||
nil (current-buffer) nil))))) | ||
(shell (when shell | ||
(nth 6 (split-string shell ":" nil "[ \t\n\r]+"))))) | ||
(or shell second)) |
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.
Thank you for your PR (and so sorry for the very very long turnaround time...)
What assumptions are being made here, and how robust is this with respect to different possible hosts (eg, different operating systems)?
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.
Yes, I see, that's an important question.
I have updated vterm--tramp-get-shell
. Now it should be pretty robust.
(if (or (eq first 'login-shell)
(and (consp first)(eq (cadr first) 'login-shell)))
(let* ((shell (ignore-errors
(with-output-to-string
(with-current-buffer standard-output
;; Use a shell command here to get $LOGNAME.
;; Using the Tramp user does not always work
;; as it can be nil, e.g., with ssh host configs
(unless (= 0 (process-file-shell-command
"getent passwd $LOGNAME"
nil (current-buffer) nil))
(error "Unexpected return value"))
;; If we have more than one line,
;; the output is not the expected single passwd entry.
;; Most likely, $LOGNAME is not set.
(when (> (count-lines (point-min) (point-max)) 1)
(error "Unexpected output"))))))
(shell (when shell
(nth 6 (split-string shell ":" nil "[ \t\n\r]+")))))
(or shell second))
first)
$LOGNAME
should be defined in every POSIX-compliant system,
see https://pubs.opengroup.org/onlinepubs/007908775/xbd/envvar.html
If it is not defined,vterm--tramp-get-shell
will return the fallback value or the default shell ("Unexpected output" ->shell
is nil).getent passwd
returns the passwd entry for the specified user, independently from the used name service (I also tested it with LDAP),
see https://en.wikipedia.org/wiki/Getent
Thegetent
command is widely available in Unix systems [1].
However, MacOS X does not provide it. Thus for MacOS X hosts,vterm--tramp-get-shell
will return the fallback or the default value ("Unexpected return value" ->shell
is nil).
[1] from the FreeBSD manpage for getent: "A getent command appeared in NetBSD 3.0, and was imported into FreeBSD 7.0. It was based on the command of the same name in Solaris and Linux."
https://man.freebsd.org/cgi/man.cgi?query=getent
So, it works for most Unix-like remote hosts, including GNU/Linux and the BSDs.
For MacOS X, the probing fails (since getent
is missing), but the function should return the fallback value (not tested, since I have no Mac).
I don't know about Windows.. I guess with the Linux subsystem it should work.
If you deem it too unstable, I would argue for changing the default value of vterm-tramp-shells
back to the original value but include the 'login-shell functionality as an optional feature.
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.
Thank you.
I think we can go ahead with this. Here are some comments to make the solution more robust (and cleanup a few things):
- Tramp supports all sorts of protocols and systems. While I expect ssh to be the most common (by far), we should not make too many assumptions about it. I suggest we turn the
login-shell
method you introduce to be the default forssh
, but let the user pick it for other protocols if they want to. - In this vain, I think it would be nice to check that we have a corresponding entry in
vterm-tramp-shells
for the method being used and error out when we don't - Restrictions and assumptions should be documented in the code (as you did in the previous message). More explanation is welcomed too (e.g., why
(nth 6
)? You could paste an example so that readers would konw what to expect) - Please, format your code and check that docstrings are complete and accurate, including defining all the arguments function take
- Please, add an entry to the README.
Thank you again!
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.
I have updated the PR.
Now, there is extensive in-code documentation and a extended docstrings. I also added a README entry with examples.
I suggest we turn the login-shell method you introduce to be the default for
ssh
I set it to default for ssh
and scp
, since scp
are ssh servers in most cases. What do you think?
In this vain, I think it would be nice to check that we have a corresponding entry in vterm-tramp-shells for the method being used and error out when we don't
I am not exactly sure what you mean by that.
Lets the user specify 'login-shell as a SHELL in `vterm-tramp-shells', which leads to vterm starting with the remote location's login shell.
Lets the user specify a shell for all Tramp methods.
This will fall back to Tramp's shell if the login shell cannot be determined.
This feature would be quite useful to me and I just stumbled upon this pull request. |
I've been using @florommel's fork since March and haven't encountered any problems with it. Would like to have it merged as well. |
For remote machines, let vterm start with the default login shell of the remote host.
This can be configured by specifying a the symbol 'login-shell as SHELL in vterm-tramp-shells. The current default behavior is not changed.
In addition, allow "*" as a wildcard method in vterm-tramp-shells.
This was requested in #655