Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Jens Schmidt]

On 2023-12-07  19:14, Stefan Monnier wrote:
> branch: master
> commit 3b1fd42732f7ca5b2db6ad6e75af1c037e1c54e4
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
> 
>     * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

I don't mind the error => warning change, we have been discussing
that in a slightly different context, anyway.

But when I tried to place that advice-checking-code into loadup.el,
I also saw this rmc.el and didn't quite know what to do with it.  I
thought that Lars might have placed it *inadvertently* after the

  (load "site-load" t)

block, because, um, logically it belongs to before that, doesn't it?
First all the core libraries get preloaded (including rmc.el), then
any site-specific libraries.

So probably TRT here would be to place these paragraphs as follows:

0. (load "leim/leim-list.el" t)

1. (load "emacs-lisp/rmc")

2. Check for advices (on core libs only).

3. (load "site-load" t)

4. "default-directory must be unibyte"

What do you think?

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Stefan Monnier]

> But when I tried to place that advice-checking-code into loadup.el,
> I also saw this rmc.el and didn't quite know what to do with it.  I
> thought that Lars might have placed it *inadvertently* after the
>
>   (load "site-load" t)
>
> block, because, um, logically it belongs to before that, doesn't it?

That was also my impression, indeed.

> So probably TRT here would be to place these paragraphs as follows:
>
> 0. (load "leim/leim-list.el" t)
>
> 1. (load "emacs-lisp/rmc")
>
> 2. Check for advices (on core libs only).
>
> 3. (load "site-load" t)
>
> 4. "default-directory must be unibyte"
>
> What do you think?

I decided that it was OK to keep "check for advice" after loading
`site-load` now that it's a warning.  IOW I don't have a strong opinion
on the order between `2` and `3`.
But I agree that `rmc` should come before those two.


        Stefan

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Eli Zaretskii]

> Date: Tue, 12 Dec 2023 22:48:56 +0100
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> So probably TRT here would be to place these paragraphs as follows:
> 
> 0. (load "leim/leim-list.el" t)
> 
> 1. (load "emacs-lisp/rmc")
> 
> 2. Check for advices (on core libs only).
> 
> 3. (load "site-load" t)
> 
> 4. "default-directory must be unibyte"
> 
> What do you think?

I think you are right about rmc, but leim-list should be after rmc,
not before.

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Jens Schmidt]

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

On 2023-12-13  12:46, Eli Zaretskii wrote:
>> Date: Tue, 12 Dec 2023 22:48:56 +0100
>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>>
>> So probably TRT here would be to place these paragraphs as follows:
>>
>> 0. (load "leim/leim-list.el" t)
>>
>> 1. (load "emacs-lisp/rmc")
>>
>> 2. Check for advices (on core libs only).
>>
>> 3. (load "site-load" t)
>>
>> 4. "default-directory must be unibyte"
>>
>> What do you think?
> 
> I think you are right about rmc, but leim-list should be after rmc,
> not before.

Is rmc (technically) required at all in loadup.el?  I tried
bootstrap'ping and check'ing with the attached patch, and these come
through fine.

rmc.el provides autoloaded function `read-multiple-choice', which is
called from `kill-buffer--possibly-save' (defined in simple.el),
which is called from Fkill_buffer (defined in buffer.c), but only
during interactive use.

Of course, killing buffers is everyday business, but autoloading "rmc"
while prompting for user input should be neglectable, shouldn't it?
And during bootstrap everything should be noninteractive, so "rmc"
wouldn't be required at that time, would it?

And do I get a free wish from Eli when I help reducing preloaded libs?

[-- Attachment #2: 0001-Remove-preload-of-rmc-from-loadup.el.patch --]
[-- Type: text/x-patch, Size: 2764 bytes --]

From 9e81cf1c7ec547098ce7ada260b116a5242bde72 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Wed, 13 Dec 2023 22:11:32 +0100
Subject: [PATCH] ; Remove preload of rmc from loadup.el

See discussion on
https://lists.gnu.org/archive/html/emacs-devel/2023-12/msg00309.html.

* lisp/loadup.el ("emacs-lisp/rmc"): Remove preload.
* lisp/files.el (read-multiple-choice):
* lisp/simple.el (read-multiple-choice): Declare function.
* lisp/loadup.el: Make the warning on advised preloaded functions a
tad noisier.
---
 lisp/files.el  | 3 +++
 lisp/loadup.el | 5 +----
 lisp/simple.el | 3 +++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index f87e7807301..4ee7614f99a 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2297,6 +2297,9 @@ files--ask-user-about-large-file-help-text
    op-type
    op-type))
 
+(declare-function read-multiple-choice "emacs-lisp/rmc"
+                  (prompt choices &optional help-string show-help long-form))
+
 (defun files--ask-user-about-large-file (size op-type filename offer-raw)
   "Query the user about what to do with large files.
 Files are \"large\" if file SIZE is larger than `large-file-warning-threshold'.
diff --git a/lisp/loadup.el b/lisp/loadup.el
index 1f959464b23..de1934e9299 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -406,9 +406,6 @@
       (message "Warning: Change in load-path due to site-load will be \
 lost after dumping")))
 
-;; Used by `kill-buffer', for instance.
-(load "emacs-lisp/rmc")
-
 ;; Actively check for advised functions during preload since:
 ;; - advices in Emacs's core are generally considered bad style;
 ;; - `Snarf-documentation' looses docstrings of primitives advised
@@ -419,7 +416,7 @@
         ;; Don't make it an error because it's not serious enough and
         ;; it can be annoying during development.  Also there are still
         ;; circumstances where we use advice on preloaded functions.
-        (message "Warning: Advice installed on preloaded function %S" f))))
+        (message "\nWarning: Advice installed on preloaded function %S" f))))
 
 ;; Make sure default-directory is unibyte when dumping.  This is
 ;; because we cannot decode and encode it correctly (since the locale
diff --git a/lisp/simple.el b/lisp/simple.el
index cee1ddac52f..71cedd6cb17 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -11093,6 +11093,9 @@ scratch-buffer
   (interactive)
   (pop-to-buffer-same-window (get-scratch-buffer-create)))
 
+(declare-function read-multiple-choice "emacs-lisp/rmc"
+                  (prompt choices &optional help-string show-help long-form))
+
 (defun kill-buffer--possibly-save (buffer)
   "Ask the user to confirm killing of a modified BUFFER.
 
-- 
2.30.2

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Eli Zaretskii]

> Date: Thu, 14 Dec 2023 00:03:16 +0100
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> Is rmc (technically) required at all in loadup.el?  I tried
> bootstrap'ping and check'ing with the attached patch, and these come
> through fine.

Preloaded files are not preloaded only because they are used during
bootstrap.  They are preloaded because other preloaded files call
their functions in situations that we consider frequent enough to
justify preloading.

> rmc.el provides autoloaded function `read-multiple-choice', which is
> called from `kill-buffer--possibly-save' (defined in simple.el),
> which is called from Fkill_buffer (defined in buffer.c), but only
> during interactive use.
> 
> Of course, killing buffers is everyday business, but autoloading "rmc"
> while prompting for user input should be neglectable, shouldn't it?
> And during bootstrap everything should be noninteractive, so "rmc"
> wouldn't be required at that time, would it?

That might have been a valid question back when we decided to preload
rmc, but it's too late to raise it today: I'm quite sure there are
enough Lisp programs out there that depend on it being preloaded.  And
even besides this, kill-buffer is basic enough to have all of its code
preloaded.

So I don't think we should remove it from loadup.el at this time.

> And do I get a free wish from Eli when I help reducing preloaded libs?

Only when you do that in time ;-)

Thanks.

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Jens Schmidt]

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

On 2023-12-14  08:23, Eli Zaretskii wrote:

> So I don't think we should remove it from loadup.el at this time.

Agreed, and thanks for the background.

I understood from your previous email [1] that the current location of
where "rmc" is loaded in loadup.el is not optimal.

How about moving it to the block that also loads "minibuffer"?  Attached
patch does that and bootstraps and checks without error and warnings.

[1] https://lists.gnu.org/archive/html/emacs-devel/2023-12/msg00320.html


[-- Attachment #2: 0001-Move-preload-of-rmc-to-a-more-appropriate-place.patch --]
[-- Type: text/x-patch, Size: 1225 bytes --]

From 2e8a6c78c140390b2538b89906f11986815e7f53 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 14 Dec 2023 20:47:22 +0100
Subject: [PATCH] ; Move preload of rmc to a more appropriate place

See discussion on
https://lists.gnu.org/archive/html/emacs-devel/2023-12/msg00309.html.

* lisp/loadup.el ("emacs-lisp/rmc"): Move preload to a more
appropriate place.
---
 lisp/loadup.el | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lisp/loadup.el b/lisp/loadup.el
index 1f959464b23..1692b6ee7f3 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -256,6 +256,7 @@
 (load "emacs-lisp/syntax")
 (load "font-lock")
 (load "jit-lock")
+(load "emacs-lisp/rmc") ; Used by `kill-buffer', for instance.
 
 (load "mouse")
 ;; This loading happens on Android despite scroll bars being
@@ -406,9 +407,6 @@
       (message "Warning: Change in load-path due to site-load will be \
 lost after dumping")))
 
-;; Used by `kill-buffer', for instance.
-(load "emacs-lisp/rmc")
-
 ;; Actively check for advised functions during preload since:
 ;; - advices in Emacs's core are generally considered bad style;
 ;; - `Snarf-documentation' looses docstrings of primitives advised
-- 
2.30.2

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Eli Zaretskii]

> Date: Thu, 14 Dec 2023 21:03:02 +0100
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> I understood from your previous email [1] that the current location of
> where "rmc" is loaded in loadup.el is not optimal.

Not "not optimal", incorrect.  It should be before leim-list.el.

> How about moving it to the block that also loads "minibuffer"?

Why risk issues that could cause?  I'd leave it the last loaded
package.  We don't have any real reason to move it higher up, do we?

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Jens Schmidt]

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

On 2023-12-14  21:27, Eli Zaretskii wrote:
>> Date: Thu, 14 Dec 2023 21:03:02 +0100
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>>
>> I understood from your previous email [1] that the current location of
>> where "rmc" is loaded in loadup.el is not optimal.
> 
> Not "not optimal", incorrect.  It should be before leim-list.el.
> 
>> How about moving it to the block that also loads "minibuffer"?
> 
> Why risk issues that could cause?  I'd leave it the last loaded
> package.  We don't have any real reason to move it higher up, do we?

I thought it'd feel lonely there.  On a more serious note, there are
more files that seem to race for the last places ("cus-start", for
example), and I thought it would be helpful to have one candidate
less wanting for a special position.

Anyway, if it should be before leim-list.el, attached is it.


[-- Attachment #2: 0001-Move-preload-of-rmc-to-a-more-appropriate-place.patch --]
[-- Type: text/x-patch, Size: 1345 bytes --]

From 29973cba6200c1bb7659a2e02dc5aaa5195c654a Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 14 Dec 2023 20:47:22 +0100
Subject: [PATCH] ; Move preload of rmc to a more appropriate place

See discussion on
https://lists.gnu.org/archive/html/emacs-devel/2023-12/msg00309.html.

* lisp/loadup.el ("emacs-lisp/rmc"): Move preload to a more
appropriate place.
---
 lisp/loadup.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/loadup.el b/lisp/loadup.el
index 1f959464b23..d2a4c6a7cf7 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -387,6 +387,9 @@
     (load "tooltip"))
 (load "international/iso-transl") ; Binds Alt-[ and friends.
 
+;; Used by `kill-buffer', for instance.
+(load "emacs-lisp/rmc")
+
 ;; This file doesn't exist when building a development version of Emacs
 ;; from the repository.  It is generated just after temacs is built.
 (load "leim/leim-list.el" t)
@@ -406,9 +409,6 @@
       (message "Warning: Change in load-path due to site-load will be \
 lost after dumping")))
 
-;; Used by `kill-buffer', for instance.
-(load "emacs-lisp/rmc")
-
 ;; Actively check for advised functions during preload since:
 ;; - advices in Emacs's core are generally considered bad style;
 ;; - `Snarf-documentation' looses docstrings of primitives advised
-- 
2.30.2

Re: master 3b1fd42732f: * lisp/loadup.el: Check advice after `rmc.el`; turn error into warning

[Eli Zaretskii]

> Date: Thu, 14 Dec 2023 22:21:24 +0100
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> On 2023-12-14  21:27, Eli Zaretskii wrote:
> >> Date: Thu, 14 Dec 2023 21:03:02 +0100
> >> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> >> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> >>
> >> I understood from your previous email [1] that the current location of
> >> where "rmc" is loaded in loadup.el is not optimal.
> > 
> > Not "not optimal", incorrect.  It should be before leim-list.el.
> > 
> >> How about moving it to the block that also loads "minibuffer"?
> > 
> > Why risk issues that could cause?  I'd leave it the last loaded
> > package.  We don't have any real reason to move it higher up, do we?
> 
> I thought it'd feel lonely there.  On a more serious note, there are
> more files that seem to race for the last places ("cus-start", for
> example), and I thought it would be helpful to have one candidate
> less wanting for a special position.
> 
> Anyway, if it should be before leim-list.el, attached is it.

Thanks, installed on the master branch.