Hi all, the commit 1a2d603bb3 was supposed to fix bug#58790, but it introduces (or possibly surfaces) another one. With Emacs (checkout c3b64985aa), eval'ing the next 2 forms returned an URI: (require 'eglot) (insert "\n" (format "%s" (eglot--path-to-uri "d:/digestif-test/tikz-test.tex"))) => file:///d%3A/digestif-test/tikz-test.tex With Emacs 623db40d, it looks like this: (require 'eglot) (insert "\n" (format "%s" (eglot--path-to-uri "d:/digestif-test/tikz-test.tex"))) => d:/digestif-test/tikz-test.tex I think the underlying problem is with the return value of `url-generic-parse-url', but I'm not familiar enough with it to make a judgement. As the result, digestif-LSP doesn't work with the current Eglot on Windows -- checkout c3b64985aa does. This is with: In GNU Emacs 29.0.50 (build 1, x86_64-w64-mingw32) of 2022-11-17 Repository revision: 623db40dd1cd21623c5cecdc0abbf3ce885f92b1 Repository branch: master System Description: Microsoft Windows 10 Pro Best, Arash
> From: Arash Esbati <arash@gnu.org>
> Date: Thu, 17 Nov 2022 17:51:01 +0100
>
> the commit 1a2d603bb3 was supposed to fix bug#58790, but it introduces
> (or possibly surfaces) another one. With Emacs (checkout c3b64985aa),
> eval'ing the next 2 forms returned an URI:
>
> (require 'eglot)
> (insert "\n" (format "%s" (eglot--path-to-uri
> "d:/digestif-test/tikz-test.tex")))
> => file:///d%3A/digestif-test/tikz-test.tex
>
> With Emacs 623db40d, it looks like this:
>
> (require 'eglot)
> (insert "\n" (format "%s" (eglot--path-to-uri
> "d:/digestif-test/tikz-test.tex")))
> => d:/digestif-test/tikz-test.tex
Both are wrong, right? The correct URL should AFAIU be
file:///d:/digestif-test/tikz-test.tex
IOW, the problem is that the URL is being run through url-encode-url,
which doesn't support file:// URLs on Windows properly.
On Thu, 17 Nov 2022 at 19:06, Eli Zaretskii wrote:
> Both are wrong, right? The correct URL should AFAIU be
>
> file:///d:/digestif-test/tikz-test.tex
>
> IOW, the problem is that the URL is being run through url-encode-url,
> which doesn't support file:// URLs on Windows properly.
While we are at it, note that
(url-filename
(url-generic-parse-url "file:///d:/digestif-test/tikz-test.tex"))
=> "/d:/digestif-test/tikz-test.tex"
is not the right file name under Windows. Eglot treats this special
case correctly, but every package that deals with file URLs has to
repeat the work. So there should be a helper function in Emacs for
this.
Augusto Stoffel <arstoffel@gmail.com> writes: > On Thu, 17 Nov 2022 at 19:06, Eli Zaretskii wrote: > >> Both are wrong, right? The correct URL should AFAIU be >> >> file:///d:/digestif-test/tikz-test.tex >> >> IOW, the problem is that the URL is being run through url-encode-url, >> which doesn't support file:// URLs on Windows properly. > > While we are at it, note that > > (url-filename > (url-generic-parse-url "file:///d:/digestif-test/tikz-test.tex")) > => "/d:/digestif-test/tikz-test.tex" > > is not the right file name under Windows. Eglot treats this special > case correctly, but every package that deals with file URLs has to > repeat the work. So there should be a helper function in Emacs for > this. With the original problem: we're now getting a false positive in eglot when checking for URLs being passed to `eglot--path-to-uri` when the path is a windows path. Is there something we can do to detect a windows path and continue treating it as a path like we were before this change? ``` (defun eglot--path-to-uri (path) "URIfy PATH." (let ((truepath (file-truename path))) (if (and (url-type (url-generic-parse-url truepath)) (NOT_WINDOWS_PATH truepath) ;; what would this be? ) ;; ... blah blah blah ``` If there is no function available already, it may be enough to check if the return value of `url-type` is not 1 character. Looking at this list of what I believe are official URI schemes, all of them have at least two characters: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml -- Danny Freeman
Eli Zaretskii <eliz@gnu.org> writes: > Both are wrong, right? The correct URL should AFAIU be > > file:///d:/digestif-test/tikz-test.tex Yes, this is also my understanding, after having a closer look. > IOW, the problem is that the URL is being run through url-encode-url, > which doesn't support file:// URLs on Windows properly. As a note, texlab LSP-server still works with this version: => d:/digestif-test/tikz-test.tex and digestif LSP-server accepts this version: => file:///d%3A/digestif-test/tikz-test.tex Best, Arash
On Thu, 17 Nov 2022 at 17:27, Danny Freeman wrote: > ``` > (defun eglot--path-to-uri (path) > "URIfy PATH." > (let ((truepath (file-truename path))) > (if (and (url-type (url-generic-parse-url truepath)) > (NOT_WINDOWS_PATH truepath) ;; what would this be? > ) > ;; ... blah blah blah > ``` > > If there is no function available already, it may be enough to check if > the return value of `url-type` is not 1 character. Looking at this list > of what I believe are official URI schemes, all of them have at least > two characters: > https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml I think that makes sense. I find the above logic a bit funny, though. What do you expect `truepath' to look like if `path' is actually an URI? Shouldn't `path' be returned unchanged? I also think that calling `url-generic-parse-url' might be overkill here. Based on https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax I would just test if `path' matches "\\`[A-Za-z][+.0-9A-Za-z-]+:".
> From: Danny Freeman <danny@dfreeman.email> > Cc: Eli Zaretskii <eliz@gnu.org>, Arash Esbati <arash@gnu.org>, > 59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org > Date: Thu, 17 Nov 2022 17:27:40 -0500 > > Is there something we can do to detect a windows path You mean, a Windows-style file name? You can detect that, but it is easier to test system-type instead: these file names cannot happen on any system except Windows, and if they do happen, they don't have the same semantics (i.e., "d:/foo/bar" is NOT an absolute file name on Posix systems). Or maybe I don't understand the purpose of the test you have in mind? > and continue treating it as a path like we were before this change? I'd advise against such kludges. If a function wants a file:// URL, it should receive a valid file:// URL on all systems, and it should be capable of handling file:// URLs on MS-Windows as well as on Posix systems. Likewise with functions which produce file:// URLs. Letting local file names into this is a clear path to future bugs, because many people will not realize this subtlety, and will think they deal with file:// URLs on all platforms. > If there is no function available already, it may be enough to check if > the return value of `url-type` is not 1 character. Looking at this list > of what I believe are official URI schemes, all of them have at least > two characters: > https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml But hosts can have 1-character names (although that is unlikely). Anyway, I'm against such kludges, especially since we don't need them here. We just need to make our functions that handle file:// URLs to be capable of supporting file:// on MS-Windows. It is not hard to do, so let's do that.
> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Arash Esbati <arash@gnu.org>,
> 59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> Date: Thu, 17 Nov 2022 18:12:05 -0500
>
> On Thu, 17 Nov 2022 at 17:27, Danny Freeman wrote:
>
> I also think that calling `url-generic-parse-url' might be overkill
> here. Based on
> https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax I would
> just test if `path' matches "\\`[A-Za-z][+.0-9A-Za-z-]+:".
Not sure I understand why this matters in the context of this
discussion. We need to make eglot--path-to-uri produce valid file://
URL on MS-Windows and on Posix systems, right? Then why does it
matter how URI schema are defined? What am I missing?
Eli Zaretskii <eliz@gnu.org> writes:
>
> Not sure I understand why this matters in the context of this
> discussion. We need to make eglot--path-to-uri produce valid file://
> URL on MS-Windows and on Posix systems, right? Then why does it
> matter how URI schema are defined? What am I missing?
The current eglot--path-to-uri implementation should produce a valid
file:// url unless what it receives is already a URL.
So it could receive something like:
/home/user/project/whatever.c
d:/what/home/is/on/windows/whatever.c
Both of which should be transformed into file:// URLs
OR what it receives may already be a URL like
zipfile:home/user/project.zip::/path/in/zip.c
If it receives a URL, we want to pass it along, and not transform it
into a file:// URL.
If it is a full windows path, we DO want to turn that into a file url.
So how do we detect that is is a windows path, and not a URL already?
That's what I was trying to get at in the other message you replied to.
Just checking the user's current OS is not enough, because this function
could also receive a URL on Windows.
--
Danny Freeman
> From: Danny Freeman <danny@dfreeman.email>
> Cc: Augusto Stoffel <arstoffel@gmail.com>, arash@gnu.org,
> 59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> Date: Fri, 18 Nov 2022 08:39:13 -0500
>
> The current eglot--path-to-uri implementation should produce a valid
> file:// url unless what it receives is already a URL.
>
> So it could receive something like:
>
> /home/user/project/whatever.c
> d:/what/home/is/on/windows/whatever.c
>
> Both of which should be transformed into file:// URLs
> OR what it receives may already be a URL like
>
> zipfile:home/user/project.zip::/path/in/zip.c
>
> If it receives a URL, we want to pass it along, and not transform it
> into a file:// URL.
>
> If it is a full windows path, we DO want to turn that into a file url.
>
> So how do we detect that is is a windows path, and not a URL already?
You test that system-type is windows-nt AND that the
file-name-absolute-p returns non-nil for the argument.
[-- Attachment #1: Type: text/plain, Size: 376 bytes --] Eli Zaretskii <eliz@gnu.org> writes: >> If it is a full windows path, we DO want to turn that into a file url. >> >> So how do we detect that is is a windows path, and not a URL already? > > You test that system-type is windows-nt AND that the > file-name-absolute-p returns non-nil for the argument. That is incredible simple. Maybe something like this patch will work: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Eglot-don-t-confuse-URLs-and-windows-file-paths.patch --] [-- Type: text/x-patch, Size: 1419 bytes --] From 85821844094e5d00ceb01902c55d9aa0fef73d17 Mon Sep 17 00:00:00 2001 From: dannyfreeman <danny@dfreeman.email> Date: Fri, 18 Nov 2022 10:09:17 -0500 Subject: [PATCH] Eglot: don't confuse URLs and windows file paths * lisp/progmodes/eglot.el (eglot--path-to-uri): check for windows path --- lisp/progmodes/eglot.el | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index c2d7fc309d..4661cf0e1a 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -1502,11 +1502,15 @@ eglot--uri-path-allowed-chars (defun eglot--path-to-uri (path) "URIfy PATH." (let ((truepath (file-truename path))) - (if (url-type (url-generic-parse-url truepath)) + (if (and (url-type (url-generic-parse-url path)) + ;; It might be a windows path which includes a drive + ;; letter that looks like a URL scheme + (not (and (eq system-type 'windows-nt) + (file-name-absolute-p truepath)))) ;; Path is already a URI, so forward it to the LSP server ;; untouched. The server should be able to handle it, since ;; it provided this URI to clients in the first place. - truepath + path (concat "file://" ;; Add a leading "/" for local MS Windows-style paths. (if (and (eq system-type 'windows-nt) -- 2.38.1 [-- Attachment #3: Type: text/plain, Size: 133 bytes --] I can confirm it works on linux, but I don't have a windows machine. Someone else would need to verify it there. -- Danny Freeman
Danny Freeman <danny@dfreeman.email> writes: > I can confirm it works on linux, but I don't have a windows machine. > Someone else would need to verify it there. Many thanks for working on this. I tried your patch on my Windows box and this is what I get: (require 'eglot) (insert "\n" (format "%s" (eglot--path-to-uri "d:/digestif-test/tikz-test.tex"))) => file:///d%3A/digestif-test/tikz-test.tex (insert "\n" (format "%s" (eglot--path-to-uri "d:/digestif-test/tikz test.tex"))) => file:///d%3A/digestif-test/tikz%20test.tex As Eli already mentioned, %-escaping the colon seems to be wrong (I couldn't find a definitive source for this, though), but it seems a deliberate decision in eglot.el's `eglot--uri-path-allowed-chars', which also references this GitHub issue[1]. My original problem is solved and digestif-LSP works on Windows again. So for now, I suggest to apply your patch and close this report. Best, Arash Footnotes: [1] https://github.com/joaotavora/eglot/pull/639
[-- Attachment #1: Type: text/plain, Size: 2385 bytes --] I just found out this bug was ongoing. Eli, if you're proposing to fix url-parse.el to not be fooled by windows file names, then I support that idea, and I think it's the correct thing to do. But.... we still need the eglot.el kludge installed because url-parse.el is not distributed as an ELPA package and Eglot is. So users of Emacs < 29 would not receive the fix and would have their WIndows Eglot broken. João On Fri, Nov 18, 2022 at 7:13 AM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Danny Freeman <danny@dfreeman.email> > > Cc: Eli Zaretskii <eliz@gnu.org>, Arash Esbati <arash@gnu.org>, > > 59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org > > Date: Thu, 17 Nov 2022 17:27:40 -0500 > > > > Is there something we can do to detect a windows path > > You mean, a Windows-style file name? You can detect that, but it is > easier to test system-type instead: these file names cannot happen on > any system except Windows, and if they do happen, they don't have the > same semantics (i.e., "d:/foo/bar" is NOT an absolute file name on > Posix systems). > > Or maybe I don't understand the purpose of the test you have in mind? > > > and continue treating it as a path like we were before this change? > > I'd advise against such kludges. If a function wants a file:// URL, > it should receive a valid file:// URL on all systems, and it should be > capable of handling file:// URLs on MS-Windows as well as on Posix > systems. Likewise with functions which produce file:// URLs. Letting > local file names into this is a clear path to future bugs, because > many people will not realize this subtlety, and will think they deal > with file:// URLs on all platforms. > > > If there is no function available already, it may be enough to check if > > the return value of `url-type` is not 1 character. Looking at this list > > of what I believe are official URI schemes, all of them have at least > > two characters: > > https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml > > But hosts can have 1-character names (although that is unlikely). > > Anyway, I'm against such kludges, especially since we don't need them > here. We just need to make our functions that handle file:// URLs to > be capable of supporting file:// on MS-Windows. It is not hard to do, > so let's do that. > > > > -- João Távora [-- Attachment #2: Type: text/html, Size: 3393 bytes --]
João Távora <joaotavora@gmail.com> writes: > I just found out this bug was ongoing. > > Eli, if you're proposing to fix url-parse.el to not be fooled by windows > file names, then I support that idea, and I think it's the correct > thing to do. > > But.... we still need the eglot.el kludge installed because url-parse.el > is not distributed as an ELPA package and Eglot is. So users of > Emacs < 29 would not receive the fix and would have their > WIndows Eglot broken. > > João > Should my patch for eglot be merged then? https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53 -- Danny Freeman
[-- Attachment #1: Type: text/plain, Size: 865 bytes --] I've just tested it on a Windows machine and pushed it, thanks. Closing this. João On Thu, Nov 24, 2022 at 1:44 PM Danny Freeman <danny@dfreeman.email> wrote: > > João Távora <joaotavora@gmail.com> writes: > > > I just found out this bug was ongoing. > > > > Eli, if you're proposing to fix url-parse.el to not be fooled by windows > > file names, then I support that idea, and I think it's the correct > > thing to do. > > > > But.... we still need the eglot.el kludge installed because url-parse.el > > is not distributed as an ELPA package and Eglot is. So users of > > Emacs < 29 would not receive the fix and would have their > > WIndows Eglot broken. > > > > João > > > > Should my patch for eglot be merged then? > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53 > > -- > Danny Freeman > -- João Távora [-- Attachment #2: Type: text/html, Size: 1485 bytes --]