Tramp support, fixes #84 #463

FelipeLema · 2020-05-08T18:30:35Z

fixes #84

beware: this is only supported in Emacs 27 onwards. I may have used things from nightly, but I'm not certain about it

Pending items:

this pending review that has already been solved
this only works on Emacs 27, not Emacs 26. Should we add a backwards-compatibility layer ?

FelipeLema · 2020-05-08T22:34:42Z

I don't know where to look at in order to fix the checks

garyo · 2020-05-09T14:14:04Z

It would be super cool if this, or a followup to it, would enable using Windows Emacs with a Linux/WSL project (toolchain running in WSL). Currently it doesn't work because of the pathname differences; the WSL toolchain sees "/c/my/project/foo.ts" but Emacs sees "c:/my/project/foo.ts". Because of this I run an extra copy of my toolchain on Windows just for LSP. Even if this PR only supports tramp mode, maybe an extension to it would help me. Interested in your thoughts, @FelipeLema .

joaotavora · 2020-05-09T14:25:59Z

let's do it, I like the patch, but we have to conditionalize this on Emacs-27. And clean up the commit messages etc. And get someone to test. I suppose you would @garyo ?

joaotavora

Looks pretty good, I don't even think we need to do any checking on Emacs version.

But I think we need a test or two. if you can't program that test please describe how I can test this in my local machine.

eglot.el

garyo · 2020-05-09T17:00:35Z

Yes, I'll try it -- not a tramp expert, and tramp isn't needed for Windows->WSL (just filename mapping) but it would be cool to see it working.

FelipeLema · 2020-05-11T14:04:09Z

will add unit tests next, yo

FelipeLema · 2020-05-11T14:17:37Z

@garyo I don't think tramp support will help you in the short term, because this will only handle the prefix of remote files, but to clangd (or ccls?) it will still be reported as a "Windows slashed path" since it was built that way. Emacs Windows build handles path differently, and this is where the problem lies.

~~This being said, this Pull Request will show us where to modify the code to support "Windows slashed paths".~~

I don't know if these "Windows slashed paths" are part of the LSProtocol and/or you can ask a LSP server if they report URIs this way or if these "Windows slashed paths" are exclusive to clangd. Will look into it on the side. Maybe the fix will be included in this PR, although it doesn't look it's going to be this way.

See #350 (comment)

don't test remoteness on process… that doesn't make any sense

inspired from FelipeLema/emacs-counsel-gtags@dc6bc8b#diff-354f30a63fb0907d4ad57269548329e3

FelipeLema · 2020-05-11T16:11:41Z

huh? travis CI wasn't triggered?

eglot.el

FelipeLema · 2020-05-11T23:26:32Z

I activated this PR to get Travis CI to compile, although this is not working in my current setup (hint: it should). Now it just waits forever for an answer from the server.

I wanted to activate Travis to check output from the tests. Maybe get something more helpful than "eglot timed out".

joaotavora · 2020-05-11T23:30:33Z

@FelipeLema don't worry much about Travis. The important thing is to have tests running with make check not necessarily with Travis.

So we need elisp tests that check tramp-related scenarios in eglot-tests.el.

Although, it's certainly nice to have them passing in Travis, if you feel you're getting somewhere. But just so you understand the priorities, Travis CI isn't one of them. It's likely that Eglot's tests suite will move from Travis CI to something else soon.

joaotavora · 2020-05-12T14:43:39Z

Makefile

@@ -43,6 +43,7 @@ eglot-check: compile
 		-l eglot						\
 		-l eglot-tests						\
 		--eval '(setq ert-batch-backtrace-right-margin 200)'	\
+		--eval '(setq default-directory (concat "/ssh:localhost:" default-directory))'	\


Why do we need this? We don't want to run all the tests through a loopback ssh Tramp connection. We just want to do that maybe, for a couple (or more) of tests that are related to Tramp. What if the user running the tests doesn't have the ssh program?

we don't need this: I'm leveraging Travis to run the tests for me (setups several LSP servers). I wanted to keep this as draft PR, but Travis stopped working.

see my comments above as I've explained

So what you're saying is that your idea is to run all the tests in a Tramp setting?

joaotavora · 2020-05-12T14:45:23Z

eglot-tests.el

@@ -69,7 +69,11 @@ then restored."

 (defun eglot--call-with-fixture (fixture fn)
  "Helper for `eglot--with-fixture'.  Run FN under FIXTURE."
-  (let* ((fixture-directory (make-temp-file "eglot--fixture" t))
+  (let* ((temporary-file-directory ;; ensure tmp directory respects remote


Similar comment here, though maybe this makes sense. However, it is only in the subset of Tram tests that we need to worry about these things. So a second macro like eglot--with-tramp-fixure or just directly let-binding this in the new Tramp-related tests, would be sufficient.

no need to do a second macro, simply let-ing default-directory to something remote (ie: with tramp prefix) before calling this function should do the trick.

no need to do a second macro, simply let-ing default-directory to something remote (ie: with tramp prefix) before calling this function should do the trick.

Yes, that works fine too. That's what I meant by "directly let-binding this" in the new tests.

I mis-read the fix I did online

it was originally there and using `delete-trailing-whitespace` in `before-save-hook` would erase it

The purpose of calling `eglot--maybe-activate-editing-mode` here is to handle all buffers that could've been handled by the server we've just set up *and* hadn't been doing so. However, it wouldn't make sense to try to manage / edit a buffer that is not reachable by the server. This happens when we're editing tramp / remote files, in which the server may be running in one machine (say, my-remote-server) and a buffer is running in another one (say, localhost)

nemethf · 2020-07-25T18:51:04Z

Hi Felipe. I won't be able to review this PR thoroughly or even try it out. Additionally, I won't use this feature. Nevertheless, I'd like to see this merged. However, it will make my life a bit harder, because I rely on the x-files LSP extension (implemented in eglot-x.el) that allows us to work on tramp files using a local LSP server. But your approach is definitely better for larger projects. This PR interferes with eglot-x, but do not worry about that. I'll fix elgot-x when necessary.

Regarding the documentation in README.md, I don't see why users should configure anything if clangd is in the users' (remote) path. Can the tramp support work out-of-the box for simple cases?

I don't understand this comment: "activate managed mode on other buffers in the same machine this server is running in". Why do you need special treatment for remote files? What happens if you have an emacs session with two projects on the same remote machine but with different LSP servers, or an independently opened remote file not belonging to any of those projects?

edkolev · 2020-07-26T06:05:08Z

What's this eglot-x thing, I don't see such a file in the project?

@nemethf I'd advise against supporting so many features in eglot - is there any value in allowing multiple options to work over tramp? Would be confusing for the user, hard for the maintainer

edkolev · 2020-07-27T08:35:22Z

@FelipeLema I tested your branch on emacs 26.3 - I get an error for wrong number of args passed to executable-find

I assume emacs 27 has signature (executable-find COMMAND &optional REMOTE)
but my emacs has (executable-find COMMAND)

trace:

Debugger entered--Lisp error: (wrong-number-of-arguments (1 . 1) 2)
  executable-find("gopls" t)
  (if (file-remote-p default-directory) (executable-find program t) (executable-find program))
  (not (if (file-remote-p default-directory) (executable-find program t) (executable-find program)))
  (and program (not (if (file-remote-p default-directory) (executable-find program t) (executable-find program))))
...

FelipeLema · 2020-07-28T14:36:55Z

@edkolev yes, you're right. executable-find changed signature in version 27: the &optional REMOTE was added there. As far as I know, this feature is not available as an (m)ELPA package.

Perhaps this feature can be backported into eglot (like eglot--executable-find COMMAND &optional REMOTE), but I don't know if it's part of the goal of eglot.

FelipeLema · 2020-07-28T14:57:24Z

Hi Felipe. I won't be able to review this PR thoroughly or even try it out. Additionally, I won't use this feature. Nevertheless, I'd like to see this merged. However, it will make my life a bit harder, because I rely on the x-files LSP extension (implemented in eglot-x.el) that allows us to work on tramp files using a local LSP server. But your approach is definitely better for larger projects. This PR interferes with eglot-x, but do not worry about that. I'll fix elgot-x when necessary.

Let me know if I can help of that. I had no idea of this eglot-x project you mention.

Regarding the documentation in README.md, I don't see why users should configure anything if clangd is in the users' (remote) path. Can the tramp support work out-of-the box for simple cases?

Yes, it will support simple cases where the command (say clangd) is in the PATH in the remote server. However, when specifying an explicit remote full path may not be that obvious.

Perhaps it's too aggressive to put it there in the readme / front page and maybe that info should go somewhere else. Would you have somewhere specific in mind?

I don't understand this comment: "activate managed mode on other buffers in the same machine this server is running in". Why do you need special treatment for remote files? What happens if you have an emacs session with two projects on the same remote machine but with different LSP servers, or an independently opened remote file not belonging to any of those projects?

Let's say you are working on laptop L and you have remote connections to machines A in Armenia, B in Belarus and C in Chile. For simplicity, you start with Emacs with no opened files.

So you start editing a python file animal.py @ A, starting pyls. No other buffers should be in managed mode.
Then, you open another python file beads.py @ B. It should not be managed by pyls @ A since pyls @A cannot reach files in B. So you end up with pyls @ A managing animal.py and pyls @ B managing beads.py.
Say, you open another file access.py @ A. This one should be managed by pyls @ A and not by pyls @ B, right?

The way it works is that tramp uses a prefix for paths, (/ssh:A/somewhere/animal.py, /ssh:B/elsewhere/beads.py). This prefix has several properties and we're using it to compare if two paths share the same connection (/ssh:A/ & /ssh:B/).
At the same time, when this prefix is nil it means that the path corresponds to a local file (in L). Using equal we can compare prefixes and it works out all right (nil ≠ /ssh:A means that "local file" is not using the same connection as "ssh to machine A").

I can't think of a re-phrasing of this comment that can a) be more clear about it b) uses one or two lines. Suggestions are welcome.

This helps when dealing with same basename-directory in different servers

It helps readibility since it's closer to tramp naming scheme in which the hostname goes at the beginning of the filepath.

nemethf · 2020-08-03T14:27:40Z

Let me know if I can help of that. I had no idea of this eglot-x project you mention.

Elgot-x is a project of mine. I mentioned it because its remote server support is the reason I don't follow your work closely. You shouldn't worry about eglot-x at this point. (Although it would make my life easier later if eglot--path-to-uri only used file-local-name if the server is a tramp remote server.)

Regarding the documentation in README.md, I don't see why users should configure anything if clangd is in the users' (remote) path. Can the tramp support work out-of-the box for simple cases?

Yes, it will support simple cases where the command (say clangd) is in the PATH in the remote server. However, when specifying an explicit remote full path may not be that obvious.

Perhaps it's too aggressive to put it there in the readme / front page and maybe that info should go somewhere else. Would you have somewhere specific in mind?

I don't know. Maybe stating this is only necessary for explicitly specifying a remote full path is enough.

I don't understand this comment: "activate managed mode on other buffers in the same machine this server is running in". Why do you need special treatment for remote files? What happens if you have an emacs session with two projects on the same remote machine but with different LSP servers, or an independently opened remote file not belonging to any of those projects?

Let's say you are working on laptop L and you have remote connections to machines A in Armenia, B in Belarus and C in Chile. For simplicity, you start with Emacs with no opened files. [...]

I think the approach you described is problematic. This is the correct approach: two files should be served by the same server if they belong to the same project. They belong to the same project if their project-roots are equal. It is worth noting that project.el can handle remote files, but the whole project must be on a single (remote or local) system.

FelipeLema · 2020-08-03T20:37:10Z

Let me know if I can help of that. I had no idea of this eglot-x project you mention.

Elgot-x is a project of mine. I mentioned it because its remote server support is the reason I don't follow your work closely. You shouldn't worry about eglot-x at this point. (Although it would make my life easier later if eglot--path-to-uri only used file-local-name if the server is a tramp remote server.)

Regarding the documentation in README.md, I don't see why users should configure anything if clangd is in the users' (remote) path. Can the tramp support work out-of-the box for simple cases?

Yes, it will support simple cases where the command (say clangd) is in the PATH in the remote server. However, when specifying an explicit remote full path may not be that obvious.
Perhaps it's too aggressive to put it there in the readme / front page and maybe that info should go somewhere else. Would you have somewhere specific in mind?

I don't know. Maybe stating this is only necessary for explicitly specifying a remote full path is enough.

will update the README accordingly, then.

I don't understand this comment: "activate managed mode on other buffers in the same machine this server is running in". Why do you need special treatment for remote files? What happens if you have an emacs session with two projects on the same remote machine but with different LSP servers, or an independently opened remote file not belonging to any of those projects?

Let's say you are working on laptop L and you have remote connections to machines A in Armenia, B in Belarus and C in Chile. For simplicity, you start with Emacs with no opened files. [...]

I think the approach you described is problematic. This is the correct approach: two files should be served by the same server if they belong to the same project. They belong to the same project if their project-roots are equal. It is worth noting that project.el can handle remote files, but the whole project must be on a single (remote or local) system.

I understand where you're coming from. This is actually an optimization to reduce the number of calls to each function in project-find-functions. It works with or without this optimization.

However, calls to functions in project-find-functions are not cheap when dealing with remote servers. It can take even seconds to call a single one. Even after spending a lot in tinkering (mostly caching) with every of those functions, it would still take a lot of time to a LangServer to load because it had to cover every buffer. Adding this "reduced buffer list" optimization was worth more than all the tinkering I did myself.

nemethf · 2020-08-10T17:27:29Z

I don't understand this comment: "activate managed mode on other buffers in the same machine this server is running in". Why do you need special treatment for remote files? What happens if you have an emacs session with two projects on the same remote machine but with different LSP servers, or an independently opened remote file not belonging to any of those projects?

[...] This is actually an optimization to reduce the number of calls to each function in project-find-functions. It works with or without this optimization.

Now that I've actually read the code, I think I understand it. The code below the comment pre-filters (buffer-list) to remove buffers that are on different (remote) systems, therefore the (eglot--maybe-activate-editing-mode) is an expensive no-op for them. I'd say the comment didn't help to understand this.

What do you think about something like the following comment instead:

;; Remove buffers belonging to a different (remote) system.  They are
;; surely not part of this project and calling
;; `eglot--maybe-activate-editing-mode' for them can be quite
;; expensive.

Additionally, the white-space changes right below this change are unnecessary and confusing.

Also, it's not a big deal, but somehow I'd like to avoid non-ascii chars when possible. Can you replace «» with something simpler?

Thanks.

FelipeLema · 2020-08-11T14:24:56Z

no problem, applying changes

Requested in #463 (comment)

Requested at #463 (comment)

edkolev · 2020-09-02T05:32:22Z

@FelipeLema what's pending to move this PR forward?

If you need help with testing - I can pull the branch and test

FelipeLema · 2020-09-02T14:58:52Z

I've compiled the pending items in the first post of this PR

Also close joaotavora/eglot#463, close joaotavora/eglot#84. Thanks to Brian Cully for the original simple idea. The basic technique is to pass :file-handler t to make-process, then tweak eglot--uri-to-path and eglot--path-to-uri, along with some other functions, to be aware of "trampy" paths". Crucially, a "stty hack" was needed. It has been encapsulated in a new a new eglot--cmd helper, which contains a comment explaining the hack. Co-authored-by: João Távora <joaotavora@gmail.com> * eglot.el (eglot--executable-find): Shim two-arg executable-find function only available on Emacs 27. (eglot--guess-contact): Use eglot--executable-find. (eglot--cmd): New helper. (eglot--connect): Use eglot--cmd. Use :file-handler arg to make-process. (eglot--connect, eglot--path-to-uri): Be aware of trampy file names. * eglot-tests.el (eglot-tests--auto-detect-running-server-1): New helper. (eglot--guessing-contact): Better mock for executable-find. (eglot--tramp-test): New test. * NEWS.md: mention TRAMP support. * README.md: mention TRAMP support.

Also close #463, close #84. Thanks to Brian Cully for the original simple idea. The basic technique is to pass :file-handler t to make-process, then tweak eglot--uri-to-path and eglot--path-to-uri, along with some other functions, to be aware of "trampy" paths". Crucially, a "stty hack" was needed. It has been encapsulated in a new a new eglot--cmd helper, which contains a comment explaining the hack. Co-authored-by: João Távora <joaotavora@gmail.com> * eglot.el (eglot--executable-find): Shim two-arg executable-find function only available on Emacs 27. (eglot--guess-contact): Use eglot--executable-find. (eglot--cmd): New helper. (eglot--connect): Use eglot--cmd. Use :file-handler arg to make-process. (eglot--connect, eglot--path-to-uri): Be aware of trampy file names. * eglot-tests.el (eglot-tests--auto-detect-running-server-1): New helper. (eglot--guessing-contact): Better mock for executable-find. (eglot--tramp-test): New test. * NEWS.md: mention TRAMP support. * README.md: mention TRAMP support. #637: joaotavora/eglot#637 #463: joaotavora/eglot#463 #84: joaotavora/eglot#84

Also close joaotavora/eglot#463, close joaotavora/eglot#84. Thanks to Brian Cully for the original simple idea. The basic technique is to pass :file-handler t to make-process, then tweak eglot--uri-to-path and eglot--path-to-uri, along with some other functions, to be aware of "trampy" paths". Crucially, a "stty hack" was needed. It has been encapsulated in a new a new eglot--cmd helper, which contains a comment explaining the hack. Co-authored-by: João Távora <joaotavora@gmail.com> * eglot.el (eglot--executable-find): Shim two-arg executable-find function only available on Emacs 27. (eglot--guess-contact): Use eglot--executable-find. (eglot--cmd): New helper. (eglot--connect): Use eglot--cmd. Use :file-handler arg to make-process. (eglot--connect, eglot--path-to-uri): Be aware of trampy file names. * eglot-tests.el (eglot-tests--auto-detect-running-server-1): New helper. (eglot--guessing-contact): Better mock for executable-find. (eglot--tramp-test): New test. * NEWS.md: mention TRAMP support. * README.md: mention TRAMP support. GitHub-reference: close joaotavora/eglot#637

FelipeLema added 2 commits May 8, 2020 14:19

make uri↔path & make-process remote-aware

fdee656

make executable-find remote aware

0f01fb7

joaotavora requested changes May 9, 2020

View reviewed changes

eglot.el Outdated Show resolved Hide resolved

eglot.el Outdated Show resolved Hide resolved

eglot.el Outdated Show resolved Hide resolved

nemethf reviewed May 9, 2020

View reviewed changes

eglot.el Outdated Show resolved Hide resolved

FelipeLema added 4 commits May 11, 2020 09:59

no multi-byte characters that can be omitted

8220c42

concise documentation per review

0df2e04

compact code per review

a1bb6f2

tramp-file-local-name → file-local-name

96937a3

FelipeLema mentioned this pull request May 11, 2020

Allow path mapping for remote or Windows/WSL/Mingw servers #350

Open

FelipeLema added 2 commits May 11, 2020 11:42

test remoteness on path (project root)

a1f9105

don't test remoteness on process… that doesn't make any sense

run all tests using tramp (remote editing through ssh localhost)

96efc95

inspired from FelipeLema/emacs-counsel-gtags@dc6bc8b#diff-354f30a63fb0907d4ad57269548329e3

joaotavora reviewed May 11, 2020

View reviewed changes

eglot.el Outdated Show resolved Hide resolved

FelipeLema added 2 commits May 11, 2020 15:15

remove redundant comment

c9e5aeb

reported (project) root is relative to server process

fb82c10

FelipeLema marked this pull request as ready for review May 11, 2020 23:22

Merge branch 'master' into tramp-support

175a3e2

continuous lines for install section in Travis

7e2051a

FelipeLema added 2 commits May 11, 2020 19:30

typo: matching parentheses

e2f750e

ensure temp directories respect remoteness of default-directory

88b6643

joaotavora reviewed May 12, 2020

View reviewed changes

Felipe Lema added 3 commits July 13, 2020 19:16

fix 3e8221a

00b196a

I mis-read the fix I did online

add trailing newline

c5ac58b

it was originally there and using `delete-trailing-whitespace` in `before-save-hook` would erase it

revert trailing newlines in eglot{,-tests}.el to original

0a98071

FelipeLema requested review from joaotavora and nemethf July 14, 2020 14:33

Felipe Lema added 3 commits July 20, 2020 13:12

use the full path to get tramp prefix, not file-local-name

a7f5a3f

don't provide processId for servers executing remotely

0e10c31

Felipe Lema added 2 commits July 29, 2020 13:14

add the hostname in the nickname

062d16d

This helps when dealing with same basename-directory in different servers

format nickname as "host:directory" instead of "directory@host"

ab38015

It helps readibility since it's closer to tramp naming scheme in which the hostname goes at the beginning of the filepath.

Introduce "note for special case of tramp commands not in PATH"

6f82edd

Felipe Lema added 2 commits August 11, 2020 10:32

explain reason for filtering buffers by tramp prefix, revert indent

981aa45

Requested in #463 (comment)

replace / avoid non-ascii chararters

857c89f

Requested at #463 (comment)

bjc mentioned this pull request Mar 3, 2021

TRAMP support #637

Closed

joaotavora closed this in 7918fac Mar 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tramp support, fixes #84 #463

Tramp support, fixes #84 #463

FelipeLema commented May 8, 2020 •

edited

Loading

FelipeLema commented May 8, 2020

garyo commented May 9, 2020

joaotavora commented May 9, 2020

joaotavora left a comment

garyo commented May 9, 2020

FelipeLema commented May 11, 2020

FelipeLema commented May 11, 2020 •

edited

Loading

FelipeLema commented May 11, 2020

FelipeLema commented May 11, 2020

joaotavora commented May 11, 2020

joaotavora May 12, 2020

FelipeLema May 12, 2020

joaotavora May 12, 2020

joaotavora May 12, 2020

FelipeLema May 12, 2020

joaotavora May 12, 2020

nemethf commented Jul 25, 2020

edkolev commented Jul 26, 2020

edkolev commented Jul 27, 2020

FelipeLema commented Jul 28, 2020

FelipeLema commented Jul 28, 2020

nemethf commented Aug 3, 2020

FelipeLema commented Aug 3, 2020 •

edited

Loading

nemethf commented Aug 10, 2020

FelipeLema commented Aug 11, 2020

edkolev commented Sep 2, 2020

FelipeLema commented Sep 2, 2020

Tramp support, fixes #84 #463

Tramp support, fixes #84 #463

Conversation

FelipeLema commented May 8, 2020 • edited Loading

FelipeLema commented May 8, 2020

garyo commented May 9, 2020

joaotavora commented May 9, 2020

joaotavora left a comment

Choose a reason for hiding this comment

garyo commented May 9, 2020

FelipeLema commented May 11, 2020

FelipeLema commented May 11, 2020 • edited Loading

FelipeLema commented May 11, 2020

FelipeLema commented May 11, 2020

joaotavora commented May 11, 2020

joaotavora May 12, 2020

Choose a reason for hiding this comment

FelipeLema May 12, 2020

Choose a reason for hiding this comment

joaotavora May 12, 2020

Choose a reason for hiding this comment

joaotavora May 12, 2020

Choose a reason for hiding this comment

FelipeLema May 12, 2020

Choose a reason for hiding this comment

joaotavora May 12, 2020

Choose a reason for hiding this comment

nemethf commented Jul 25, 2020

edkolev commented Jul 26, 2020

edkolev commented Jul 27, 2020

FelipeLema commented Jul 28, 2020

FelipeLema commented Jul 28, 2020

nemethf commented Aug 3, 2020

FelipeLema commented Aug 3, 2020 • edited Loading

nemethf commented Aug 10, 2020

FelipeLema commented Aug 11, 2020

edkolev commented Sep 2, 2020

FelipeLema commented Sep 2, 2020

FelipeLema commented May 8, 2020 •

edited

Loading

FelipeLema commented May 11, 2020 •

edited

Loading

FelipeLema commented Aug 3, 2020 •

edited

Loading