bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Chris Feng]

Currently `package--builtin-versions' does not contain versioned preloaded
libraries (cl-generic, tabulated-list), which makes package.el believe they're
not builtin and install the versions from ELPA.  This patch fixes the problem.

Also, since `package--builtin-versions' is now complete, should we remove the
version info in `package--builtins'?

Chris
---

* lisp/emacs-lisp/autoload.el (update-directory-autoloads): Do not exclude
preloaded libraries or remove entries generated for them.
(autoload-generate-file-autoloads): Do not generate autoload statements for
preloaded libraries.
---
 lisp/emacs-lisp/autoload.el | 57 ++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/lisp/emacs-lisp/autoload.el b/lisp/emacs-lisp/autoload.el
index e688d6b..2f2c856 100644
--- a/lisp/emacs-lisp/autoload.el
+++ b/lisp/emacs-lisp/autoload.el
@@ -578,22 +578,24 @@ autoload-generate-file-autoloads
                                                package--builtin-versions))
                                  (princ "\n")))))
 
-                      (goto-char (point-min))
-                      (while (not (eobp))
-                        (skip-chars-forward " \t\n\f")
-                        (cond
-                         ((looking-at (regexp-quote generate-autoload-cookie))
-                          ;; If not done yet, figure out where to insert this text.
-                          (unless output-start
-                            (setq output-start (autoload--setup-output
-                                                otherbuf outbuf absfile load-name)))
-                          (autoload--print-cookie-text output-start load-name file))
-                         ((looking-at ";")
-                          ;; Don't read the comment.
-                          (forward-line 1))
-                         (t
-                          (forward-sexp 1)
-                          (forward-line 1))))))
+                      ;; Do not insert autoload entries for excluded files.
+                      (unless (member absfile autoload-excludes)
+                        (goto-char (point-min))
+                        (while (not (eobp))
+                          (skip-chars-forward " \t\n\f")
+                          (cond
+                           ((looking-at (regexp-quote generate-autoload-cookie))
+                            ;; If not done yet, figure out where to insert this text.
+                            (unless output-start
+                              (setq output-start (autoload--setup-output
+                                                  otherbuf outbuf absfile load-name)))
+                            (autoload--print-cookie-text output-start load-name file))
+                           ((looking-at ";")
+                            ;; Don't read the comment.
+                            (forward-line 1))
+                           (t
+                            (forward-sexp 1)
+                            (forward-line 1)))))))
 
                   (when output-start
                     (let ((secondary-autoloads-file-buf
@@ -810,9 +812,7 @@ update-directory-autoloads
 		  ((not (stringp file)))
 		  ((or (not (file-exists-p file))
                        ;; Remove duplicates as well, just in case.
-                       (member file done)
-                       ;; If the file is actually excluded.
-                       (member (expand-file-name file) autoload-excludes))
+                       (member file done))
                    ;; Remove the obsolete section.
 		   (autoload-remove-section (match-beginning 0)))
 		  ((not (time-less-p (nth 4 form)
@@ -830,16 +830,15 @@ update-directory-autoloads
       ;; Elements remaining in FILES have no existing autoload sections yet.
       (let ((no-autoloads-time (or last-time '(0 0 0 0))) file-time)
 	(dolist (file files)
-	  (cond
-	   ((member (expand-file-name file) autoload-excludes) nil)
-	   ;; Passing nil as second argument forces
-	   ;; autoload-generate-file-autoloads to look for the right
-	   ;; spot where to insert each autoloads section.
-	   ((setq file-time
-		  (autoload-generate-file-autoloads file nil buffer-file-name))
-	    (push file no-autoloads)
-	    (if (time-less-p no-autoloads-time file-time)
-		(setq no-autoloads-time file-time)))))
+          ;; Passing nil as second argument forces
+          ;; autoload-generate-file-autoloads to look for the right
+          ;; spot where to insert each autoloads section.
+          (setq file-time
+                (autoload-generate-file-autoloads file nil buffer-file-name))
+          (when file-time
+            (push file no-autoloads)
+            (if (time-less-p no-autoloads-time file-time)
+                (setq no-autoloads-time file-time))))
 
 	(when no-autoloads
 	  ;; Sort them for better readability.
-- 
2.7.0

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Chris Feng]

> Currently `package--builtin-versions' does not contain versioned preloaded
> libraries (cl-generic, tabulated-list), which makes package.el believe they're
> not builtin and install the versions from ELPA.  This patch fixes the problem.

I forgot to mention that the bug can be reproduced by installing a
package having a preloaded lib as a dependency, e.g.:

  M-x package-install RET xelb RET

Then cl-generic 0.2 will get installed.

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Stefan Monnier]

> Also, since `package--builtin-versions' is now complete, should we remove the
> version info in `package--builtins'?

Not sure what you mean nor what would be the benefit.

> I forgot to mention that the bug can be reproduced by installing a
> package having a preloaded lib as a dependency, e.g.:
>
>   M-x package-install RET xelb RET
>
> Then cl-generic 0.2 will get installed.

A more direct way is to check (assq 'cl-generic package--builtin-versions)
which should not return nil (but does :-( ).

I haven't looked in detail at your patch, but it'd be good to do
something along these lines.

It's probably too late to use your approach for emacs-25, but I think we
need to install some fix for this problem in emacs-25, since otherwise
Emacs-25.1 will end up insisting on installing cl-generic from GNU ELPA
every occasion it gets.

How 'bout you install your patch into master and we install the patch
below into emacs-25?


        Stefan


diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 37edf45..e5bab8d 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -86,6 +86,11 @@
 
 ;;; Code:
 
+;; The autoloads.el mechanism which adds package--builtin-versions
+;; maintenance to loaddefs.el doesn't work for preloaded packages (such
+;; as this one), so we have to do it by hand!
+(push (purecopy '(cl-generic 1 0)) package--builtin-versions)
+
 ;; Note: For generic functions that dispatch on several arguments (i.e. those
 ;; which use the multiple-dispatch feature), we always use the same "tagcodes"
 ;; and the same set of arguments on which to dispatch.  This works, but is

bug#22817: Fwd: bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Chris Feng]

>> Also, since `package--builtin-versions' is now complete, should we remove the
>> version info in `package--builtins'?
>
> Not sure what you mean nor what would be the benefit.

I just noticed you wrote this in finder.el:

    ;; FIXME: Now that we have package--builtin-versions, package--builtins is
    ;; only needed to get the list of unversioned packages and to get the
    ;; summary description of each package.

>> I forgot to mention that the bug can be reproduced by installing a
>> package having a preloaded lib as a dependency, e.g.:
>>
>>   M-x package-install RET xelb RET
>>
>> Then cl-generic 0.2 will get installed.
>
> A more direct way is to check (assq 'cl-generic package--builtin-versions)
> which should not return nil (but does :-( ).
>
> I haven't looked in detail at your patch, but it'd be good to do
> something along these lines.
>
> It's probably too late to use your approach for emacs-25, but I think we
> need to install some fix for this problem in emacs-25, since otherwise
> Emacs-25.1 will end up insisting on installing cl-generic from GNU ELPA
> every occasion it gets.
>
> How 'bout you install your patch into master and we install the patch
> below into emacs-25?

That'd be okay.

Chris

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Chris Feng]

>>>> Also, since `package--builtin-versions' is now complete, should we
>>>> remove the version info in `package--builtins'?
>>> Not sure what you mean nor what would be the benefit.
>> I just noticed you wrote this in finder.el:
>>     ;; FIXME: Now that we have package--builtin-versions, package--builtins
>>     ;; is only needed to get the list of unversioned packages and to get the
>>     ;; summary description of each package.
>
> Yes, but that still doesn't tell me what you suggest we do ;-)

I think we can simply drop the version info in `package--builtins' /
`package--bi-desc' so as to eliminate duplicated info and prevent
future confusion.  Also the variables are only used in a few places so
it'd be easy to make the change.

>>> How 'bout you install your patch into master and we install the patch
>>> below into emacs-25?
>> That'd be okay.
>
> Eli?  John?

Chris

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Stefan Monnier]

> I think we can simply drop the version info in `package--builtins' /
> `package--bi-desc' so as to eliminate duplicated info and prevent
> future confusion.  Also the variables are only used in a few places so
> it'd be easy to make the change.

I'm not completely sure the end result will be simpler/cleaner, but
I guess it's worth a try.


        Stefan

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Chris Feng]

>> I think we can simply drop the version info in `package--builtins' /
>> `package--bi-desc' so as to eliminate duplicated info and prevent
>> future confusion.  Also the variables are only used in a few places so
>> it'd be easy to make the change.
>
> I'm not completely sure the end result will be simpler/cleaner, but
> I guess it's worth a try.

I've installed this patch into master.  It turns out
`package--builtins' and `package--bi-desc' cannot be elegantly cleaned
up so I just give up the idea.

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[Stefan Monnier]

Version: 25.1

> Fine with me, thanks.

Thanks, installed.


        Stefan

bug#22817: 25.0.91; [PATCH] Include versioned preloaded libraries in `package--builtin-versions'

[John Wiegley]

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

>>>>> "SM" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

SM> How 'bout you install your patch into master and we install the patch
SM> below into emacs-25?

I'm not quite clear on the impact of this patch, so I defer to Eli on this
one.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]