From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.bugs Subject: bug#62734: Always fully rebuild autoloads in package-generate-autoloads Date: Sat, 29 Apr 2023 08:19:35 +0000 Message-ID: <87354jnlrc.fsf@posteo.net> References: <87lej2oz14.fsf@le0.gs> <874jp0aw7h.fsf@posteo.net> <83pm7oqa7d.fsf@gnu.org> <87o7n7ga4x.fsf@posteo.net> <83o7n7ri64.fsf@gnu.org> <87fs8jg93g.fsf@posteo.net> <83mt2rqm57.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29893"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 62734@debbugs.gnu.org, leo.gaskin@le0.gs To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Apr 29 10:20:32 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1psfom-0007Zq-CZ for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 29 Apr 2023 10:20:32 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1psfoN-0005Nk-EA; Sat, 29 Apr 2023 04:20:07 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1psfoK-0005NX-UB for bug-gnu-emacs@gnu.org; Sat, 29 Apr 2023 04:20:05 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1psfoI-0000ki-Qx for bug-gnu-emacs@gnu.org; Sat, 29 Apr 2023 04:20:04 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1psfoI-0000Ie-FM for bug-gnu-emacs@gnu.org; Sat, 29 Apr 2023 04:20:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Philip Kaludercic Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 29 Apr 2023 08:20:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 62734 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 62734-submit@debbugs.gnu.org id=B62734.16827563561094 (code B ref 62734); Sat, 29 Apr 2023 08:20:02 +0000 Original-Received: (at 62734) by debbugs.gnu.org; 29 Apr 2023 08:19:16 +0000 Original-Received: from localhost ([127.0.0.1]:35175 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1psfnX-0000HZ-At for submit@debbugs.gnu.org; Sat, 29 Apr 2023 04:19:15 -0400 Original-Received: from mout02.posteo.de ([185.67.36.66]:53031) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1psfnT-0000HI-04 for 62734@debbugs.gnu.org; Sat, 29 Apr 2023 04:19:13 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id EBD07240287 for <62734@debbugs.gnu.org>; Sat, 29 Apr 2023 10:19:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1682756345; bh=higtTOzPzwHhNyxXeMCEsXypJ30YvFHtO2xfm5x3QG4=; h=From:To:Cc:Subject:Autocrypt:Date:From; b=GPhf9TrF0vQzW706uR/kySX4Sj4aXl2AK61i1IkdjzDhyvjZoEHYcrhxogPbyH+g5 Hdp613QBeWANScMUAn0AH9kwyoKOiPT9qTkAS4ww4R3QxMGSSzg67V+3AP7hs4mdHv XnybhpFRe+Q6Z2s3Mw3ta85JOHN+FX0R/ohV+n+fdMA2x8ZYYMsf64b+Y55cJachA/ s7dkuFNrA6zNbac8zTXwxtTJ1LUyfrYM3L6A/QXtGMy1m0LKmbGc9+29J2HjslVsxX U66pkyPp/akR05cwcyLS9hVb1O7QOzu0hTzvPtlVcstso41bZh2JgRYC8TkBRVaoKt 2r2XGirM0iqlQ== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Q7j6t696kz6twS; Sat, 29 Apr 2023 10:19:02 +0200 (CEST) In-Reply-To: <83mt2rqm57.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 29 Apr 2023 08:43:00 +0300") Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:260803 Archived-At: --=-=-= Content-Type: text/plain Eli Zaretskii writes: >> From: Philip Kaludercic >> Cc: leo.gaskin@le0.gs, 62734@debbugs.gnu.org >> Date: Fri, 28 Apr 2023 18:22:43 +0000 >> >> Eli Zaretskii 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: --=-=-= Content-Type: text/plain Content-Disposition: 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." --=-=-=--