bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

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

1) trash ~/.emacs.d     ;; i.e. remove .emacs.d (optional)

2) emacs -Q   (the -Q isn’t necessary,  I’ve observed the problem every way that I’ve started emacs)

3) in *scratch* buffer, type and execute "(package-insert ‘ack)”       (or run M-x package-install or install from M-x list-packages.
Any and every package I’ve installed produces the problem, ack is just easy to type)

4) C-h v  load-path    (that is, run describe-variable on load-path to see its value, note the first two entries)

Its value is
("/Users/todd/.emacs.d/elpa/ack-1.11" "/Users/todd/.emacs.d/elpa/ack-1.11/" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/vc" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/use-package" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/url" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/textmodes" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/progmodes" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/play" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/org" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/nxml" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/net" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/mh-e" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/mail" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/leim" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/language" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/international" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/image" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/gnus" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/eshell" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/erc" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/emulation" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/emacs-lisp" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/cedet" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/calendar" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/calc" "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/obsolete")

5) Unlike Emacs 28, two entries for package ack have been
added to load-path (see the first two entries). These are both for the
same directory (one has a trailing "/" the other doesn’t, but both strings are equivalent paths to the directory for Emacs).

6) It doesn't matter how Emacs is started with differing command line
args. It doesn't matter if package-install is invoked with M-x in the
mini-buffer or (package-install ...) executed in the *scratch* buffer or
(use-package ... :ensure t) in an init.el file. It doesn't matter which
package is being installed. It doesn't matter how many packages are
installed. In all cases duplicate directory names are
added as entries to load-path for *every* package installed in
Emacs 29.0.90.

7) Thank You!



In GNU Emacs 29.0.90 (build 2, aarch64-apple-darwin22.4.0, NS
appkit-2299.50 Version 13.3.1 (Build 22E261)) of 2023-04-26 built on
firefly.local
System Description:  macOS 13.3.1

Configured using:
'configure --disable-dependency-tracking --disable-silent-rules
--enable-locallisppath=/opt/homebrew/share/emacs/site-lisp
--infodir=/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/info/emacs
--prefix=/opt/homebrew/Cellar/emacs-plus@29/29.0.60 --with-xml2
--with-gnutls --with-native-compilation --without-compress-install
--without-dbus --without-imagemagick --with-modules --with-rsvg
--without-pop --with-ns --disable-ns-self-contained 'CFLAGS=-Os -w
-pipe -mmacosx-version-min=13
-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk
-DFD_SETSIZE=10000 -DDARWIN_UNLIMITED_SELECT'
'CPPFLAGS=-I/opt/homebrew/opt/zlib/include
-I/opt/homebrew/opt/jpeg/include -I/opt/homebrew/opt/readline/include
-I/opt/homebrew/opt/icu4c/include
-I/opt/homebrew/opt/openssl@1.1/include -isystem/opt/homebrew/include
-F/opt/homebrew/Frameworks
-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk'
'LDFLAGS=-L/opt/homebrew/opt/zlib/lib -L/opt/homebrew/opt/jpeg/lib
-L/opt/homebrew/opt/readline/lib -L/opt/homebrew/opt/icu4c/lib
-L/opt/homebrew/opt/openssl@1.1/lib -L/opt/homebrew/lib
-F/opt/homebrew/Frameworks -Wl,-headerpad_max_install_names
-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk''

Configured features:
ACL GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug cl-print help-fns pcomplete thingatpt
pcase compile comint ansi-osc ansi-color ring ack-autoloads loaddefs-gen
lisp-mnt radix-tree tar-mode arc-mode archive-mode cus-edit pp cus-start
cus-load wid-edit mm-archive message sendmail yank-media dired
dired-loaddefs rfc822 mml mml-sec epa derived gnus-util
text-property-search time-date mailabbrev gmm-utils mailheader mm-decode
mm-bodies mm-encode mail-utils gnutls network-stream url-cache url-http
url-auth mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr url-gw nsm puny epg rfc6068 epg-config finder-inf package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source eieio eieio-core password-cache json map url-vars
comp comp-cstr warnings icons subr-x rx cl-seq cl-macs cl-extra
help-mode cl-loaddefs cl-lib term/xterm xterm byte-opt gv bytecomp
byte-compile rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads kqueue cocoa ns lcms2
multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 209790 38117)
(symbols 48 12653 0)
(strings 32 60200 3226)
(string-bytes 1 1802843)
(vectors 16 34731)
(vector-slots 8 536516 26744)
(floats 8 74 494)
(intervals 56 424 0)
(buffers 984 15))
B

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

>>>>> On Sat, 20 May 2023 16:45:34 -0500, todd smith via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    Todd> 1) trash ~/.emacs.d     ;; i.e. remove .emacs.d (optional)

    Todd> 2) emacs -Q   (the -Q isn’t necessary,  I’ve observed the problem every way that I’ve started emacs)

    Todd> 3) in *scratch* buffer, type and execute "(package-insert ‘ack)”       (or run M-x package-install or install from M-x list-packages.
    Todd> Any and every package I’ve installed produces the problem, ack is just easy to type)

    Todd> 4) C-h v  load-path    (that is, run describe-variable on load-path to see its value, note the first two entries)

    Todd> Its value is
    Todd> ("/Users/todd/.emacs.d/elpa/ack-1.11"
    Todd> "/Users/todd/.emacs.d/elpa/ack-1.11/"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/vc"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/use-package"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/url"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/textmodes"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/progmodes"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/play"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/org"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/nxml"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/net"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/mh-e"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/mail"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/leim"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/language"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/international"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/image"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/gnus"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/eshell"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/erc"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/emulation"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/emacs-lisp"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/cedet"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/calendar"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/calc"
    Todd> "/opt/homebrew/Cellar/emacs-plus@29/29.0.60/share/emacs/29.0.90/lisp/obsolete")

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))))))
     (let ((buf (find-buffer-visiting output-file)))
       (when buf (kill-buffer buf)))
     auto-name))


Robert
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Eli Zaretskii]

> Cc: todd smith <toddasmith@mac.com>
> From: Robert Pluim <rpluim@gmail.com>
> Date: Mon, 22 May 2023 10:55:13 +0200
> 
> 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))))))
>      (let ((buf (find-buffer-visiting output-file)))
>        (when buf (kill-buffer buf)))
>      auto-name))

Thanks.

Philip, Stefan: any comments or suggestions?

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

>>>>> On Mon, 22 May 2023 14:25:02 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> Cc: todd smith <toddasmith@mac.com>
    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Mon, 22 May 2023 10:55:13 +0200
    >> 
    >> 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))))))
    >> (let ((buf (find-buffer-visiting output-file)))
    >> (when buf (kill-buffer buf)))
    >> auto-name))

    Eli> Thanks.

    Eli> Philip, Stefan: any comments or suggestions?

Two other things:

1. Can `load-file-name' ever be nil here?
2. Should we just use $# instead of `load-file-nameʼ'?

(I also have a sneaking suspicion that this adding to `load-path' is
being done twice, but Iʼll look at that when this issue is fixed)

Robert
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: todd smith <toddasmith@mac.com>
>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Mon, 22 May 2023 10:55:13 +0200
>> 
>> 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))))))
>>      (let ((buf (find-buffer-visiting output-file)))
>>        (when buf (kill-buffer buf)))
>>      auto-name))
>
> Thanks.
>
> Philip, Stefan: any comments or suggestions?

This looks like the adequate change to me.

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

>>>>> On Mon, 22 May 2023 13:18:17 +0000, Philip Kaludercic <philipk@posteo.net> said:

    >> 
    >> Thanks.
    >> 
    >> Philip, Stefan: any comments or suggestions?

    Philip> This looks like the adequate change to me.

I was hoping for 'correct' or 'minimal', but Iʼll take 'adequate' 😄

Robert
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

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

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

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

>>>>> On Mon, 22 May 2023 09:58:34 -0400, Stefan Monnier <monnier@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
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

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

> `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 `load-file-name` is always absolute, so I think we should be
safe, but that's the question, yes.

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

Please don't, it's too recently introduced IMO and such generated files
may be loaded in older Emacsen.

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

Maybe we can have that patch in emacs-29 and remove the code altogether
on `master`?


        Stefan

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

>>>>> On Mon, 22 May 2023 11:04:46 -0400, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

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

    Stefan> I think `load-file-name` is always absolute, so I think we should be
    Stefan> safe, but that's the question, yes.

This is where we quote Knuth on the difference between proof and
testing :-)

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

    Stefan> Please don't, it's too recently introduced IMO and such generated files
    Stefan> may be loaded in older Emacsen.

OK

    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> Maybe we can have that patch in emacs-29 and remove the code altogether
    Stefan> on `master`?

That seems reasonable. Eli?

Robert
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>, 63625@debbugs.gnu.org,
>  todd smith <toddasmith@mac.com>
> From: Robert Pluim <rpluim@gmail.com>
> Date: Mon, 22 May 2023 16:19:49 +0200
> 
>     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?

The issue at hand must be fixed on the emacs-29 branch, but I'm not
sure I understand what you mean by "that" here.

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>, 63625@debbugs.gnu.org,
>  todd smith <toddasmith@mac.com>
> From: Robert Pluim <rpluim@gmail.com>
> Date: Mon, 22 May 2023 17:36:09 +0200
> 
>     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> Maybe we can have that patch in emacs-29 and remove the code altogether
>     Stefan> on `master`?
> 
> That seems reasonable. Eli?

I'm a bit confused by "that patch" and stuff, and would prefer to see
the patch for emacs-29 and another for master, please.

Thanks.

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

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

>>>>> On Mon, 22 May 2023 18:53:47 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> Cc: Philip Kaludercic <philipk@posteo.net>, 63625@debbugs.gnu.org,
    >> todd smith <toddasmith@mac.com>
    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Mon, 22 May 2023 17:36:09 +0200
    >> 
    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> Maybe we can have that patch in emacs-29 and remove the code altogether
    Stefan> on `master`?
    >> 
    >> That seems reasonable. Eli?

    Eli> I'm a bit confused by "that patch" and stuff, and would prefer to see
    Eli> the patch for emacs-29 and another for master, please.

3 patches, 2 for emacs-29 below.

The 3rd one for master is just

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 2892728ebd9..28bac0401ed 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -902,7 +902,6 @@ 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)))
       ;; Add info node.
       (when (file-exists-p (expand-file-name "dir" pkg-dir))
         ;; FIXME: not the friendliest, but simple.

Robert
-- 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-duplicate-load-path-entry-when-generating-pack.patch --]
[-- Type: text/x-diff, Size: 1583 bytes --]

From 713da42a9c4569093368dcc41f606fb460fcd0e1 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Mon, 22 May 2023 15:44:21 +0200
Subject: [PATCH 1/2] Avoid duplicate load-path entry when generating package
 autoloads
To: emacs-devel@gnu.org

'file-name-directory' produces a path ending in '/', so that needs to be
run through 'directory-file-name' to avoid duplicate entries in
'load-path'.  (Bug#63625)

* lisp/emacs-lisp/package.el (package-generate-autoloads): Call
'directory-file-name' on the directory of 'load-file-name'.
---
 lisp/emacs-lisp/package.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index c684840ab7e..3d3da7909d7 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1110,8 +1110,12 @@ 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.
-        (or (and load-file-name (file-name-directory load-file-name))
-            (car load-path)))))
+        ;; `loaddefs-generate' has code to do this for us, but it's
+        ;; not currently exposed.  (Bug#63625)
+        (or (and load-file-name
+                 (directory-file-name
+                  (file-name-directory load-file-name)))
+             (car load-path)))))
     (let ((buf (find-buffer-visiting output-file)))
       (when buf (kill-buffer buf)))
     auto-name))
-- 
2.38.1.420.g319605f8f0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Avoid-duplicates-when-adding-package-dirs-to-load-pa.patch --]
[-- Type: text/x-diff, Size: 1605 bytes --]

From 983ed8b5d1f30e58be5d2e165cefefc8c70ae2f9 Mon Sep 17 00:00:00 2001
From: Stefan Monnier <monnier@iro.umontreal.ca>
Date: Mon, 22 May 2023 18:49:26 +0200
Subject: [PATCH 2/2] Avoid duplicates when adding package dirs to load-path
To: emacs-devel@gnu.org

* lisp/emacs-lisp/package.el (package-activate-1): Check if the path
we're about to add is already in 'load-path', since package autoload
files have been updating 'load-path' for a decade.

Do not merge to master, we're going to delete this code there.
---
 lisp/emacs-lisp/package.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 3d3da7909d7..4665ef0aa8e 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -904,7 +904,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.
-- 
2.38.1.420.g319605f8f0

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: philipk@posteo.net,  toddasmith@mac.com,  monnier@iro.umontreal.ca,
>   63625@debbugs.gnu.org
> Date: Mon, 22 May 2023 18:57:13 +0200
> 
>     Eli> I'm a bit confused by "that patch" and stuff, and would prefer to see
>     Eli> the patch for emacs-29 and another for master, please.
> 
> 3 patches, 2 for emacs-29 below.
> 
> The 3rd one for master is just

Thanks, these are fine to go in, as far as I'm concerned.

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

[Robert Pluim]

tags 63625 fixed
close 63625 29.1
quit

>>>>> On Tue, 23 May 2023 15:06:00 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: philipk@posteo.net,  toddasmith@mac.com,  monnier@iro.umontreal.ca,
    >> 63625@debbugs.gnu.org
    >> Date: Mon, 22 May 2023 18:57:13 +0200
    >> 
    Eli> I'm a bit confused by "that patch" and stuff, and would prefer to see
    Eli> the patch for emacs-29 and another for master, please.
    >> 
    >> 3 patches, 2 for emacs-29 below.
    >> 
    >> The 3rd one for master is just

    Eli> Thanks, these are fine to go in, as far as I'm concerned.

Closing.
Committed as 6f6071c5261 and 0abb79ca09a for emacs-29, and 1d5b164109b
for master.

Robert
-- 

bug#63625: 29.0.90; package-install inserts package directory into load-path twice.

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

> Committed as 6f6071c5261 and 0abb79ca09a for emacs-29, and 1d5b164109b
> for master.

Thanks, Robert,


        Stefan