bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

Any objection to installing this patch into `emacs-28`?


        Stefan


 In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2022-08-29 built on pastel
Repository revision: b4cabfe2b1be20c2d896da81a8e408b57612905b
Repository branch: work
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure -C --enable-checking --enable-check-lisp-object-type --with-modules --with-cairo --with-tiff=ifavailable
 'CFLAGS=-Wall -g3 -Og -Wno-pointer-sign'
 PKG_CONFIG_PATH=/home/monnier/lib/pkgconfig'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-emacs-lisp-comp.el-comp-run-async-workers-Fail-.patch --]
[-- Type: text/patch, Size: 3524 bytes --]

From d0a989debc23ddeb692c120fb4aa5b8568e3c754 Mon Sep 17 00:00:00 2001
From: Stefan Monnier <monnier@iro.umontreal.ca>
Date: Sat, 3 Sep 2022 10:51:17 -0400
Subject: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more
 gracefully

Otherwise Emacs may fail to start if it can't find a writable
`~/.emacs.d/eln-cache` directory.
See Debian's bug #1017739.
---
 lisp/emacs-lisp/comp.el | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 306ec918b1a..6e2564236db 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -1370,19 +1370,22 @@ comp-spill-lap-function
   (unless byte-to-native-top-level-forms
     (signal 'native-compiler-error-empty-byte filename))
   (unless (comp-ctxt-output comp-ctxt)
-    (setf (comp-ctxt-output comp-ctxt) (comp-el-to-eln-filename
-                                        filename
-                                        (or native-compile-target-directory
-                                            (when byte+native-compile
-                                              (car (last native-comp-eln-load-path)))))))
+    (setf (comp-ctxt-output comp-ctxt)
+          (comp-el-to-eln-filename
+           filename
+           (or native-compile-target-directory
+               (when byte+native-compile
+                 (car (last native-comp-eln-load-path)))))))
   (setf (comp-ctxt-speed comp-ctxt) (alist-get 'native-comp-speed
                                                byte-native-qualities)
         (comp-ctxt-debug comp-ctxt) (alist-get 'native-comp-debug
                                                byte-native-qualities)
-        (comp-ctxt-compiler-options comp-ctxt) (alist-get 'native-comp-compiler-options
-                                                        byte-native-qualities)
-        (comp-ctxt-driver-options comp-ctxt) (alist-get 'native-comp-driver-options
-                                                        byte-native-qualities)
+        (comp-ctxt-compiler-options comp-ctxt)
+        (alist-get 'native-comp-compiler-options
+                   byte-native-qualities)
+        (comp-ctxt-driver-options comp-ctxt)
+        (alist-get 'native-comp-driver-options
+                   byte-native-qualities)
         (comp-ctxt-top-level-forms comp-ctxt)
         (cl-loop
          for form in (reverse byte-to-native-top-level-forms)
@@ -3933,10 +3936,13 @@ comp-run-async-workers
                        "`comp-files-queue' should be \".el\" files: %s"
                        source-file)
          when (or native-comp-always-compile
-                  load ; Always compile when the compilation is
-                       ; commanded for late load.
-                  (file-newer-than-file-p
-                   source-file (comp-el-to-eln-filename source-file)))
+                  load        ; Always compile when the compilation is
+                              ; commanded for late load.
+                  ;; Skip compilation if `comp-el-to-eln-filename' fails
+                  ;; to find a writable directory.
+                  (with-demoted-errors "Async compilation :%S"
+                    (file-newer-than-file-p
+                     source-file (comp-el-to-eln-filename source-file))))
          do (let* ((expr `((require 'comp)
                            (setq comp-async-compilation t)
                            (setq warning-fill-column most-positive-fixnum)
-- 
2.30.2

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> Date: Sat, 03 Sep 2022 11:00:09 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Any objection to installing this patch into `emacs-28`?

I have trouble seeing the actual change amongst unrelated whitespace
changes.  Please show a patch with the minimal necessary change,
without all the reformatting.  Emacs 28.2 is so close to a release
that playing whitespace and formatting games is not appropriate there.

Thanks.

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> I have trouble seeing the actual change amongst unrelated whitespace
> changes.  Please show a patch with the minimal necessary change,
> without all the reformatting.  Emacs 28.2 is so close to a release
> that playing whitespace and formatting games is not appropriate there.

Hmm... looks like I sent the patch I had on `master`, sorry.
Here is the cleaner `emacs-28` version.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: comp.patch --]
[-- Type: text/x-diff, Size: 1345 bytes --]

commit 516d2ea5e958866120f1c2d04771c04d1f5a4425
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Sat Sep 3 11:03:01 2022 -0400

    * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully
    
    Otherwise Emacs may fail to start if it can't find a writable
    `~/.emacs.d/eln-cache` directory.
    See Debian's bug #1017739.

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 374b39e9990..a5ab12ae388 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3927,8 +3927,11 @@ comp-run-async-workers
          when (or native-comp-always-compile
                   load ; Always compile when the compilation is
                        ; commanded for late load.
-                  (file-newer-than-file-p
-                   source-file (comp-el-to-eln-filename source-file)))
+                  ;; Skip compilation if `comp-el-to-eln-filename' fails
+                  ;; to find a writable directory.
+                  (with-demoted-errors "Async compilation :%S"
+                    (file-newer-than-file-p
+                     source-file (comp-el-to-eln-filename source-file))))
          do (let* ((expr `((require 'comp)
                            ,(when (boundp 'backtrace-line-length)
                               `(setf backtrace-line-length ,backtrace-line-length))

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I have trouble seeing the actual change amongst unrelated whitespace
> changes.  Please show a patch with the minimal necessary change,
> without all the reformatting.  Emacs 28.2 is so close to a release
> that playing whitespace and formatting games is not appropriate there.

BTW, it might be worth checking with the Debian maintainers to make sure
we fix the problem they bumped into while preparing the `emacs` package
for 28.1 before we release 28.2.

I think they also found serious problems when you use a build with native
compilation enabled but where the `.el` files are missing (which is not
the "recommended" situation in Debian, but is quite common).


        Stefan

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57562@debbugs.gnu.org
> Date: Sat, 03 Sep 2022 11:24:02 -0400
> 
> > I have trouble seeing the actual change amongst unrelated whitespace
> > changes.  Please show a patch with the minimal necessary change,
> > without all the reformatting.  Emacs 28.2 is so close to a release
> > that playing whitespace and formatting games is not appropriate there.
> 
> Hmm... looks like I sent the patch I had on `master`, sorry.
> Here is the cleaner `emacs-28` version.

So this means we will silently skip native-compilation, leaving an
obscure message in a buffer, is that right?  Maybe this is the best we
can do for emacs-28 so close to the release, but for master wouldn't
it be better to use delayed-warning instead?

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57562@debbugs.gnu.org
> Date: Sat, 03 Sep 2022 11:27:31 -0400
> 
> > I have trouble seeing the actual change amongst unrelated whitespace
> > changes.  Please show a patch with the minimal necessary change,
> > without all the reformatting.  Emacs 28.2 is so close to a release
> > that playing whitespace and formatting games is not appropriate there.
> 
> BTW, it might be worth checking with the Debian maintainers to make sure
> we fix the problem they bumped into while preparing the `emacs` package
> for 28.1 before we release 28.2.
> 
> I think they also found serious problems when you use a build with native
> compilation enabled but where the `.el` files are missing (which is not
> the "recommended" situation in Debian, but is quite common).

It's too bad they don't report these issues back when they are
discovered.  Emacs 28.1 was released 5 months ago!  From my POV, Emacs
28.2 is ready to be released in a matter of days, and waiting for
Debian to report and then waiting more to resolve those issues is not
my idea of fun.

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > I have trouble seeing the actual change amongst unrelated whitespace
>> > changes.  Please show a patch with the minimal necessary change,
>> > without all the reformatting.  Emacs 28.2 is so close to a release
>> > that playing whitespace and formatting games is not appropriate there.
>> Hmm... looks like I sent the patch I had on `master`, sorry.
>> Here is the cleaner `emacs-28` version.
> So this means we will silently skip native-compilation, leaving an
> obscure message in a buffer, is that right?

In *Messages*, I think, yes.

I suspect this message will tend to be repeated later on, whenever
a new `.elc` file is loaded.  In a sense that's good (less chance that
the message will be overlooked) but it also risks hiding other messages.

> Maybe this is the best we can do for emacs-28 so close to the release,
> but for master wouldn't it be better to use delayed-warning instead?

Sounds about right, tho I suspect we might need to do even a bit more on
`master`, like disabling lazy native compilation when we bump into this
problem, to avoid repeating that same error message, or trying harder to
find a writable directory.


        Stefan

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57562@debbugs.gnu.org
> Date: Sat, 03 Sep 2022 11:41:14 -0400
> 
> >> > I have trouble seeing the actual change amongst unrelated whitespace
> >> > changes.  Please show a patch with the minimal necessary change,
> >> > without all the reformatting.  Emacs 28.2 is so close to a release
> >> > that playing whitespace and formatting games is not appropriate there.
> >> Hmm... looks like I sent the patch I had on `master`, sorry.
> >> Here is the cleaner `emacs-28` version.
> > So this means we will silently skip native-compilation, leaving an
> > obscure message in a buffer, is that right?
> 
> In *Messages*, I think, yes.

I guess that's enough for Emacs 28.  (And I do wonder how come people
come up with unwritable home directories.)

> > Maybe this is the best we can do for emacs-28 so close to the release,
> > but for master wouldn't it be better to use delayed-warning instead?
> 
> Sounds about right, tho I suspect we might need to do even a bit more on
> `master`, like disabling lazy native compilation when we bump into this
> problem, to avoid repeating that same error message, or trying harder to
> find a writable directory.

I'm more bothered about letting the user know than about working
around the problem.

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: 57562@debbugs.gnu.org
>> Date: Sat, 03 Sep 2022 11:41:14 -0400
>> 
>> >> > I have trouble seeing the actual change amongst unrelated whitespace
>> >> > changes.  Please show a patch with the minimal necessary change,
>> >> > without all the reformatting.  Emacs 28.2 is so close to a release
>> >> > that playing whitespace and formatting games is not appropriate there.
>> >> Hmm... looks like I sent the patch I had on `master`, sorry.
>> >> Here is the cleaner `emacs-28` version.
>> > So this means we will silently skip native-compilation, leaving an
>> > obscure message in a buffer, is that right?
>> 
>> In *Messages*, I think, yes.
>
> I guess that's enough for Emacs 28.  (And I do wonder how come people
> come up with unwritable home directories.)

In the discussion of that Debian bug#1017739, Russ Allbery mentions
a(n unrelated) problem which can lead to this:

    % su
    # emacs

If the user hasn't yet used Emacs (and depending on the details of which
version of `su` you use) this can create
a /home/<user>/.emacs.d/eln-cache that's owned by root because we create
that dir according to `~$USER` rather than according to `$HOME`.

>> > Maybe this is the best we can do for emacs-28 so close to the release,
>> > but for master wouldn't it be better to use delayed-warning instead?
>> 
>> Sounds about right, tho I suspect we might need to do even a bit more on
>> `master`, like disabling lazy native compilation when we bump into this
>> problem, to avoid repeating that same error message, or trying harder to
>> find a writable directory.
>
> I'm more bothered about letting the user know than about working
> around the problem.

Good.


        Stefan

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here is the cleaner `emacs-28` version.

Pushed,


        Stefan

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57562@debbugs.gnu.org
> Date: Sat, 03 Sep 2022 15:16:23 -0400
> 
> > I guess that's enough for Emacs 28.  (And I do wonder how come people
> > come up with unwritable home directories.)
> 
> In the discussion of that Debian bug#1017739, Russ Allbery mentions
> a(n unrelated) problem which can lead to this:
> 
>     % su
>     # emacs
> 
> If the user hasn't yet used Emacs (and depending on the details of which
> version of `su` you use) this can create
> a /home/<user>/.emacs.d/eln-cache that's owned by root because we create
> that dir according to `~$USER` rather than according to `$HOME`.

I didn't mean to say that I didn't understand how this could happen
_technically_.  What I don't get is how come people let this happen,
and don't pay attention until Emacs complains?  Isn't it crazy to have
your home directory unwritable by your user??

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii [2022-09-03 22:28:20] wrote:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: 57562@debbugs.gnu.org
>> Date: Sat, 03 Sep 2022 15:16:23 -0400
>> 
>> > I guess that's enough for Emacs 28.  (And I do wonder how come people
>> > come up with unwritable home directories.)
>> 
>> In the discussion of that Debian bug#1017739, Russ Allbery mentions
>> a(n unrelated) problem which can lead to this:
>> 
>>     % su
>>     # emacs
>> 
>> If the user hasn't yet used Emacs (and depending on the details of which
>> version of `su` you use) this can create
>> a /home/<user>/.emacs.d/eln-cache that's owned by root because we create
>> that dir according to `~$USER` rather than according to `$HOME`.
>
> I didn't mean to say that I didn't understand how this could happen
> _technically_.  What I don't get is how come people let this happen,
> and don't pay attention until Emacs complains?  Isn't it crazy to have
> your home directory unwritable by your user??

I think in the above scenario, if you just upgraded to Emacs-28 and it's
hence the first time you run an Emacs with native compilation, you get
a home directory that's still perfectly writable, including `~/.emacs.d`
and it's only `~/.emacs.d/eln-cache` that's owned by root.

And since Emacs error(s|ed) out right away, the error message didn't let
you find the name of the directories that were searched to figure out
the origin of the problem.


        Stefan

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57562@debbugs.gnu.org
> Date: Sat, 03 Sep 2022 22:02:07 -0400
> 
> > I didn't mean to say that I didn't understand how this could happen
> > _technically_.  What I don't get is how come people let this happen,
> > and don't pay attention until Emacs complains?  Isn't it crazy to have
> > your home directory unwritable by your user??
> 
> I think in the above scenario, if you just upgraded to Emacs-28 and it's
> hence the first time you run an Emacs with native compilation, you get
> a home directory that's still perfectly writable, including `~/.emacs.d`
> and it's only `~/.emacs.d/eln-cache` that's owned by root.

Isn't the home directory created in a way that all its subdirectories
inherit the access rights by default?

Anyway, what you describe is a bug in the distro, and should ideally
be fixed there.

bug#57562: [PATCH] * lisp/emacs-lisp/comp.el (comp-run-async-workers): Fail more gracefully

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > I didn't mean to say that I didn't understand how this could happen
>> > _technically_.  What I don't get is how come people let this happen,
>> > and don't pay attention until Emacs complains?  Isn't it crazy to have
>> > your home directory unwritable by your user??
>> 
>> I think in the above scenario, if you just upgraded to Emacs-28 and it's
>> hence the first time you run an Emacs with native compilation, you get
>> a home directory that's still perfectly writable, including `~/.emacs.d`
>> and it's only `~/.emacs.d/eln-cache` that's owned by root.
>
> Isn't the home directory created in a way that all its subdirectories
> inherit the access rights by default?

No, and the problem is ownership for which there's nothing equivalent
the `setgid` bit on directories.

Here's the scenario again:

- User FOO has been using his home directory for a while under Debian,
  using Emacs-27.
- /home/FOO/.emacs.d is owned by user A.
- /home/FOO/.emacs.d/eln-cache does not exist.
- User FOO does

      % su
      # apt upgrade
      ... this installs Emacs-28, yay! ...
      # emacs ...babla...

- when Emacs is started, HOME=/root and USER=FOO
- Since the new flashy Emacs-28 was built with native-compilation it
  tries to compile some files.
- Decides to store the files in `~$USER/.emacs.d/eln-cache`, hence in
  /home/FOO/.emacs.d/eln-cache.
- Hence it creates /home/FOO/.emacs.d/eln-cache, but we're running as
  root, so this dir will be owned by root :-(

> Anyway, what you describe is a bug in the distro, and should ideally
> be fixed there.

We can point fingers, but the result is still the same.

Personally, I think part of the problem is that we use ~$USER/ rather
than $HOME (but other people consider it to be a feature rather than a bug).
Other people will claim that the problem is the use of `su` rather than
`su -`.

I'm not sure we can really solve all those problems.  Maybe we should
try and refrain from creating dirs and files in `~/.emacs.d` when we
don't have the same uid, but it seems difficult to do it in a way that
doesn't introduce worse problems than it's solving.


        Stefan