bug#62734: Always fully rebuild autoloads in package-generate-autoloads

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
>> Date: Fri, 28 Apr 2023 18:22:43 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > What is meant by "building the package"?  Is it just compiling the
>> > Lisp files?
>> 
>> >From `package-vc-rebuild':
>> 
>>   Rebuilding an installation means scraping for new autoload
>>   cookies, re-compiling Emacs Lisp files, building and installing
>>   any documentation, downloading any missing dependencies.
>
> Thanks.  As a tangent: this is confusing terminology, so it is
> unfortunate that it was selected for this operation.

Is there a different way you would have put this?  What we do here is
sort of combining the steps that the ELPA server and a local
package-install do into one, and in my mind this constitutes building
software.

>> >> (time-less-p output-time
>> >>   (file-attribute-modification-time
>> >>   (file-attributes file)))
>> >> --8<---------------cut here---------------end--------------->8---
>> >> 
>> >> does not hold
>> >
>> > Why would it not hold?  Updating from VCS should update the timestamp
>> > of the updated files.
>> 
>> I don't think this necessarily holds if there were no changes affecting
>> a file.
>
> I don't follow: a file that didn't change doesn't need its autoloads
> updated, right?

Right, but if we want to add some additional code to the autoloads (as
we are with package.el, when injecting load-path modification), then
loaddefs-generate does not re-use the old data, and instead just throws
it away and creates a new file with contents of EXTRA-DATA, but without
any autoload information.

And I have checked, the only place where `loaddefs-generate' is invoked
with EXTRA-DATA, is in `package-generate-autoloads'.  So the fact that
all other places where this function work as intended makes sense.

>> >> Another idea is just to get rid of this faulty optimisation.  From my
>> >> tests this would also resolve the bug.
>> >
>> > I don't yet understand what optimization is that, but getting rid of
>> > it should not alter what the code does for the loaddefs files inside
>> > the Emacs tree, because there it does work, and I don't want to touch
>> > that.
>> 
>> Are you sure it does work?
>
> It works well in the Emacs tree, I'm sure.  So if it doesn't in this
> case, I'd encourage some debugging, because it could be that this is
> some subtle bug or feature in loaddefs-generate, and we should
> investigate that and fix whatever needs fixing now, since this
> function is new in Emacs 29.

I think the central issue here is the

  (and (not defs) extra-data)

check.  Just because we did not find any new definitions to autoload
/and/ EXTRA-DATA is non-nil, does not mean the old contents should be
discarded.  The else-case already does the right thing, so I really do
think that getting rid of the `if' could resolve the issue:

diff --git a/lisp/emacs-lisp/loaddefs-gen.el b/lisp/emacs-lisp/loaddefs-gen.el
index 1007be62dd9..8da0795870c 100644
--- a/lisp/emacs-lisp/loaddefs-gen.el
+++ b/lisp/emacs-lisp/loaddefs-gen.el
@@ -597,73 +597,64 @@ loaddefs-generate
                                 defs))))))
       (progress-reporter-done progress))
 
-    ;; If we have no autoloads data, but we have EXTRA-DATA, then
-    ;; generate the (almost) empty file anyway.
-    (if (and (not defs) extra-data)
+    ;; We have some data, so generate the loaddef files.  First
+    ;; group per output file.
+    (dolist (fdefs (seq-group-by (lambda (x) (expand-file-name (car x)))
+                                 defs))
+      (let ((loaddefs-file (car fdefs))
+            hash)
         (with-temp-buffer
-          (insert (loaddefs-generate--rubric output-file nil t))
-          (search-backward "\f")
-          (insert extra-data)
-          (ensure-empty-lines 1)
-          (write-region (point-min) (point-max) output-file nil 'silent))
-      ;; We have some data, so generate the loaddef files.  First
-      ;; group per output file.
-      (dolist (fdefs (seq-group-by (lambda (x) (expand-file-name (car x)))
-                                   defs))
-        (let ((loaddefs-file (car fdefs))
-              hash)
-          (with-temp-buffer
-            (if (and updating (file-exists-p loaddefs-file))
-                (insert-file-contents loaddefs-file)
-              (insert (loaddefs-generate--rubric
-                       loaddefs-file nil t include-package-version))
-              (search-backward "\f")
-              (when extra-data
-                (insert extra-data)
-                (ensure-empty-lines 1)))
-            (setq hash (buffer-hash))
-            ;; Then group by source file (and sort alphabetically).
-            (dolist (section (sort (seq-group-by #'cadr (cdr fdefs))
-                                   (lambda (e1 e2)
-                                     (string<
-                                      (file-name-sans-extension
-                                       (file-name-nondirectory (car e1)))
-                                      (file-name-sans-extension
-                                       (file-name-nondirectory (car e2)))))))
-              (pop section)
-              (let* ((relfile (file-relative-name
-                               (cadar section)
-                               (file-name-directory loaddefs-file)))
-                     (head (concat "\n\f\n;;; Generated autoloads from "
-                                   relfile "\n\n")))
-                (when (file-exists-p loaddefs-file)
-                  ;; If we're updating an old loaddefs file, then see if
-                  ;; there's a section here for this file already.
-                  (goto-char (point-min))
-                  (if (not (search-forward head nil t))
-                      ;; It's a new file; put the data at the end.
-                      (progn
-                        (goto-char (point-max))
-                        (search-backward "\f\n" nil t))
-                    ;; Delete the old version of the section.
-                    (delete-region (match-beginning 0)
-                                   (and (search-forward "\n\f\n;;;")
-                                        (match-beginning 0)))
-                    (forward-line -2)))
-                (insert head)
-                (dolist (def (reverse section))
-                  (setq def (caddr def))
-                  (if (stringp def)
-                      (princ def (current-buffer))
-                    (loaddefs-generate--print-form def))
-                  (unless (bolp)
-                    (insert "\n")))))
-            ;; Only write the file if we actually made a change.
-            (unless (equal (buffer-hash) hash)
-              (write-region (point-min) (point-max) loaddefs-file nil 'silent)
-              (byte-compile-info
-               (file-relative-name loaddefs-file (car (ensure-list dir)))
-               t "GEN"))))))))
+          (if (and updating (file-exists-p loaddefs-file))
+              (insert-file-contents loaddefs-file)
+            (insert (loaddefs-generate--rubric
+                     loaddefs-file nil t include-package-version))
+            (search-backward "\f")
+            (when extra-data
+              (insert extra-data)
+              (ensure-empty-lines 1)))
+          (setq hash (buffer-hash))
+          ;; Then group by source file (and sort alphabetically).
+          (dolist (section (sort (seq-group-by #'cadr (cdr fdefs))
+                                 (lambda (e1 e2)
+                                   (string<
+                                    (file-name-sans-extension
+                                     (file-name-nondirectory (car e1)))
+                                    (file-name-sans-extension
+                                     (file-name-nondirectory (car e2)))))))
+            (pop section)
+            (let* ((relfile (file-relative-name
+                             (cadar section)
+                             (file-name-directory loaddefs-file)))
+                   (head (concat "\n\f\n;;; Generated autoloads from "
+                                 relfile "\n\n")))
+              (when (file-exists-p loaddefs-file)
+                ;; If we're updating an old loaddefs file, then see if
+                ;; there's a section here for this file already.
+                (goto-char (point-min))
+                (if (not (search-forward head nil t))
+                    ;; It's a new file; put the data at the end.
+                    (progn
+                      (goto-char (point-max))
+                      (search-backward "\f\n" nil t))
+                  ;; Delete the old version of the section.
+                  (delete-region (match-beginning 0)
+                                 (and (search-forward "\n\f\n;;;")
+                                      (match-beginning 0)))
+                  (forward-line -2)))
+              (insert head)
+              (dolist (def (reverse section))
+                (setq def (caddr def))
+                (if (stringp def)
+                    (princ def (current-buffer))
+                  (loaddefs-generate--print-form def))
+                (unless (bolp)
+                  (insert "\n")))))
+          ;; Only write the file if we actually made a change.
+          (unless (equal (buffer-hash) hash)
+            (write-region (point-min) (point-max) loaddefs-file nil 'silent)
+            (byte-compile-info
+             (file-relative-name loaddefs-file (car (ensure-list dir)))
+             t "GEN")))))))
 
 (defun loaddefs-generate--print-form (def)
   "Print DEF in a format that makes sense for version control."