bug#56614: two problems with `package--reload-previously-loaded'

bug#56614: two problems with `package--reload-previously-loaded'

[Paul Pogonyshev]

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

Commit 9dfd945a2c added function `package--reload-previously-loaded'.
Previously package library would just always reload all package files
(except its autoloads), new function tries to be smart and avoid reloading
unneeded things, presumably for performance reasons.

However, this has created at least two problems for Eldev (
https://github.com/doublep/eldev), Elisp development tool.

1. Function excludes package's current directory, see variable
`load-path-sans-dir'. This is problematic if a package is reinstalled from
the same directory after changes have been made. I know this is not how
things usually work, but it seems to create problems for no reason: if the
same directory is never used, why bother about it here? Normal usecases are
not affected at all, why "special" usecases that reuse the same directory
suffer.

2. When searching in `load-history', the function ignores changes from
byte-compiled to source and vice versa, i.e. `.el' -> `.elc' and `.elc' ->
`.el'. In other words, if you replace a byte-compiled file with a
non-compiled (or vice versa), it never gets reloaded. A simple fix (having
zero effect in normal usecases) would be e.g. this:

        (truename (file-truename canonical))
        (found (or (member truename history)
                   (member (if (string-suffix-p ".el" truename)
                               (replace-regexp-in-string (rx ".el" eos)
".elc" truename t)
                             (replace-regexp-in-string (rx ".elc" eos)
".el" truename t))
                           history)))

Paul

[-- Attachment #2: Type: text/html, Size: 1921 bytes --]

bug#56614: two problems with `package--reload-previously-loaded'

[Lars Ingebrigtsen]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> 1. Function excludes package's current directory, see variable
> `load-path-sans-dir'.  This is problematic if a package is reinstalled
> from the same directory after changes have been made. I know this is
> not how things usually work, but it seems to create problems for no
> reason: if the same directory is never used, why bother about it here?
> Normal usecases are not affected at all, why "special" usecases that
> reuse the same directory suffer.

Yes, that seems odd.

> 2. When searching in `load-history', the function ignores changes from byte-compiled
> to source and vice versa, i.e. `.el' -> `.elc' and `.elc' -> `.el'. In other words, if you
> replace a byte-compiled file with a non-compiled (or vice versa), it never gets
> reloaded. A simple fix (having zero effect in normal usecases) would be e.g. this:
>
>         (truename (file-truename canonical))
>         (found (or (member truename history)
>                    (member (if (string-suffix-p ".el" truename)
>                                (replace-regexp-in-string (rx ".el" eos) ".elc" truename t)
>                              (replace-regexp-in-string (rx ".elc" eos) ".el" truename t))
>                            history)))
>

I think that makes sense.  Could you prepare a patch for both these
issues?  (It's safer if you do it so that you can verify that things
work fine after the change (since you already have a test case).)

bug#56614: two problems with `package--reload-previously-loaded'

[Paul Pogonyshev]

[-- Attachment #1.1: Type: text/plain, Size: 2044 bytes --]

> Could you prepare a patch for both these
> issues?  (It's safer if you do it so that you can verify that things
> work fine after the change (since you already have a test case).)

Please find attached. I added comments for both changes that explain the
reasoning, so that they are not accidentally reverted by a later change.

Also replaced obsolete `find-function-source-path' with its new name
`find-library-source-path'.

Paul

On Sat, 23 Jul 2022 at 10:30, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Paul Pogonyshev <pogonyshev@gmail.com> writes:
>
> > 1. Function excludes package's current directory, see variable
> > `load-path-sans-dir'.  This is problematic if a package is reinstalled
> > from the same directory after changes have been made. I know this is
> > not how things usually work, but it seems to create problems for no
> > reason: if the same directory is never used, why bother about it here?
> > Normal usecases are not affected at all, why "special" usecases that
> > reuse the same directory suffer.
>
> Yes, that seems odd.
>
> > 2. When searching in `load-history', the function ignores changes from
> byte-compiled
> > to source and vice versa, i.e. `.el' -> `.elc' and `.elc' -> `.el'. In
> other words, if you
> > replace a byte-compiled file with a non-compiled (or vice versa), it
> never gets
> > reloaded. A simple fix (having zero effect in normal usecases) would be
> e.g. this:
> >
> >         (truename (file-truename canonical))
> >         (found (or (member truename history)
> >                    (member (if (string-suffix-p ".el" truename)
> >                                (replace-regexp-in-string (rx ".el" eos)
> ".elc" truename t)
> >                              (replace-regexp-in-string (rx ".elc" eos)
> ".el" truename t))
> >                            history)))
> >
>
> I think that makes sense.  Could you prepare a patch for both these
> issues?  (It's safer if you do it so that you can verify that things
> work fine after the change (since you already have a test case).)
>
>

[-- Attachment #1.2: Type: text/html, Size: 2924 bytes --]

[-- Attachment #2: 0001-Tweak-package-reload-previously-loaded-bug-56614.patch --]
[-- Type: text/x-patch, Size: 2919 bytes --]

From ba0fec69f810f924cb10231694fd741bd3915103 Mon Sep 17 00:00:00 2001
From: Paul Pogonyshev <pogonyshev@gmail.com>
Date: Sun, 24 Jul 2022 11:00:46 +0200
Subject: [PATCH] Tweak `package--reload-previously-loaded' (bug#56614)

---
 lisp/emacs-lisp/package.el | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 00beee811b..6b55726546 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -785,10 +785,14 @@ package--reload-previously-loaded
   (with-demoted-errors "Error in package--load-files-for-activation: %s"
     (let* (result
            (dir (package-desc-dir pkg-desc))
-           (load-path-sans-dir
-            (cl-remove-if (apply-partially #'string= dir)
-                          (or (bound-and-true-p find-function-source-path)
-                              load-path)))
+           ;; A previous implementation would skip `dir' itself.
+           ;; However, in normal use reloading from the same directory
+           ;; never happens anyway, while in certain cases external to
+           ;; Emacs a package in the same directory not necessary
+           ;; stays byte-identical, e.g.  during development.  Just
+           ;; don't special-case `dir'.
+           (effective-path (or (bound-and-true-p find-library-source-path)
+                               load-path))
            (files (directory-files-recursively dir "\\`[^\\.].*\\.el\\'"))
            (history (mapcar #'file-truename
                             (cl-remove-if-not #'stringp
@@ -796,8 +800,17 @@ package--reload-previously-loaded
       (dolist (file files)
         (when-let ((library (package--library-stem
                              (file-relative-name file dir)))
-                   (canonical (locate-library library nil load-path-sans-dir))
-                   (found (member (file-truename canonical) history))
+                   (canonical (locate-library library nil effective-path))
+                   (truename (file-truename canonical))
+                   ;; Normally, all files in a package are compiled by
+                   ;; now, but don't assume that.  E.g. different
+                   ;; versions can add or remove `no-byte-compile'.
+                   (altname (if (string-suffix-p ".el" truename)
+                                (replace-regexp-in-string "\\.el\\'" ".elc" truename t)
+                              (replace-regexp-in-string "\\.elc\\'" ".el" truename t)))
+                   (found (or (member truename history)
+                              (and (not (string= altname truename))
+                                   (member altname history))))
                    (recent-index (length found)))
           (unless (equal (file-name-base library)
                          (format "%s-autoloads" (package-desc-name pkg-desc)))
-- 
2.35.1

bug#56614: two problems with `package--reload-previously-loaded'

[Lars Ingebrigtsen]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> Please find attached. I added comments for both changes that explain the reasoning,
> so that they are not accidentally reverted by a later change.
>
> Also replaced obsolete `find-function-source-path' with its new name
> `find-library-source-path'.

Thanks; pushed to Emacs 29 (with some whitespace changes).

bug#56614: two problems with `package--reload-previously-loaded'

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

> Add e.g. `(message "RELOAD %S" (car c))' before the `(load ...)' form in
> the function, reevaluate it and reinstall any package, e.g.:

When you re-install, the new files's names will be the same as the
previously loaded ones, so it's the corner case where it happens to work.

But if you start Emacs, load its builtin Org, and then install GNU
ELPA's Org, I doesn't seem to reload anything.


        Stefan

bug#56614: two problems with `package--reload-previously-loaded'

[Paul Pogonyshev]

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

Have you tried?

Save this as `package-reload.el' and run `emacs --batch -l
package-reload.el':

(defadvice load (before debug)
  (message "(RE)LOADING %s" file))
(defadvice package--reload-previously-loaded (around debug activate)
  (ad-activate 'load)
  ad-do-it
  (ad-deactivate 'load))
;(require 'org)
(let ((temp (make-temp-file "packages" t)))
  (setf package-user-dir temp))
(require 'package)
(package-initialize)
(package-refresh-contents)
(package-install (cadr (assq 'org package-archive-contents)))

I get messages like "(RE)LOADING /tmp/packagesDLaBkh/org-9.6/org-macs".
Doesn't matter if I uncomment that line with "(require 'org)" or not.

Paul



On Mon, 19 Dec 2022 at 00:48, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Add e.g. `(message "RELOAD %S" (car c))' before the `(load ...)' form in
> > the function, reevaluate it and reinstall any package, e.g.:
>
> When you re-install, the new files's names will be the same as the
> previously loaded ones, so it's the corner case where it happens to work.
>
> But if you start Emacs, load its builtin Org, and then install GNU
> ELPA's Org, I doesn't seem to reload anything.
>
>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 1777 bytes --]