Package: emacs;
Reported by: Leo Georg Gaskin <leo.gaskin <at> le0.gs>
Date: Sun, 9 Apr 2023 04:12:02 UTC
Severity: normal
Tags: patch
Done: Philip Kaludercic <philipk <at> posteo.net>
Bug is archived. No further changes may be made.
Message #35 received at 62734 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 62734 <at> debbugs.gnu.org, leo.gaskin <at> le0.gs Subject: Re: bug#62734: Always fully rebuild autoloads in package-generate-autoloads Date: Sat, 29 Apr 2023 08:19:35 +0000
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes: >> From: Philip Kaludercic <philipk <at> posteo.net> >> Cc: leo.gaskin <at> le0.gs, 62734 <at> debbugs.gnu.org >> Date: Fri, 28 Apr 2023 18:22:43 +0000 >> >> Eli Zaretskii <eliz <at> 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:
[Message part 2 (text/plain, inline)]
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."
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.