bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

A corner case I've recently run into with comp-run-async-workers:

1. create a non-empty elisp file at /tmp/dir/test.el
2. visit /tmp/dir/test.el
3. delete /tmp/dir/
4. from within the test.el buffer `M-x 
emacs-lisp-native-compile-and-load`

The process will fail with:

> /tmp/dir/test.el: Opening input file, No such file or directory, 
> /tmp/dir/test.el

A similar edge case is accounted for when the buffer has no 
associated file.
e.g. attempting emacs-lisp-native-compile-and-load from the 
*scratch* buffer results in the user error: 

> The buffer must be saved in a file first.

Perhaps a similar check should be made in the case described 
above.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.36, cairo version 1.17.8) of 2023-02-06 built on nbook
Repository revision: 907fd1f7ff402f9d226ebb3b891ea5b54fac1d1c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 
11.0.12101006
System Description: Arch Linux

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: No Wayman <iarchivedmywholelife@gmail.com>
> Date: Mon, 06 Mar 2023 10:48:58 -0500
> 
> 
> A corner case I've recently run into with comp-run-async-workers:
> 
> 1. create a non-empty elisp file at /tmp/dir/test.el
> 2. visit /tmp/dir/test.el
> 3. delete /tmp/dir/
> 4. from within the test.el buffer `M-x 
> emacs-lisp-native-compile-and-load`
> 
> The process will fail with:
> 
> > /tmp/dir/test.el: Opening input file, No such file or directory, 
> > /tmp/dir/test.el
> 
> A similar edge case is accounted for when the buffer has no 
> associated file.
> e.g. attempting emacs-lisp-native-compile-and-load from the 
> *scratch* buffer results in the user error: 
> 
> > The buffer must be saved in a file first.
> 
> Perhaps a similar check should be made in the case described 
> above.

That's not the same check: the latter checks whether the buffer visits
a file, which in the former case is true.

I'm not sure we should do anything here: after all, the error message
tells what's wrong quite clearly.  Andrea, WDYT?

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

Eli Zaretskii <eliz@gnu.org> writes:

> That's not the same check

I'm aware. That's why I chose "similar" over "same".

> I'm not sure we should do anything here: after all, the error 
> message
> tells what's wrong quite clearly.

If it's the caller's responsibility to ensure a subprocess is 
invoked from an existing directory, fair enough.
This is one such case where that is not guaranteed, though.
I've seen similar errors pop up in other packages.
It would probably be better to offer a way to ensure the 
subprocess is run in an existing directory in general.
Is there an elisp idiom for such cases? e.g.

>(let ((default-directory (guaranteed-to-exist-directory)))
>  ;; start subprocess
> )

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: No Wayman <iarchivedmywholelife@gmail.com>
> Cc: Andrea Corallo <akrl@sdf.org>, 62004@debbugs.gnu.org
> Date: Mon, 06 Mar 2023 12:20:21 -0500
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm not sure we should do anything here: after all, the error 
> > message
> > tells what's wrong quite clearly.
> 
> If it's the caller's responsibility to ensure a subprocess is 
> invoked from an existing directory, fair enough.

I don't think this is about the directory where the subprocess is
invoked, I think this is about the .el file not being present.  Emacs
needs the file that is being natively-compiled to exist as a file on
disk.

Andrea, am I right?

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think this is about the directory where the subprocess 
> is
> invoked, I think this is about the .el file not being present. 
> Emacs
> needs the file that is being natively-compiled to exist as a 
> file on
> disk.

The example I gave used the emacs-lisp-native-compile-and-load 
directly because it is the simplest way to reproduce the error.
The same error can happen if the jit native compilation kicks in 
(because an entirely different feature is loaded but needs to be 
jit compiled) while in the buffer which no longer has a file 
associated with it.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: No Wayman <iarchivedmywholelife@gmail.com>
> Cc: akrl@sdf.org, 62004@debbugs.gnu.org
> Date: Mon, 06 Mar 2023 13:46:39 -0500
> 
> The example I gave used the emacs-lisp-native-compile-and-load 
> directly because it is the simplest way to reproduce the error.
> The same error can happen if the jit native compilation kicks in 
> (because an entirely different feature is loaded but needs to be 
> jit compiled) while in the buffer which no longer has a file 
> associated with it.

Sorry, I don't think I understand.  Are you saying that we don't bind
default-directory to a safe value when compiling?  IOW, how could a
directory where the async compilation subprocess runs become invalid,
in Real Life?

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, I don't think I understand.  Are you saying that we don't 
> bind
> default-directory to a safe value when compiling?

Correct. The default-directory is dependent on where 
comp-run-async-workers happens to kick off.
This can be reliably reproduced by:

1. saving the following elisp into test.el:

--8<---------------cut here---------------start------------->8---
;; -*- lexical-binding: t; -*-
(let* ((tempdir (expand-file-name "./temp/" user-emacs-directory))
       (default-directory tempdir)
       (feat 'org))
  ;; Ensure fresh test dir
  (when (file-exists-p tempdir) (delete-file tempdir))
  (make-directory tempdir)
  ;; Ensure test feature is not loaded.
  (when (featurep feat) (unload-feature feat t))
  (setq initial-buffer-choice
        (lambda ()
          (with-current-buffer (find-file (expand-file-name 
          "./temp.txt" tempdir))
            (insert "My directory will be deleted.")
            (write-file (expand-file-name "./temp.txt" tempdir))
            (delete-directory tempdir 'recursive)
            (message "default-directory: %S" default-directory)
            ;; comp-run-async-workers kicked off by JIT 
            compilation here.
            ;; This buffer has a file-name, but the directory no 
            longer exists.
            (require feat)
            (get-buffer-create (buffer-file-name))))))
--8<---------------cut here---------------end--------------->8---

2. launching emacs in a temporary init directory via:

> $ rm -rf /tmp/comp.test/ && emacs 
> --init-directory=/tmp/comp.test/ -l test.el

This should result in a *Messages* buffer similar to:

> For information about GNU Emacs and the GNU system, type C-h 
> C-a.
> (New file)
> Saving file /tmp/comp.test/temp/temp.txt...
> Wrote /tmp/comp.test/temp/temp.txt
> default-directory: "/tmp/comp.test/temp/"
> comp-run-async-workers: Setting current directory: No such file 
> or directory, /tmp/comp.test/temp/

>  IOW, how could a directory where the async compilation 
>  subprocess runs become invalid, in Real Life?

I ran into this error in the wild by:

- Installing a package to review it.
- Deleting the package's repository, but still had the package's 
  main elisp buffer open/current.
- Ran a command, which loaded a package, which kicked off the JIT 
  comp process.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: No Wayman <iarchivedmywholelife@gmail.com>
> Cc: akrl@sdf.org, 62004@debbugs.gnu.org
> Date: Mon, 06 Mar 2023 16:29:19 -0500
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sorry, I don't think I understand.  Are you saying that we don't 
> > bind
> > default-directory to a safe value when compiling?
> 
> Correct. The default-directory is dependent on where 
> comp-run-async-workers happens to kick off.

I'm not sure what would be a safe value for that.  We had a lot of
trouble in other cases where such a value was required.

Andrea, any ideas?  How come we never ran into this issue until now?
Is the default-directory value when native compilation is forked
somehow derived from the directory of the file being compiled?

> >  IOW, how could a directory where the async compilation 
> >  subprocess runs become invalid, in Real Life?
> 
> I ran into this error in the wild by:
> 
> - Installing a package to review it.
> - Deleting the package's repository, but still had the package's 
>   main elisp buffer open/current.
> - Ran a command, which loaded a package, which kicked off the JIT 
>   comp process.

That's a pretty unusual situation, IMO.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: No Wayman <iarchivedmywholelife@gmail.com>,  62004@debbugs.gnu.org
> Date: Tue, 07 Mar 2023 11:16:40 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Andrea, any ideas?  How come we never ran into this issue until now?
> 
> I guess it's a very unlikely condition that was never
> encountered/reported.
> 
> > Is the default-directory value when native compilation is forked
> > somehow derived from the directory of the file being compiled?
> 
> If it is is not evident to me why.  We spawan something of this kind
> 
> /pathtoemacs/emacs -Q -batch -l /tmp/emacs-async-comp-something.el
> 
> where emacs-async-comp-something.el contains the actual setup (where we
> don't touch default-directory) and compilation command.
> 
> I'm probably missing something ATM.

Well, maybe we should bind the variable to be on the safe side?  What
if we bind it to the directory where we write that
emacs-async-comp-something.el file?

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: No Wayman <iarchivedmywholelife@gmail.com>,  62004@debbugs.gnu.org
>> Date: Tue, 07 Mar 2023 11:16:40 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Andrea, any ideas?  How come we never ran into this issue until now?
>> 
>> I guess it's a very unlikely condition that was never
>> encountered/reported.
>> 
>> > Is the default-directory value when native compilation is forked
>> > somehow derived from the directory of the file being compiled?
>> 
>> If it is is not evident to me why.  We spawan something of this kind
>> 
>> /pathtoemacs/emacs -Q -batch -l /tmp/emacs-async-comp-something.el
>> 
>> where emacs-async-comp-something.el contains the actual setup (where we
>> don't touch default-directory) and compilation command.
>> 
>> I'm probably missing something ATM.
>
> Well, maybe we should bind the variable to be on the safe side?  What
> if we bind it to the directory where we write that
> emacs-async-comp-something.el file?

Maybe but the reporter says "The default-directory is dependent on where
comp-run-async-workers happens to kick off." and I don't understand if
that's correct why is that.

I'd like first to understand better the issue here.

  Andrea

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: iarchivedmywholelife@gmail.com, 62004@debbugs.gnu.org
> Date: Tue, 07 Mar 2023 13:51:37 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Well, maybe we should bind the variable to be on the safe side?  What
> > if we bind it to the directory where we write that
> > emacs-async-comp-something.el file?
> 
> Maybe but the reporter says "The default-directory is dependent on where
> comp-run-async-workers happens to kick off." and I don't understand if
> that's correct why is that.
> 
> I'd like first to understand better the issue here.

I agree that we should first have a good understanding of the
situation.  Let me know if I can help in any way.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Well, maybe we should bind the variable to be on the safe 
>> > side?  What
>> > if we bind it to the directory where we write that
>> > emacs-async-comp-something.el file?

Binding default-directory makes sense.
It's just a matter of what to bind it to.
I see that `comp-run-async-workers` calls `make-temp-file' 
internally.
Binding default-directory to temporary-file-directory around the 
call to `make-process' will prevent this error and seems like a 
safe bet.

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: iarchivedmywholelife@gmail.com, 62004@debbugs.gnu.org
>> Date: Tue, 07 Mar 2023 13:51:37 +0000
>>
>> Maybe but the reporter says "The default-directory is dependent 
>> on where
>> comp-run-async-workers happens to kick off." and I don't 
>> understand if
>> that's correct why is that.
>> 
>> I'd like first to understand better the issue here.
>
> I agree that we should first have a good understanding of the
> situation.  Let me know if I can help in any way.

The call to `make-process' in `comp-run-async-workers' is executed 
in the context of whatever default-directory happens to be. If 
default-directory does not refer to an existing directory (as 
demonstrated in the reproduction case I provided) the creation of 
the subprocess will fail.
I'm not sure what more detail I can provide, so please ask a 
specific question if you have any.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]


No Wayman <iarchivedmywholelife@gmail.com> writes:

> Binding default-directory makes sense.
> It's just a matter of what to bind it to.
> I see that `comp-run-async-workers` calls `make-temp-file' 
> internally.
> Binding default-directory to temporary-file-directory around the 
> call to
> `make-process' will prevent this error and seems like a safe 
> bet.

The attached patch implements this and fixes the error on my end. 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-comp.el-comp-run-async-workers-bind-default-director.patch --]
[-- Type: text/x-patch, Size: 969 bytes --]

From fd33c2d58ac078ed53cdada5fa6e378e59247a3a Mon Sep 17 00:00:00 2001
From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
Date: Tue, 7 Mar 2023 10:44:17 -0500
Subject: [PATCH] comp.el (comp-run-async-workers): bind default-directory

Ensure default-directory exists prior to creating subprocess. (bug#62004)
---
 lisp/emacs-lisp/comp.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index ce81680a226..97cfa27a1aa 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -4023,6 +4023,7 @@ comp-run-async-workers
                         (comp-log "\n")
                         (mapc #'comp-log expr-strings)))
                    (load1 load)
+                   (default-directory temporary-file-directory)
                    (process (make-process
                              :name (concat "Compiling: " source-file)
                              :buffer (with-current-buffer
-- 
2.39.1

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Andrea Corallo]

No Wayman <iarchivedmywholelife@gmail.com> writes:

>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>  > Well, maybe we should bind the variable to be on the safe  >
>>> side?  What
>>> > if we bind it to the directory where we write that
>>> > emacs-async-comp-something.el file?
>
> Binding default-directory makes sense.
> It's just a matter of what to bind it to.
> I see that `comp-run-async-workers` calls `make-temp-file' internally.
> Binding default-directory to temporary-file-directory around the call
> to `make-process' will prevent this error and seems like a safe bet.
>
>>> From: Andrea Corallo <akrl@sdf.org>
>>> Cc: iarchivedmywholelife@gmail.com, 62004@debbugs.gnu.org
>>> Date: Tue, 07 Mar 2023 13:51:37 +0000
>>>
>>> Maybe but the reporter says "The default-directory is dependent on
>>> where
>>> comp-run-async-workers happens to kick off." and I don't understand
>>> if
>>> that's correct why is that.
>>> I'd like first to understand better the issue here.
>>
>> I agree that we should first have a good understanding of the
>> situation.  Let me know if I can help in any way.
>
> The call to `make-process' in `comp-run-async-workers' is executed in
> the context of whatever default-directory happens to be. If
> default-directory does not refer to an existing directory (as
> demonstrated in the reproduction case I provided) the creation of the
> subprocess will fail.
> I'm not sure what more detail I can provide, so please ask a specific
> question if you have any.

Ah now it's clear to me, the error is not happening in the child process
but in the main Emacs failing in running `make-process'.

Thanks

  Andrea

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Andrea Corallo]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
>> Binding default-directory makes sense.
>> It's just a matter of what to bind it to.
>> I see that `comp-run-async-workers` calls `make-temp-file'
>> internally.
>> Binding default-directory to temporary-file-directory around the
>> call to
>> `make-process' will prevent this error and seems like a safe bet.
>
> The attached patch implements this and fixes the error on my end. >From fd33c2d58ac078ed53cdada5fa6e378e59247a3a Mon Sep 17 00:00:00 2001
> From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
> Date: Tue, 7 Mar 2023 10:44:17 -0500
> Subject: [PATCH] comp.el (comp-run-async-workers): bind default-directory
>
> Ensure default-directory exists prior to creating subprocess. (bug#62004)
> ---
>  lisp/emacs-lisp/comp.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> index ce81680a226..97cfa27a1aa 100644
> --- a/lisp/emacs-lisp/comp.el
> +++ b/lisp/emacs-lisp/comp.el
> @@ -4023,6 +4023,7 @@ comp-run-async-workers
>                          (comp-log "\n")
>                          (mapc #'comp-log expr-strings)))
>                     (load1 load)
> +                   (default-directory temporary-file-directory)
>                     (process (make-process
>                               :name (concat "Compiling: " source-file)
>                               :buffer (with-current-buffer

If tested LGTM, another option (maybe safer?) would be to use
`invocation-directory'.

Thanks

  Andrea

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 62004@debbugs.gnu.org
> Date: Tue, 07 Mar 2023 16:06:56 +0000
> 
> No Wayman <iarchivedmywholelife@gmail.com> writes:
> 
> > No Wayman <iarchivedmywholelife@gmail.com> writes:
> >
> >> Binding default-directory makes sense.
> >> It's just a matter of what to bind it to.
> >> I see that `comp-run-async-workers` calls `make-temp-file'
> >> internally.
> >> Binding default-directory to temporary-file-directory around the
> >> call to
> >> `make-process' will prevent this error and seems like a safe bet.
> >
> > The attached patch implements this and fixes the error on my end. >From fd33c2d58ac078ed53cdada5fa6e378e59247a3a Mon Sep 17 00:00:00 2001
> > From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
> > Date: Tue, 7 Mar 2023 10:44:17 -0500
> > Subject: [PATCH] comp.el (comp-run-async-workers): bind default-directory
> >
> > Ensure default-directory exists prior to creating subprocess. (bug#62004)
> > ---
> >  lisp/emacs-lisp/comp.el | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> > index ce81680a226..97cfa27a1aa 100644
> > --- a/lisp/emacs-lisp/comp.el
> > +++ b/lisp/emacs-lisp/comp.el
> > @@ -4023,6 +4023,7 @@ comp-run-async-workers
> >                          (comp-log "\n")
> >                          (mapc #'comp-log expr-strings)))
> >                     (load1 load)
> > +                   (default-directory temporary-file-directory)
> >                     (process (make-process
> >                               :name (concat "Compiling: " source-file)
> >                               :buffer (with-current-buffer
> 
> If tested LGTM, another option (maybe safer?) would be to use
> `invocation-directory'.

I think I'd prefer invocation-directory, indeed.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

Eli Zaretskii <eliz@gnu.org> writes:

>> If tested LGTM, another option (maybe safer?) would be to use
>> `invocation-directory'.
>
> I think I'd prefer invocation-directory, indeed.

I don't see how temporary-file-directory is any less safe 
considering the function relies on make-temp-file, which uses that 
directory itself.
But feel free to alter the patch however you see fit.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Andrea Corallo]

Hi,

8a2a554192a should fix in 29.

Please let us know if we can close this.

Best Regards

  Andrea

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[No Wayman]

Andrea Corallo <akrl@sdf.org> writes:

> Hi,
>
> 8a2a554192a should fix in 29.
>
> Please let us know if we can close this.
>
> Best Regards
>
>   Andrea

I won't have time to install the patch today, but it stands to 
reason that it will solve the problem.
I would consider this closed.
Thanks.

bug#62004: 30.0.50; comp-run-async-workers failure when default-directory deleted

[Andrea Corallo]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>> Hi,
>>
>> 8a2a554192a should fix in 29.
>>
>> Please let us know if we can close this.
>>
>> Best Regards
>>
>>   Andrea
>
> I won't have time to install the patch today, but it stands to reason
> that it will solve the problem.
> I would consider this closed.
> Thanks.

Done