unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#56614: two problems with `package--reload-previously-loaded'
@ 2022-07-17 12:42 Paul Pogonyshev
  2022-07-23  8:30 ` Lars Ingebrigtsen
       [not found] ` <jwv5ye8xxit.fsf-monnier+emacs@gnu.org>
  0 siblings, 2 replies; 6+ messages in thread
From: Paul Pogonyshev @ 2022-07-17 12:42 UTC (permalink / raw)
  To: 56614

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#56614: two problems with `package--reload-previously-loaded'
  2022-07-17 12:42 bug#56614: two problems with `package--reload-previously-loaded' Paul Pogonyshev
@ 2022-07-23  8:30 ` Lars Ingebrigtsen
  2022-07-24  9:02   ` Paul Pogonyshev
       [not found] ` <jwv5ye8xxit.fsf-monnier+emacs@gnu.org>
  1 sibling, 1 reply; 6+ messages in thread
From: Lars Ingebrigtsen @ 2022-07-23  8:30 UTC (permalink / raw)
  To: Paul Pogonyshev; +Cc: 56614

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).)






^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#56614: two problems with `package--reload-previously-loaded'
  2022-07-23  8:30 ` Lars Ingebrigtsen
@ 2022-07-24  9:02   ` Paul Pogonyshev
  2022-07-24  9:06     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Pogonyshev @ 2022-07-24  9:02 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 56614


[-- 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* bug#56614: two problems with `package--reload-previously-loaded'
  2022-07-24  9:02   ` Paul Pogonyshev
@ 2022-07-24  9:06     ` Lars Ingebrigtsen
  0 siblings, 0 replies; 6+ messages in thread
From: Lars Ingebrigtsen @ 2022-07-24  9:06 UTC (permalink / raw)
  To: Paul Pogonyshev; +Cc: 56614

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).







^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#56614: two problems with `package--reload-previously-loaded'
       [not found]   ` <CAG7Bparvk4=u992jdmgNqnsjYajDnjupDhhsHRksW38SERcuLA@mail.gmail.com>
@ 2022-12-18 23:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-12-19  9:07       ` Paul Pogonyshev
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-12-18 23:48 UTC (permalink / raw)
  To: Paul Pogonyshev; +Cc: 56614

> 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






^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#56614: two problems with `package--reload-previously-loaded'
  2022-12-18 23:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-12-19  9:07       ` Paul Pogonyshev
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Pogonyshev @ 2022-12-19  9:07 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 56614

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-12-19  9:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-17 12:42 bug#56614: two problems with `package--reload-previously-loaded' Paul Pogonyshev
2022-07-23  8:30 ` Lars Ingebrigtsen
2022-07-24  9:02   ` Paul Pogonyshev
2022-07-24  9:06     ` Lars Ingebrigtsen
     [not found] ` <jwv5ye8xxit.fsf-monnier+emacs@gnu.org>
     [not found]   ` <CAG7Bparvk4=u992jdmgNqnsjYajDnjupDhhsHRksW38SERcuLA@mail.gmail.com>
2022-12-18 23:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-12-19  9:07       ` Paul Pogonyshev

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).