GNU bug report logs - #49283
[PATCH] 27.2; `(call-process "program" null-device ...)' fails over TRAMP from local MS Windows

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49283 in the body.
You can then email your comments to 49283 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] 27.2; `(call-process "program" null-device ...)' fails over
 TRAMP from local MS Windows
Date: Tue, 29 Jun 2021 22:14:19 -0700

[Message part 1 (text/plain, inline)]

I noticed an issue when trying to use flyspell (or ispell) using
hunspell from a local MS Windows system on a TRAMP file. It results in
an error that it can't find the file "/sshx:server:/path/to/NUL". I
narrowed this down to the fact that
`ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
set to `null-device'. To see this in action:

  emacs -Q
  C-x C-f /sshx:server:~/path/to/file.txt
  M-: (setq ispell-program-name "hunspell") RET
  M-x flyspell-mode
  ;; or...
  M-: (call-process "something" null-device) RET

This results in the following error:

----------------------------------------
Debugger entered--Lisp error: (file-missing "Opening process input
file" #("No such file or directory" 0 25 (charset windows-1252))
"/sshx:server:/path/to/NUL")
  call-process("something" "NUL")
  eval((call-process "something" null-device) t)
  eval-expression((call-process "something" null-device) nil nil 127)
  funcall-interactively(eval-expression (call-process "something"
null-device) nil nil 127)
  call-interactively(eval-expression nil nil)
  command-execute(eval-expression)
----------------------------------------

It seems this is a result of the fact that `null-device' on MS Windows
is "NUL", and `(expand-file-name "NUL")' is "<default-directory>/NUL".
When `default-directory' is a local MS Windows path, this is ok, but
when it's a TRAMP path, it looks for a real file named NUL on the
remote (GNU/Linux) machine. However, since `call-process' executes
from the (local) home directory if `default-directory' is a TRAMP
path, I think it makes more sense for `infile' to be interpreted
relative to the homedir too.

I've attached a speculative patch that I think fixes this. (Note: I
don't have an MS Windows build environment set up at the moment, so I
only tested that this works like I'd expect from GNU/Linux. It'd
probably be good to make sure it works on MS Windows too.)

While I'm hesitant to touch something as low-level as `call-process',
I think fixing this in general would be the best long-term solution,
assuming it doesn't break something I'm unaware of. Another,
less-invasive fix would be to fix `ispell-find-hunspell-dictionaries'
to pass `nil' as the `infile' when invoking hunspell.

[0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch (application/octet-stream, attachment)]

Message #8 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 49283 <at> debbugs.gnu.org
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Wed, 30 Jun 2021 09:24:47 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> I noticed an issue when trying to use flyspell (or ispell) using
> hunspell from a local MS Windows system on a TRAMP file. It results in
> an error that it can't find the file "/sshx:server:/path/to/NUL". I
> narrowed this down to the fact that
> `ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
> set to `null-device'.

`call-process' doesn't know of remote files. You must use `process-file' instead.

> To see this in action:
>
>   emacs -Q
>   C-x C-f /sshx:server:~/path/to/file.txt
>   M-: (setq ispell-program-name "hunspell") RET
>   M-x flyspell-mode
>   ;; or...
>   M-: (call-process "something" null-device) RET

It is not a good idea to use `null-device' as INFILE, just use nil. At
least in the `process-file' case, Tramp shall know which value to take
for `null-device'.

After all, it seems to be rather a flyspell/ispell feature request to
support remote files.

Best regards, Michael.

Message #11 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 49283 <at> debbugs.gnu.org
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Wed, 30 Jun 2021 10:16:28 -0700

[Message part 1 (text/plain, inline)]

On Wed, Jun 30, 2021 at 12:24 AM Michael Albinus <michael.albinus <at> gmx.de> wrote:
>
> Jim Porter <jporterbugs <at> gmail.com> writes:
>
> > I noticed an issue when trying to use flyspell (or ispell) using
> > hunspell from a local MS Windows system on a TRAMP file. It results in
> > an error that it can't find the file "/sshx:server:/path/to/NUL". I
> > narrowed this down to the fact that
> > `ispell-find-hunspell-dictionaries' calls `call-process' with `infile'
> > set to `null-device'.
>
> `call-process' doesn't know of remote files. You must use `process-file' instead.

That's not a problem; it's actually the right thing to do in this
case, I think. flyspell/ispell is trying to use my local version of
hunspell on the contents of a remote buffer. Since flyspell/ispell
just look at the buffer contents and not the actual file, it can use
`call-process'.

> It is not a good idea to use `null-device' as INFILE, just use nil. At
> least in the `process-file' case, Tramp shall know which value to take
> for `null-device'.

That fix would also work (see the attached patch).

However, when I read the `call-process' documentation, it says that
when `default-directory' is remote, it runs the program from "~".
That's fine overall, and means it's a good way to be sure you're
always running a process locally. It's just that when you do this,
INFILE's path is expanded relative to the remote directory. I don't
think that can ever work, since `call-process' doesn't know how to
open a TRAMP file. Because of that, it makes more sense to me that
you'd expand INFILE's path relative to wherever PROGRAM will be run
from. That means that when `default-directory' is remote, both PROGRAM
and INFILE are expanded relative to "~". That's more consistent, and
my first patch would hopefully prevent similar errors anytime
`call-process' is used from a remote buffer.

[0001-Don-t-pass-null-device-to-call-process-in-ispell.patch (application/octet-stream, attachment)]

Message #14 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 49283 <at> debbugs.gnu.org, Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Thu, 01 Jul 2021 13:07:54 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

>> It is not a good idea to use `null-device' as INFILE, just use nil. At
>> least in the `process-file' case, Tramp shall know which value to take
>> for `null-device'.
>
> That fix would also work (see the attached patch).

Applied to Emacs 28 now.

> However, when I read the `call-process' documentation, it says that
> when `default-directory' is remote, it runs the program from "~".
> That's fine overall, and means it's a good way to be sure you're
> always running a process locally. It's just that when you do this,
> INFILE's path is expanded relative to the remote directory. I don't
> think that can ever work, since `call-process' doesn't know how to
> open a TRAMP file. Because of that, it makes more sense to me that
> you'd expand INFILE's path relative to wherever PROGRAM will be run
> from. That means that when `default-directory' is remote, both PROGRAM
> and INFILE are expanded relative to "~". That's more consistent, and
> my first patch would hopefully prevent similar errors anytime
> `call-process' is used from a remote buffer.

But is that what your first patch does?

* src/callproc.c (Fcall_process): Interpret 'infile' relative to the
directory from which 'program' is called, not 'default-directory'.
---
 src/callproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/callproc.c b/src/callproc.c
index aabc39313b..57cf781d28 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -270,7 +270,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
 
   if (nargs >= 2 && ! NILP (args[1]))
     {
-      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
+      infile = Fexpand_file_name (args[1], encode_current_directory ());
       CHECK_STRING (infile);



-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #17 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 49283 <at> debbugs.gnu.org
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Thu, 01 Jul 2021 14:26:08 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

Hi,

>> However, when I read the `call-process' documentation, it says that
>> when `default-directory' is remote, it runs the program from "~".
>> That's fine overall, and means it's a good way to be sure you're
>> always running a process locally. It's just that when you do this,
>> INFILE's path is expanded relative to the remote directory. I don't
>> think that can ever work, since `call-process' doesn't know how to
>> open a TRAMP file. Because of that, it makes more sense to me that
>> you'd expand INFILE's path relative to wherever PROGRAM will be run
>> from. That means that when `default-directory' is remote, both PROGRAM
>> and INFILE are expanded relative to "~". That's more consistent, and
>> my first patch would hopefully prevent similar errors anytime
>> `call-process' is used from a remote buffer.
>
> But is that what your first patch does?
>
> * src/callproc.c (Fcall_process): Interpret 'infile' relative to the
> directory from which 'program' is called, not 'default-directory'.
> ---
>  src/callproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/callproc.c b/src/callproc.c
> index aabc39313b..57cf781d28 100644
> --- a/src/callproc.c
> +++ b/src/callproc.c
> @@ -270,7 +270,7 @@ DEFUN ("call-process", Fcall_process, Scall_process, 1, MANY, 0,
>
>    if (nargs >= 2 && ! NILP (args[1]))
>      {
> -      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
> +      infile = Fexpand_file_name (args[1], encode_current_directory ());
>        CHECK_STRING (infile);

Yes, this seems TRT. encode_current_directory returns either
default-directory if this is a local dir, or "~" otherwise. Expanding
INFILE to that directory is OK, I believe.

So we shall apply Jim's patch. Maybe the docstring could be enhanced a
little bit at the end, saying that INFILE, if it is a relative file
name, is expanded to the directory the process uses as cwd.

Message #20 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 49283 <at> debbugs.gnu.org
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Thu, 01 Jul 2021 14:34:35 +0200

Michael Albinus <michael.albinus <at> gmx.de> writes:

>> * src/callproc.c (Fcall_process): Interpret 'infile' relative to the
>> directory from which 'program' is called, not 'default-directory'.

[...]

> Yes, this seems TRT. encode_current_directory returns either
> default-directory if this is a local dir, or "~" otherwise. Expanding
> INFILE to that directory is OK, I believe.
>
> So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> little bit at the end, saying that INFILE, if it is a relative file
> name, is expanded to the directory the process uses as cwd.

Oh, I interpreted the commit message as "the directory where 'program'
is" and the patch didn't seem to do that.

But I still don't understand "the directory from which 'program' is
called", because that's `default-directory'.  (Except, as you say, when
it's a remote dir.)

The patch looks good to me, too, but the commit message is confusing.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #23 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: jporterbugs <at> gmail.com, larsi <at> gnus.org, 49283 <at> debbugs.gnu.org
Subject: Re: bug#49283: [PATCH] 27.2;
 `(call-process "program" null-device ...)' fails over TRAMP from
 local MS Windows
Date: Thu, 01 Jul 2021 16:12:25 +0300

> From: Michael Albinus <michael.albinus <at> gmx.de>
> Date: Thu, 01 Jul 2021 14:26:08 +0200
> Cc: Jim Porter <jporterbugs <at> gmail.com>, 49283 <at> debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> 
> > -      infile = Fexpand_file_name (args[1], BVAR (current_buffer, directory));
> > +      infile = Fexpand_file_name (args[1], encode_current_directory ());
> >        CHECK_STRING (infile);
> 
> Yes, this seems TRT. encode_current_directory returns either
> default-directory if this is a local dir, or "~" otherwise. Expanding
> INFILE to that directory is OK, I believe.
> 
> So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> little bit at the end, saying that INFILE, if it is a relative file
> name, is expanded to the directory the process uses as cwd.

encode_current_directory returns an encoded file name.  So if we make
this change, we should avoid calling ENCODE_FILE on it (doing so is a
no-op, but it's still unclean).

Message #26 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 49283 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Thu, 1 Jul 2021 12:45:42 -0700

[Message part 1 (text/plain, inline)]

On Thu, Jul 1, 2021 at 6:12 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Michael Albinus <michael.albinus <at> gmx.de>
> > Date: Thu, 01 Jul 2021 14:26:08 +0200
> > Cc: Jim Porter <jporterbugs <at> gmail.com>, 49283 <at> debbugs.gnu.org
> >
> > So we shall apply Jim's patch. Maybe the docstring could be enhanced a
> > little bit at the end, saying that INFILE, if it is a relative file
> > name, is expanded to the directory the process uses as cwd.
>
> encode_current_directory returns an encoded file name.  So if we make
> this change, we should avoid calling ENCODE_FILE on it (doing so is a
> no-op, but it's still unclean).

I'd considered that when writing my initial patch to `call-process',
but I wasn't sure what the most-correct way to avoid that would be. It
seems we want an encoded path before returning from
`encode_current_directory' in order to check that our result is
actually accessible. But then that encoded dir gets passed in to
`expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
`expand-file-name' be un-encoded as well?

Maybe a better way would be to get the cwd *without* encoding it (see
the attached patch). However, maybe there's a simpler answer to all of
this that I just don't know about since I'm not very familiar with how
Emacs encodes file names.

[0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch (application/octet-stream, attachment)]

Message #29 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 49283 <at> debbugs.gnu.org, larsi <at> gnus.org, michael.albinus <at> gmx.de
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Fri, 02 Jul 2021 09:37:49 +0300

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Thu, 1 Jul 2021 12:45:42 -0700
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, Lars Ingebrigtsen <larsi <at> gnus.org>, 49283 <at> debbugs.gnu.org
> 
> > encode_current_directory returns an encoded file name.  So if we make
> > this change, we should avoid calling ENCODE_FILE on it (doing so is a
> > no-op, but it's still unclean).
> 
> I'd considered that when writing my initial patch to `call-process',
> but I wasn't sure what the most-correct way to avoid that would be. It
> seems we want an encoded path before returning from
> `encode_current_directory' in order to check that our result is
> actually accessible. But then that encoded dir gets passed in to
> `expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
> `expand-file-name' be un-encoded as well?

expand-file-name is not a problem, it can deal with encoded file
names.  The problem is the calls to remove_slash_colon and
report_file_error: they should receive file names in their internal
representation.

> Maybe a better way would be to get the cwd *without* encoding it (see
> the attached patch). However, maybe there's a simpler answer to all of
> this that I just don't know about since I'm not very familiar with how
> Emacs encodes file names.

How about adding a 'bool' argument to encode_current_directory, so
that the caller could control whether or not it encodes the directory
file name?  Then you could in this case tell encode_current_directory
not to encode the directory file name.

Message #32 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 49283 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Fri, 2 Jul 2021 11:47:12 -0700

[Message part 1 (text/plain, inline)]

On Thu, Jul 1, 2021 at 11:37 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Jim Porter <jporterbugs <at> gmail.com>
> > Date: Thu, 1 Jul 2021 12:45:42 -0700
> > Cc: Michael Albinus <michael.albinus <at> gmx.de>, Lars Ingebrigtsen <larsi <at> gnus.org>, 49283 <at> debbugs.gnu.org
> >
> > I'd considered that when writing my initial patch to `call-process',
> > but I wasn't sure what the most-correct way to avoid that would be. It
> > seems we want an encoded path before returning from
> > `encode_current_directory' in order to check that our result is
> > actually accessible. But then that encoded dir gets passed in to
> > `expand-file-name'. If INFILE is an un-encoded absolute path, wouldn't
> > `expand-file-name' be un-encoded as well?
>
> expand-file-name is not a problem, it can deal with encoded file
> names.  The problem is the calls to remove_slash_colon and
> report_file_error: they should receive file names in their internal
> representation.

Right, I was just worried that if I relied on
`encode_current_directory' returning an encoded path,
`expand-file-name' might sometimes return an encoded path (e.g. if
INFILE is a simple relative path like "foo") and sometimes an
unencoded path (e.g. if INFILE is an absolute path). I might be wrong
though, since I didn't look closely at the implementation...

> How about adding a 'bool' argument to encode_current_directory, so
> that the caller could control whether or not it encodes the directory
> file name?  Then you could in this case tell encode_current_directory
> not to encode the directory file name.

Ok, I did that (and renamed it to `get_current_directory' since it
doesn't always encode anymore). How does the attached patch look?

[0001-Ensure-call-process-interprets-infile-as-a-local-pat.patch (application/octet-stream, attachment)]

Message #35 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 49283 <at> debbugs.gnu.org, larsi <at> gnus.org, michael.albinus <at> gmx.de
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Sat, 03 Jul 2021 10:02:32 +0300

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 2 Jul 2021 11:47:12 -0700
> Cc: Michael Albinus <michael.albinus <at> gmx.de>, Lars Ingebrigtsen <larsi <at> gnus.org>, 49283 <at> debbugs.gnu.org
> 
> > expand-file-name is not a problem, it can deal with encoded file
> > names.  The problem is the calls to remove_slash_colon and
> > report_file_error: they should receive file names in their internal
> > representation.
> 
> Right, I was just worried that if I relied on
> `encode_current_directory' returning an encoded path,
> `expand-file-name' might sometimes return an encoded path (e.g. if
> INFILE is a simple relative path like "foo") and sometimes an
> unencoded path (e.g. if INFILE is an absolute path). I might be wrong
> though, since I didn't look closely at the implementation...
> 
> > How about adding a 'bool' argument to encode_current_directory, so
> > that the caller could control whether or not it encodes the directory
> > file name?  Then you could in this case tell encode_current_directory
> > not to encode the directory file name.
> 
> Ok, I did that (and renamed it to `get_current_directory' since it
> doesn't always encode anymore). How does the attached patch look?

LGTM, thanks.

Message #38 received at 49283 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 49283 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Michael Albinus <michael.albinus <at> gmx.de>
Subject: Re: bug#49283: [PATCH] 27.2; `(call-process "program" null-device
 ...)' fails over TRAMP from local MS Windows
Date: Sun, 04 Jul 2021 15:32:35 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> Ok, I did that (and renamed it to `get_current_directory' since it
> doesn't always encode anymore). How does the attached patch look?

Looks good; so I've now pushed it to Emacs 28, and with that I'm closing
this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.