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

If current buffer is a TRAMP buffer, use its filename for SSH tunnel #3264

Merged
merged 3 commits into from
Dec 16, 2022

Conversation

OknoLombarda
Copy link
Contributor

@OknoLombarda OknoLombarda commented Nov 5, 2022

#3261
#2822

If current buffer is a TRAMP buffer, use its filename for SSH tunnel. This way, username and SSH port will be parsed from existing SSH connection, instead of using current username and default SSH port.

I'll edit changelog and manual (if needed) after review


  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@OknoLombarda OknoLombarda changed the title If current buffer is a TRAMP buffer, use it's filename for SSH tunnel If current buffer is a TRAMP buffer, use its filename for SSH tunnel Nov 8, 2022
@bbatsov
Copy link
Member

bbatsov commented Nov 23, 2022

The proposed change seems reasonable. Probably we should also document this behavior as it might not be obvious to people.

@bbatsov
Copy link
Member

bbatsov commented Dec 2, 2022

Ping :-)

@OknoLombarda
Copy link
Contributor Author

Yeah, I didn't forget :D. Didn't have much time lately. I added comments, but doesn't this line in docs already imply that CIDER takes care of SSH tunneling if user works with remote files over TRAMP? Or should I describe what exactly it does?
image
https://docs.cider.mx/cider/basics/up_and_running.html#working-with-remote-hosts

@bbatsov
Copy link
Member

bbatsov commented Dec 3, 2022

I was thinking it might be useful to explain in a bit more details what the TRAMP integration does and what exactly people can do to influence what SSH connection gets established. Maybe cover some edge cases, etc. The current docs are definitely very basic, and I myself never use TRAMP, so I didn't bother to play with this and document it better.

@OknoLombarda
Copy link
Contributor Author

While checking some cases I found that this change would make cider-connect-* ignore host that was selected by user and still connect to the one of current TRAMP buffer. I guess TRAMP connection should only be taken into account if its host is the same as the one selected by user. Like so (diff with master)

 (defun nrepl--ssh-tunnel-connect (host port)
   "Connect to a remote machine identified by HOST and PORT through SSH tunnel."
   (message "[nREPL] Establishing SSH tunneled connection to %s:%s ..." host port)
-  (let* ((remote-dir (if host (format "/ssh:%s:" host) default-directory))
+  (let* ((current-buf (buffer-file-name))
+         (tramp-file-regexp "/ssh:\\(.+@\\)?\\(.+?\\)\\(:\\|#\\).+")
+         (remote-dir (cond
+                      ;; If current buffer is a TRAMP buffer and its host is
+                      ;; the same as HOST, reuse its connection parameters for
+                      ;; SSH tunnel.
+                      ((and (string-match tramp-file-regexp current-buf)
+                            (string= host (match-string 2 current-buf))) current-buf)
+                      ;; Otherwise, if HOST was provided, use it for connection.
+                      (host (format "/ssh:%s:" host))
+                      ;; Use default directory as fallback.
+                      (t default-directory)))
          (ssh (or (executable-find "ssh")
                   (error "[nREPL] Cannot locate 'ssh' executable")))
          (cmd (nrepl--ssh-tunnel-command ssh remote-dir port))
@@ -598,11 +609,12 @@ If NO-ERROR is non-nil, show messages instead of throwing an error."
     ;; forwarding is set up, which is used to synchronise on, so that
     ;; the port forwarding is up when we try to connect.
     (format-spec
-     "%s -v -N -L %p:localhost:%p %u'%h'"
+     "%s -v -N -L %p:localhost:%p %u'%h' %n"
      `((?s . ,ssh)
        (?p . ,port)
        (?h . ,v-host)
-       (?u . ,(if v-user (format "-l '%s' " v-user) ""))))))
+       (?u . ,(if v-user (format "-l '%s' " v-user) ""))
+       (?n . ,(if v-port (format "-p '%s' " v-port) ""))))))
 
 (autoload 'comint-watch-for-password-prompt "comint"  "(autoload).")

I'd then change the documentation like this:

 You can safely run `cider-jack-in-*` while working with remote files over TRAMP. CIDER
-will handle this use-case transparently for you.
+will reuse existing SSH connection's parameters (like port and username) for establishing SSH tunnel.
+The same will happen if you try to `cider-connect-*` to a host that matches the one you're currently
+connected to.
 
 == Connecting via unix domain file socket

To be honest, I don't know what else can be written about it. I don't use TRAMP that much, too. I just decided to try it out because REPL starts too slow on my laptop :D

@bbatsov
Copy link
Member

bbatsov commented Dec 5, 2022

I guess TRAMP connection should only be taken into account if its host is the same as the one selected by user.

To be honest, I don't know what else can be written about it. I don't use TRAMP that much, too. I just decided to try it out because REPL starts too slow on my laptop :D

Yeah, I get it. I mostly meant to outline what are the assumptions we're making to figure out which SSH host to target. It's fine to keep this short.

@OknoLombarda OknoLombarda force-pushed the tramp-aware-ssh-tunnel branch 4 times, most recently from 22b6bdd to 1fff9bc Compare December 10, 2022 16:31
@OknoLombarda
Copy link
Contributor Author

Well, I think that's all

@bbatsov
Copy link
Member

bbatsov commented Dec 16, 2022

Can you just rebase on top of master to address the merge conflict?

@OknoLombarda
Copy link
Contributor Author

Done

@bbatsov bbatsov merged commit b02920a into clojure-emacs:master Dec 16, 2022
caadr added a commit to caadr/cider that referenced this pull request Oct 19, 2023
Calling cider-jack-in from a tramp buffer on a ssh-remote would throw:
"error in process filter: Wrong type argument: stringp, nil"

This happened because nrepl--ssh-tunnel-connect is trying to
string-match on (buffer-file-name) which returns nil, because the
execution context is the nrepl buffer.

To work around this, we pass down the buffer from which cider-jack-in
was called.

Considerations:

- This should be reverted if the cause of this feature breaking is
  found and fixable. It seemed to have worked before,
  see: clojure-emacs#3264

- In nrepl-client.el the param name "jack-in-buffer" seems a bit too
  specific. But, as long as this workaround is its only usecase, I
  consider it reasonable for discoverability.

- This should be refactored as soon as there's a more general way to convey
  contextual information to lower level functions.

Fixes:
clojure-emacs#3541
clojure-emacs#3303 (partially)
caadr added a commit to caadr/cider that referenced this pull request Oct 19, 2023
Calling cider-jack-in from a tramp buffer on a ssh-remote would throw:
"error in process filter: Wrong type argument: stringp, nil"

This happened because nrepl--ssh-tunnel-connect is trying to
string-match on (buffer-file-name) which returns nil, because the
execution context is the nrepl buffer.

To work around this, we pass down the buffer from which cider-jack-in
was called.

Considerations:

- This should be reverted if the cause of this feature breaking is
  found and fixable. It seemed to have worked before,
  see: clojure-emacs#3264

- In nrepl-client.el the param name "jack-in-buffer" seems a bit too
  specific. But, as long as this workaround is its only usecase, I
  consider it reasonable for discoverability.

- This should be refactored as soon as there's a more general way to convey
  contextual information to lower level functions.

Fixes:
clojure-emacs#3541
clojure-emacs#3303 (partially)
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.

2 participants