TRAMP support #637
TRAMP support #637
Conversation
* Copy in Emacs 27.1’s ‘executable-find’ implementation, so it can be used on older Emacsen, where we need the second argument to tell it to look for remote commands. * Adjust a number of functions to accomodate swapping between local and remote paths as appropriate. * Force unbuffered I/O on LSP subprocesses to prevent hanging during JSONRPC calls.
|
Interesting! This looks very good!
If this works, then your
I'm not keen on this, but I can be convinced, if you explain why you "feel" it's less error-prone. In things run in the same stack frame, using a special variable should be just as effective.
Yes, but I like that you commented it and isolated it. I don't fully understand it, but that's OK. Can you make a bug report to Emacs and link the resulting discussion thread here? |
* `eglot--executable-find` is bound at run-time based on
`emacs-major-version`.
* TRAMP test calls into `auto-detect-running-server` directly, after
setting `temporary-file-directory` to the loopback method.
* Conditionalized aspects of calling `make-process` on whether or
not the current file is remote:
- Use of `stty` to ensure raw I/O, and
- assuming Emacs > 27, appending `:file-handler t` to arguments.
* Removed `server` argument to `eglot--uri-to-path`, opting instead
to always call `eglot--current-server-or-lose` to determine
mappings.
* Miscellaneous whitespace changes.
* Renamed `test` TRAMP method in test to `loopback` to better match
what it’s doing.
|
Sure thing. Let me know when to pull your changes. |
Tests seem to all pass, though this is a bit odd:
Tramp: Sending command ‘exec /bin/sh ’
Tramp: Found remote shell prompt on ‘krug’
Tramp: Sending command ‘exec /bin/sh ’
Tramp: Found remote shell prompt on ‘krug’
[eglot] Test body was A FAILURE
[eglot] Killing nil, wiping /loopback:krug:/tmp/eglot--fixturefBjpKc, restoring nil
Ran 1 tests, 0 results were as expected, 1 unexpected
passed 10/43 eglot--tramp-test (0.518254 sec)
Either that "A FAILURE" is being misreported, or this must be tested
manually, to make sure.
|
There. I've simplified this. Please test. We probably still ahve to special-case the test to only run in > Emacs 27.1. Then I'll do the commit message and squash adjustments, crediting you foremost, of course. Thanks, this really looks like the lean, simple TRAMP support PR I had been waiting for. |
|
Oh no, I commited a bunch of useless files I had lying here. hehehe time to force push the branch! |
* README.md (TRAMP support): New subsection. * NEWS.md: Menation TRAMP support
|
Done. |
|
Just pulled the latest from you and everything looks good on my end. Regarding the test, it's probably best to: (skip-unless (and (executable-find "pyls") (>= emacs-major-version 27))) |
yes, exactly But did you test manually and play around a little bit with tramp? In Emacs 27.1? Are the simplistic instructions in the README.md correct? |
|
The README seems fine to me. I haven't done any special setup for my TRAMP buffers. I have done some light testing with xref style stuff and variable/method renaming, and it seems to work well. I do not have Emacs 27 installed, unfortunately. I'm running 28. I'll have to set up a container with 27 to verify it there. |
|
Great. From what I remember from the investigations of #463, I strongly suspect it won't work with Emacs 27.1, so maybe the conditions have to be tweaked But that's fine, really. Simplicity trumps it. |
|
I've just run some tests against Emacs 27.1, and haven't run into any issues. |
That's even more fantastic. To think this years-old request was so few lines of code away... Finally, can you double-check that |
To be fair, it does rely on features in 27.1 to do it as simply as it does.
I have checked this so many times, because I hate it. I've just tried again without it, in the vain hope that maybe something miraculous has happened, but no, it's still very definitely required. I would love to get rid of it, but I think it's going to take some work within Emacs itself to do so. Definitely a project I'm keen on when I get some time for it. |
The test may be good, but it was running another instance of ert inside it, meaning it would always pass, I think. It's better to have both the normal and the TRAMP test call a function. Then we can see that the TRAMP automatic test is failing. We have to understand why.
Yes, last I checked. Curious. Are you sure you deleted your elc's and and ran a simple |
|
I was testing in-Emacs with That said, I'm also using |
|
But the tests are running |
|
Curiously, on Emacs 27, my failure is entirely different: |
|
I have Your backtrace doesn't make any sense to me. It looks like something's going wrong in TRAMP itself? I'll see what I can dig up. |
|
@bjc but when you |
|
Oh, is the previous backtrace from Emacs 26? I don't have that installed, but maybe there's a dependency on a newer TRAMP that I'm not aware of. But if so, since we're only concerned with support for 27+, I don't know how important that actually is. The second backtrace just shows that The configuration isn't being bypassed -- since I never got |
|
I'm going to see if I can figure out what's wrong with |
No sorry, from Emacs 28. Sorry for not being specific.
Yes, but it should be I have it in my path . So maybe there's something wrong with that specific test. It mustn't fail like that.
Wait, so all these tests you've been running are against a modified |
Ok, that's my main Emacs, so it's not a problem for me to test there.
Agreed, but at least its tractable.
I've only changed the one language server. Everything else is unmodified. And that language server change was only because
emacs27:~/emacs-lisp/eglot% pyls ±[●][bjc/tramp-support]
Traceback (most recent call last):
File "/home/bjc/.local/bin/pyls", line 5, in <module>
from pyls.main import main
File "/home/bjc/.local/lib/python3.9/site-packages/pyls/main.py", line 65
print '%s%s' % (' ' * indent, name)
^
SyntaxError: invalid syntaxAs you can see, the issue isn't with eglot, but pyls itself. I'm going to see if I can fix it, because I would hate for this patch to be held up on account of not being able to test with it, but hopefully you can see why I've opted to change the python language server I'm using for testing. |
Yeah it seems silly. Well, if |
|
Turns out I should have done some digging sooner. I'm not the only one with this problem: palantir/python-language-server#631 Installing the correct As far as |
|
I've tracked down the issue, but I'm unsure how to resolve it. It is, indeed, a You can change this behavior with the following elisp: (add-to-list 'tramp-remote-path 'tramp-own-remote-path)I'd added that to my The easiest thing to do would be amend the test by binding (ert-deftest eglot--tramp-test ()
"Ensure LSP servers can be used over TRAMP."
(skip-unless (or (>= emacs-major-version 27) (executable-find "pyls")))
;; Set up a loopback TRAMP method that’s just a shell so the remote
;; host is really just the local host.
(let ((tramp-remote-path (cons 'tramp-own-remote-path tramp-remote-path))
(tramp-methods '(("loopback"
…I'm somewhat ambivalent about this solution. It works, but feels a bit hacky. I can't see a better solution right now, though. |
What's your concern about it? I think it should work... |
TRAMP uses the system path, as determined by ‘getconf PATH’, as well as built-in defaults. If the language server does not reside in that PATH, but relies on being found by the user-level PATH, such as may be defined in ‘$HOME/.profile’, this test would fail. By adding the symbol ‘tramp-own-remote-path’ to TRAMP’s list of paths to check, the user-lever PATH is taken into account and the language server will be found.
|
Thinking about it more, since the Even in the case of running from within Emacs, where it may be set already, it doesn't really cause any harm. So I've pushed the change up. |
|
Great, all tests pass here, I'm sqaushing this up for the master branch. Only one final hurdle remains: do you have a FSF copyright assignment? your name is not "Brendan" right? |
|
Your change can go in early regadless, but if you somehow oppose getting one for the FSF, I'll have to revert and reimplement your work. Because the solution is so simple (a very good thing!), it might count as a "trivial" contribution. I'm hailing @monnier, former maintainer of Emacs, who should know about these things. |
|
Brendan may be my brother. It's entirely possible he's signed an FSF agreement. I, however, have not, although I have no qualms doing so. I'm not sure how to go about doing it, however, so any help there is appreciated. |
His name might have popped up in the copyright file I have access to ;-) Anyway, please write to |
|
> Brendan may be my brother. It's entirely possible he's signed an FSF agreement.
His name might have popped up in the copyright file I have access to ;-)
Anyway, please write to ***@***.***` and request a copyright
No need for that. Just fill the form below and send it as instructed to
the FSF so they can send you the relevant paperwork to sign.
Stefan
Please email the following information to [email protected], and we
will send you the assignment form for your past and future changes.
Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
[What is the name of the program or package you're contributing to?]
Emacs
[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]
[Do you have an employer who might have a basis to claim to own
your changes? Do you attend a school which might make such a claim?]
[For the copyright registration, what country are you a citizen of?]
[What year were you born?]
[Please write your email address here.]
[Please write your postal address here.]
[Which files have you changed so far, and which new files have you written
so far?]
|
|
I submitted the form yesterday, and am awaiting response. I'll let you know when everything's done. |
|
In the meantime, I pushed this to master. Let's be aware of possible bug reports in the next weeks. |
|
I just got word that the copyright assignment is finished, so there should be no further issues. |
kööl
famous last words 😅 I wonder if anyone's actually using this. Anyway, it's a very minimal implementation, it should be relatively easy to debug. |
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 <[email protected]> * 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 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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
This patch adds TRAMP support to eglot (fixes #84)
I have seen #463 but decided to have another go at it. Some differences include:
executable-findto be used on older Emacsen. 27.1 and later will use the native definition.servera mandatory argument toeglot--uri-to-path, because I feel it's less error-prone than having a default. Instead, I've passed in the default where necessary.The one thing I'm not super-keen on in this patch is the use of
sttywhen creating the process for the LSP. I don't know any other way to do it, though, and it seems to be how it's done from what I could dig up. It does feel like a bug in Emacs itself, though, that it should be necessary.