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

Tramp-login-shells custom doesn't match type #689

Open
Thaodan opened this issue Nov 12, 2023 · 6 comments
Open

Tramp-login-shells custom doesn't match type #689

Thaodan opened this issue Nov 12, 2023 · 6 comments

Comments

@Thaodan
Copy link

Thaodan commented Nov 12, 2023

While customizing tramp-login-shells I have noticed that the set custom type doesn't match the format of the variable used in vterm.

For example this is the warning my Emacs showed me:

⛔ Warning (emacs): Value ‘(("docker" "/bin/sh") ("ssh" "/bin/zsh"))’ does not match type (alist :key-type string :value-type string)

My configuration:

(use-package vterm
  :bind (:map vterm-mode-map
              ("C-s" . isearch-forward))
  :config
  (setq vterm-max-scrollback 100000)
  ;; Include the title in vterm and multi-vterm buffers
  ;; setting multi-vterm-buffer-name isn't enough.
  (setq vterm-buffer-name-string "*vterm %s*")
  (setopt vterm-tramp-shells '(("docker" "/bin/sh")
                               ("ssh" "/bin/zsh"))))
@Alf0nso
Copy link

Alf0nso commented Apr 29, 2024

@Thaodan
I Think this is can be fixed if you change setopt to setq instead:

(use-package vterm
  :ensure t
  :config
  (setq vterm-tramp-shells '(("docker" "/bin/sh")
			     ("ssh" "/bin/bash"))))

At least for me this worked...

@Thaodan
Copy link
Author

Thaodan commented Apr 29, 2024

That sounds more like a workaround than a fix.

@Alf0nso
Copy link

Alf0nso commented Apr 29, 2024

Yes you are completely right, it is definitely not the fix...
I think maybe the "bug" here is that the definition of vterm-tramp-shells is written has:

(defcustom vterm-tramp-shells '(("docker" "/bin/sh"))
  "The shell that gets run in the vterm for tramp.

`vterm-tramp-shells' has to be a list of pairs of the format:
\(TRAMP-METHOD SHELL)"
  :type '(alist :key-type string :value-type string)
  :group 'vterm)

and should be:

(defcustom vterm-tramp-shells '(("docker" "/bin/sh"))
  "The shell that gets run in the vterm for tramp.

`vterm-tramp-shells' has to be a list of pairs of the format:
\(TRAMP-METHOD SHELL)"
  :type '(alist (:key-type string :value-type string))
  :group 'vterm)

At least when I do this change on the source code, this works:

  (use-package vterm
    :ensure t
    :config
    (setopt vterm-tramp-shells '(("ssh" "/bin/bash")
				 ("docker" "/bin/sh")))
    )

But I must confess I have no idea if this is right

@Sbozzolo
Copy link
Collaborator

I am not familiar with setopt, if you can provide an example from some other high-profile package that uses this pattern, we can implement the proposed fix to vterm.

@Thaodan
Copy link
Author

Thaodan commented May 20, 2024

setopt is basically setf but with check against custom types.

@Alf0nso
Copy link

Alf0nso commented May 20, 2024

@Sbozzolo I have to confess that I am also not very familiar with setopt, normally I just use setq for everything.
Like @Thaodan mentioned, indeed the setopt keyword makes use of both the types (it "type checks" what we pass it) and also a setter function in case it is defined gnu emacs manual.
In this specific case when we use setopt we are type checking against:

:type '(alist :key-type string :value-type string)

I played around a little bit more with defcustom and setopt and I think I might regress on my previous suggestion. I would like to ask for @Thaodan to try to write:

(use-package vterm
  :bind (:map vterm-mode-map
              ("C-s" . isearch-forward))
  :config
  (setq vterm-max-scrollback 100000)
  ;; Include the title in vterm and multi-vterm buffers
  ;; setting multi-vterm-buffer-name isn't enough.
  (setq vterm-buffer-name-string "*vterm %s*")
  (setopt vterm-tramp-shells '(("docker" . "/bin/sh")
                               ("ssh" . "/bin/zsh"))))

to see if it works without the warning. If yes, than the only thing that maybe should be changed is the default value of the defcustom variable. Meaning:

(defcustom vterm-tramp-shells '(("docker" "/bin/sh"))

to:

(defcustom vterm-tramp-shells '(("docker" . "/bin/sh"))

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

No branches or pull requests

3 participants