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

Previous Next

Full log

Message #26 received at 63625 <at> debbugs.gnu.org (full text, mbox):

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

>>>>> On Mon, 22 May 2023 09:58:34 -0400, Stefan Monnier <monnier <at> iro.umontreal.ca> said:

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

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

OK

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

Yes, `loaddefs-generate--rubric` needs fixing, but nothing currently
calls that branch as far as I can tell.

`file-name-directory' will return nil for names with no slashes in
them, but I donʼt know if thatʼs possible here.

I *think* we can use `when-let*' here, since subr.el is preloaded.

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

Ah, so thatʼs where the duplication was coming from. Eli, do you want
that on the release branch?

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

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

Weʼre talking about emacs-29. Iʼm not about to risk having
`load-file-nameʼ be nil ';-)

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

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

OK, so, `loaddefs-generate--rubric' is probably worth changing (on master)

And hereʼs another reason to avoid it: I typoed '#$' as '$#' :D

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


Robert
-- 

Previous Next

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