GNU bug report logs - #63625
29.0.90; package-install inserts package directory into load-path twice.

Previous Next

Full log

View this message in rfc822 format

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Philip Kaludercic <philipk <at> posteo.net>, 63625 <at> debbugs.gnu.org, todd smith <toddasmith <at> mac.com>
Subject: bug#63625: 29.0.90; package-install inserts package directory into load-path  twice.
Date: Mon, 22 May 2023 09:58:34 -0400

> This is because we didnʼt respect DRY. package.el should use the
> package support of `loaddefs-generate', but that doesnʼt expose the
> requisite feature of `loaddefs-generate--rubric' (maybe on master it does).
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 78017b77677..31e5e0809a8 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1107,8 +1107,9 @@ package-generate-autoloads
>          ;; Add the directory that will contain the autoload file to
>          ;; the load path.  We don't hard-code `pkg-dir', to avoid
>          ;; issues if the package directory is moved around.
> +        (directory-file-name
>          (or (and load-file-name (file-name-directory load-file-name))
> -            (car load-path)))))
> +            (car load-path))))))

The (car load-path) is intended to return an element
that causes `add-to-list` to do nothing, but your patch makes it go
through `directory-file-name` which risks changing the string and thus
causing the kind of duplicate we're trying to avoid.

IOW, the `directory-file-name` should be directly around
`file-name-directory` instead (tho I'm not 100% sure
`file-name-directory` can never return nil here, so it might require an
additional let binding and check).
Admittedly, this exact problem was present before Philip's change
(it was introduced by commit 4d3a595d8d3e in 2015) and is also present in
`loaddefs-generate--rubric`.

In any case, some autoloads file use a trailing / and others don't,
depending on which version of Emacs has been used to generate it, so we
need the patch below, I think (which also reverts to adding just `pkg-dir`
since that's what we used to do in Emacs-28 and this is old
compatibility code anyway).

> 1. Can `load-file-name' ever be nil here?

It's always a possibility (e.g. if you open the autoloads file and do
`eval-buffer`), tho some older versions of Emacs didn't bother to check
for it.  Not sure it's important.

> 2. Should we just use $# instead of `load-file-nameʼ'?

We used to use $# but that interacts poorly with compilation.
Until recently we never compiled autoloads files, but it's becoming much
more common.


        Stefan


diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 78017b77677..236a8e974e7 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -902,7 +902,12 @@ package-activate-1
           (package--reload-previously-loaded pkg-desc))
         (with-demoted-errors "Error loading autoloads: %s"
           (load (package--autoloads-file-name pkg-desc) nil t))
-        (add-to-list 'load-path (directory-file-name pkg-dir)))
+        ;; FIXME: Since 2013 (commit 4fac34cee97a), the autoload files take
+        ;; care of changing the `load-path', so maybe it's time to
+        ;; remove this fallback code?
+        (unless (or (member (file-name-as-directory pkg-dir) load-path)
+                    (member (directory-file-name pkg-dir) load-path))
+          (add-to-list 'load-path pkg-dir)))
       ;; Add info node.
       (when (file-exists-p (expand-file-name "dir" pkg-dir))
         ;; FIXME: not the friendliest, but simple.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.