* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. @ 2023-05-20 21:45 todd smith via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 8:55 ` Robert Pluim 0 siblings, 1 reply; 16+ messages in thread From: todd smith via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-20 21:45 UTC (permalink / raw) To: 63625 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-20 21:45 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 @ 2023-05-22 8:55 ` Robert Pluim 2023-05-22 11:25 ` Eli Zaretskii 2023-05-22 13:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 2 replies; 16+ messages in thread From: Robert Pluim @ 2023-05-22 8:55 UTC (permalink / raw) To: 63625; +Cc: todd smith >>>>> 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 -- ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 8:55 ` Robert Pluim @ 2023-05-22 11:25 ` Eli Zaretskii 2023-05-22 12:46 ` Robert Pluim 2023-05-22 13:18 ` Philip Kaludercic 2023-05-22 13:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 2 replies; 16+ messages in thread From: Eli Zaretskii @ 2023-05-22 11:25 UTC (permalink / raw) To: Robert Pluim, Philip Kaludercic, Stefan Monnier; +Cc: 63625, toddasmith > 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 11:25 ` Eli Zaretskii @ 2023-05-22 12:46 ` Robert Pluim 2023-05-22 13:18 ` Philip Kaludercic 1 sibling, 0 replies; 16+ messages in thread From: Robert Pluim @ 2023-05-22 12:46 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Philip Kaludercic, toddasmith, Stefan Monnier, 63625 >>>>> 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 11:25 ` Eli Zaretskii 2023-05-22 12:46 ` Robert Pluim @ 2023-05-22 13:18 ` Philip Kaludercic 2023-05-22 13:54 ` Robert Pluim 1 sibling, 1 reply; 16+ messages in thread From: Philip Kaludercic @ 2023-05-22 13:18 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Robert Pluim, toddasmith, Stefan Monnier, 63625 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 13:18 ` Philip Kaludercic @ 2023-05-22 13:54 ` Robert Pluim 0 siblings, 0 replies; 16+ messages in thread From: Robert Pluim @ 2023-05-22 13:54 UTC (permalink / raw) To: Philip Kaludercic; +Cc: Eli Zaretskii, toddasmith, Stefan Monnier, 63625 >>>>> 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 8:55 ` Robert Pluim 2023-05-22 11:25 ` Eli Zaretskii @ 2023-05-22 13:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 14:19 ` Robert Pluim 1 sibling, 1 reply; 16+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-22 13:58 UTC (permalink / raw) To: Robert Pluim; +Cc: Philip Kaludercic, 63625, todd smith > 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. ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 13:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-22 14:19 ` Robert Pluim 2023-05-22 15:04 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 15:49 ` Eli Zaretskii 0 siblings, 2 replies; 16+ messages in thread From: Robert Pluim @ 2023-05-22 14:19 UTC (permalink / raw) To: Stefan Monnier; +Cc: Philip Kaludercic, 63625, todd smith >>>>> 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 14:19 ` Robert Pluim @ 2023-05-22 15:04 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 15:36 ` Robert Pluim 2023-05-22 15:49 ` Eli Zaretskii 1 sibling, 1 reply; 16+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-22 15:04 UTC (permalink / raw) To: Robert Pluim; +Cc: Philip Kaludercic, 63625, todd smith > `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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 15:04 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-22 15:36 ` Robert Pluim 2023-05-22 15:53 ` Eli Zaretskii 0 siblings, 1 reply; 16+ messages in thread From: Robert Pluim @ 2023-05-22 15:36 UTC (permalink / raw) To: Stefan Monnier; +Cc: Philip Kaludercic, 63625, todd smith >>>>> 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 15:36 ` Robert Pluim @ 2023-05-22 15:53 ` Eli Zaretskii 2023-05-22 16:57 ` Robert Pluim 0 siblings, 1 reply; 16+ messages in thread From: Eli Zaretskii @ 2023-05-22 15:53 UTC (permalink / raw) To: Robert Pluim; +Cc: philipk, toddasmith, monnier, 63625 > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 15:53 ` Eli Zaretskii @ 2023-05-22 16:57 ` Robert Pluim 2023-05-23 12:06 ` Eli Zaretskii 0 siblings, 1 reply; 16+ messages in thread From: Robert Pluim @ 2023-05-22 16:57 UTC (permalink / raw) To: Eli Zaretskii; +Cc: philipk, 63625, monnier, toddasmith [-- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 16:57 ` Robert Pluim @ 2023-05-23 12:06 ` Eli Zaretskii 2023-05-23 13:20 ` Robert Pluim 0 siblings, 1 reply; 16+ messages in thread From: Eli Zaretskii @ 2023-05-23 12:06 UTC (permalink / raw) To: Robert Pluim; +Cc: philipk, 63625, monnier, toddasmith > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-23 12:06 ` Eli Zaretskii @ 2023-05-23 13:20 ` Robert Pluim 2023-05-23 13:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 16+ messages in thread From: Robert Pluim @ 2023-05-23 13:20 UTC (permalink / raw) To: Eli Zaretskii; +Cc: philipk, 63625, monnier, toddasmith 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-23 13:20 ` Robert Pluim @ 2023-05-23 13:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 0 replies; 16+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-23 13:46 UTC (permalink / raw) To: Robert Pluim; +Cc: Eli Zaretskii, 63625, philipk, toddasmith > Committed as 6f6071c5261 and 0abb79ca09a for emacs-29, and 1d5b164109b > for master. Thanks, Robert, Stefan ^ permalink raw reply [flat|nested] 16+ messages in thread
* bug#63625: 29.0.90; package-install inserts package directory into load-path twice. 2023-05-22 14:19 ` Robert Pluim 2023-05-22 15:04 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-05-22 15:49 ` Eli Zaretskii 1 sibling, 0 replies; 16+ messages in thread From: Eli Zaretskii @ 2023-05-22 15:49 UTC (permalink / raw) To: Robert Pluim; +Cc: philipk, toddasmith, monnier, 63625 > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-05-23 13:46 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-20 21:45 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 2023-05-22 8:55 ` Robert Pluim 2023-05-22 11:25 ` Eli Zaretskii 2023-05-22 12:46 ` Robert Pluim 2023-05-22 13:18 ` Philip Kaludercic 2023-05-22 13:54 ` Robert Pluim 2023-05-22 13:58 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 14:19 ` Robert Pluim 2023-05-22 15:04 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 15:36 ` Robert Pluim 2023-05-22 15:53 ` Eli Zaretskii 2023-05-22 16:57 ` Robert Pluim 2023-05-23 12:06 ` Eli Zaretskii 2023-05-23 13:20 ` Robert Pluim 2023-05-23 13:46 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-05-22 15:49 ` Eli Zaretskii
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).