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

Allow using the default login shell for remote connections #706

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

florommel
Copy link

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

@florommel
Copy link
Author

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 '((t login-shell)). This should try to detect the login shell for all methods and fall back to Tramp's shell if this fails.

@florommel
Copy link
Author

This fixes #567

vterm.el Outdated
Comment on lines 837 to 880
(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))
Copy link
Collaborator

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)?

Copy link
Author

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
    The getent 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.

Copy link
Collaborator

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 for ssh, 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!

Copy link
Author

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.
@nfuhler
Copy link

nfuhler commented Oct 28, 2024

This feature would be quite useful to me and I just stumbled upon this pull request.
I was wondering about its current state since it seems to have been approved but not yet merged?

@iostapyshyn
Copy link
Contributor

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.

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.

4 participants