-
Notifications
You must be signed in to change notification settings - Fork 203
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
Eglot usually fails to start when using TRAMP on Windows with clangd #662
Comments
Eglot + TRAMP + Windows is more than likely to not work. This was documented by @bjc in the source code:
I don't have any particular expertise to offer here, sorry. |
I marked this |
Hmm, I would have thought that comment only applied when connecting to a non-Unix system, not for the system Emacs is running on. You're probably right, however, that the issue lies somewhere in TRAMP/Emacs. I'll see if I can construct a reduced test case that doesn't rely on Eglot, since I imagine that'll make it easier to get this fixed in TRAMP/Emacs. Of course, if someone with more TRAMP expertise than me (maybe @bjc?) wants to take a look at this here, I'm happy to help, even if it's just by providing more debug logs. |
Testing this further, I don't see this problem when using ccls, so there's something unusual that clangd is doing. I did also notice that Flymake doesn't highlight errors and gets stuck in a "Wait" state when using eglot over TRAMP (on both Windows and Linux clients); I'd guess that's a Flymake issue, though I haven't been able to narrow it down yet. |
It's likely not, but we'd need a minimal reproduction recipe to reproduce it. |
Once I get some time to dig into the Flymake issue, I'll file a new issue with steps to reproduce it. As for this issue, I've narrowed the problem down to this hook in If I remove that hook, my issue described above goes away. I can run Since it looks like you wrote most of |
This is probably not an issue with eglot specifically. I can reproduce it in a vanilla Emacs 28 snapshot (acquired here), with the following code: (require 'jsonrpc)
(defun eglot-minimal ()
(interactive)
(make-instance
'jsonrpc-process-connection
:name "eglot-minimal"
:process (lambda ()
(make-process
:name "eglot-minimal"
:command '("sh" "-c" "stty raw > /dev/null; clangd")
:connection-type 'pipe
:coding 'utf-8-emacs-unix
:noquery t
:stderr (get-buffer-create "*eglot-minimal stderr*")
:file-handler t)))) (This code is extracted/reduced from Since @Ergus mentions the same problem in #667, hopefully we can coordinate to figure out what our setups have in common so we have a better idea of where this needs to be fixed. |
Oh, just a couple more small notes:
Looking at this more, when running
|
For what it's worth, the following hack seems to work: (require 'eglot)
(defun eglot--cmd (contact)
(if (file-remote-p default-directory)
(list "sh" "-c"
(string-join
(append (cons "stty raw > /dev/null;"
(mapcar #'shell-quote-argument contact))
'("2>" "/dev/null"))
" "))
contact)) That is, we do what I'll give others a bit of time to chime in on this while I also think over whether I have anything to add, and then I'll file a bug against Emacs for this. |
Yes, this seems like an API wrong use or a Tramp issue. I think that @albinus could help with this in any case to clarify either if the issue is in eglot code or in Tramp and a but report will be needed. |
stderr in remote make-process is flaky. So I'd prefer an Emacs bug with a minimal recipe. |
@albinus The shortest recipe I've been able to get is here. Evaling the Elisp in that comment in Emacs 28 and then doing the following reproduces the issue for me:
I don't think the Tramp method matters (I saw this with I noticed this on a Windows client connecting to a Linux server (I don't have access to my Linux client at the moment to test there). @Ergus, were you seeing this on a different OS combination? If this happens on all platforms, that would make it easier to test. I'll file an Emacs bug once I'm sure of what the OS requirements are to reproduce this. At least based on the results of the test in 05fe647, this only happens when you're actually connecting remotely. The "loopback" method in that test (which just invokes |
@jimporter that recipe has an error, I think you need to quote |
@joaotavora Hmm, it worked for me without the quote, but I'm not an expert on when quoting is required in Elisp. It seems to work (or at least, to reproduce this bug) with the quote too though. When I submit all this to the Emacs bug tracker, I'll be sure to give the function a slightly more-generic name though. :) |
That's very odd, |
It looks like there's a obsoleted feature in Elisp that lets a symbol-evaluated-as-a-variable refer to the function with that name.
So adding the quote is correct (not that I doubted it in the first place :) ), and I was inadvertently relying on an obsolete quirk of Elisp. |
Oh, curious. Thanks for explaining. By the way, the error message is misleading. I added |
@jimporter I'm able to reproduce the problem. It is in |
Hi @albinus Maybe you already got all this. But just in case I gave some other details here:
|
@albinus Thanks for taking a look. If you'd like me to file an Emacs bug to track this too, just let me know. |
Hi @Ergus, @jimporter Sure, that's the error case in The following patch works for me, and it shows the complete stderr output in buffer
|
@albinus That patch works for me. Thanks for looking into this! I don't know if it will help, but after some more investigation locally, I think I'm seeing the error from ;; We must ensure that `file-coding-system-alist'
;; matches `local-copy'.
(let ((file-coding-system-alist
(tramp-find-file-name-coding-system-alist
filename local-copy)))
(setq result
(insert-file-contents ; HERE
local-copy visit beg end replace)))) As you say, this might be a problem with the coding system, since the code here manipulates the coding system for the file. If I'm understanding things correctly, this code is called by the code in your patch above. Perhaps a change here would fix things; when stepping through this code, I saw that the coding system for the file (named something like Still, your patch above works for me, so maybe the rest of this comment isn't necessary. :) I'm not very familiar with what's happening here, so simply ignoring the errors might be the right thing to do. |
That's the place I've modified again and again last two days while debugging, with no luck. But I have modified my patch as below, and this looks like it is working. Although I must confess I still don't know what I'm doing. Could you please test?
|
@albinus, thanks for all this work. When you're satisfied with the patch, do you think you could make a TRAMP release to GNU ELPA so that I can make Eglot depend on that and thus fix this bug? |
Sure, the next Tramp 2.5.0.4 release is scheduled for end of April, on GNU ELPA. There are still some other problems in the queue, and the last week prior a release I need for extensive regression tests. |
@albinus The second patch doesn't work for me.
That was roughly my experience trying to fix things in |
Oops, you're right. In my local setup, direct async processes were still enabled, and they don't show this problem. Cleaning Tramp cache, the error happens also with my second patch. Sorry. I propose you shall write an Emacs bug report. I will apply my first patch (using |
@albinus I'll try to compile all the relevant information in this issue to send to the Emacs bug tracker tomorrow. Thanks again for the help! |
Filed against Emacs as https://debbugs.gnu.org/cgi/bugreport.cgi?bug=47861 @joaotavora With a workaround for this committed to Tramp and a bug filed against Emacs for what seems to be the underlying issue, maybe this issue can be resolved? It could also be worth keeping around at least until Tramp 2.5.0.4 is published to ELPA, and then make eglot depend on Tramp 2.5.0.4 (or if that's too big a dep to pull in, at least add a note in the README to update Tramp if you're having problems). |
Yep let's do that.
Do you have any indication that it's big? How big? Also, can you summarize for those who didn't follow the discussion very closely what exactly the issue was and if it's fully fixed? It seems there some kind of second issue that's not so easy to address, right? |
Checking my git checkout of Tramp, the sources are a little under 24k lines (but that includes comments, etc). I don't have a particularly strong opinion either way. Personally, I have no issue with adding Tramp 2.5.0.4+ as an ELPA dep since I already use the ELPA version of Tramp, but I wasn't sure if other people had opinions here. I guess I'd lean softly towards adding it as a dep, since it fixes this issue and should reduce the number of duplicate issues filed in this repo.
The issue here appears to be that, at least on some platform combinations (Windows client -> Linux server; possibly others), starting a remote LSP server fails if it writes to stderr. The problem happens when That said, the Tramp fix is probably just a workaround and there's some deeper problem, hence the bug filed on the Emacs tracker. Whether the bug is in Tramp, |
If it's in As to the rest, maybe I agree that it might be useful to make it a "soft dependency", i.e. warn in the README that one might need a newer TRAMP to get it to work in some combinations. |
The code in
Sounds good to me. I think so long as people can see a note in the README, that should be enough. It's all the same to me though, since I've been using the GNU ELPA version of Tramp for a while now. |
TRAMP v2.5.0.4 is out now and this bug is resolved for me; I was able to remove the hacks I'd added to my |
Update: commit 34ce6a47 in the Tramp repo provides a proper fix for this by using a named pipe for the stderr buffer (at least that's my understanding; I hope I've got that right). If you like, I can add a brief note recommending Tramp 2.5.0.4+ to the README; then I think this issue can be closed. |
Thanks. Yes. Use the small section dedicated to TRAMP, maybe link to this issue, maybe very summarily describe the issue from the non-Elisper user perspective, and mention the easiest upgrade technique. |
…2.5.0.4+ * README.md (TRAMP support): Add note about updating TRAMP. (Reporting bugs): Move "reporting bugs" anchor to here.
…2.5.0.4+ * README.md (TRAMP support): Add note about updating TRAMP. (Reporting bugs): Move "reporting bugs" anchor to here.
…2.5.0.4+ * README.md (TRAMP support): Add note about updating TRAMP. (Reporting bugs): Move "reporting bugs" anchor to here.
* README.md (TRAMP support): Add note about updating TRAMP. (Reporting bugs): Move "reporting bugs" anchor to here.
Closing per my above comment, since this is fixed in TRAMP 2.5.0.4+ and documented in the Eglot README as of #700. |
I'm trying out eglot (latest from MELPA, currently
20210403.953
) on a Windows system running emacs 27.2. When runningM-x eglot
on a TRAMP file (connected to a Linux system), I usually get an error likeWrong type argument: "inserted-chars 200"
(the number in the message varies). Every once in a while, it works, although I'm not able to reproduce that reliably. I'm not sure it matters, but I'm attempting to useclangd
here.I've tried using the
sshx
andplinkx
protocols and both fail in the same way. I've also tried with both the version of TRAMP that ships with emacs (2.4.5.27.2) and the latest version on GNU ELPA (2.5.0.3); still no difference.M-x eglot-events-buffer
sadly doesn't have much of interest:However, the error backtraces might be more informative. Here's a log generated after calling
M-x toggle-debug-on-error
:M-x toggle-debug-on-error
Starting the debugger on
insert-file-contents
and manually stepping through gives some (possibly) more information about the call stack. In this particular case, I got the following slightly-different error:Not a Tramp file name: "c:/plinkx:server:/home/jim/"
:M-x debug-on-entry RET insert-file-contents
It's possible this is a bug in TRAMP (although it's not a problem I've ever encountered with TRAMP before) or a limitation of Windows pipes as mentioned in the README (though I'd expect the pipe is being set up on the Linux system). I'm happy to provide more information or to test a potential patch, though I don't think I'd be able to author a patch on my own as my Elisp skills probably aren't up to the challenge. :/
The text was updated successfully, but these errors were encountered: