bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

I attempted to upgrade my packages today using `list-packages', which
errored out when regenerating the autoloads for vlf-write.el:

Debugger entered--Lisp error: (search-failed "\n\f\n;;;")
  search-forward("\n\f\n;;;")
  loaddefs-generate("/home/rpluim/.emacs.d/elpa/vlf-20191126.2250" "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250/vlf-a..." nil "(add-to-list 'load-path (or (and load-file-name (f...")
  package-generate-autoloads(vlf "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250")


The vlf-autoloads.el file ends like this:

--begin--
\f
;;; Generated autoloads from vlf-tune.el

(register-definition-prefixes "vlf-tune" '("vlf-"))

\f
;;; Generated autoloads from vlf-write.el

(register-definition-prefixes "vlf-write" '("vlf-"))
\f
;; Local Variables:
;; version-control: never
;; no-byte-compile: t
;; no-update-autoloads: t
;; coding: utf-8
;; End:
;;; vlf-autoloads.el ends here
--end--

which doesnʼt contain "\n\f\n;;;" after the vlf-write entry.

The code in question from loaddefs-gen.el:629 is

              (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)))

So itʼs assuming that the entry ends with ";;;"

This is a really old package, and itʼs probably somewhat obsolete, but
perhaps we should support such an old format without erroring anyway.

In GNU Emacs 29.0.90 (build 8, x86_64-pc-linux-gnu, GTK+ Version
 3.24.24, cairo version 1.16.0) of 2023-05-02 built on rltb
Repository revision: 16ecbf5af6d4fcb500941ede4d029cffd8eac757
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12009000
System Description: Debian GNU/Linux 11 (bullseye)

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

>>>>> On Tue, 02 May 2023 19:06:14 +0200, Robert Pluim <rpluim@gmail.com> said:

    Robert> I attempted to upgrade my packages today using `list-packages', which
    Robert> errored out when regenerating the autoloads for vlf-write.el:

    Robert> Debugger entered--Lisp error: (search-failed "\n\f\n;;;")
    Robert>   search-forward("\n\f\n;;;")
    Robert>   loaddefs-generate("/home/rpluim/.emacs.d/elpa/vlf-20191126.2250" "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250/vlf-a..." nil "(add-to-list 'load-path (or (and load-file-name (f...")
    Robert>   package-generate-autoloads(vlf "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250")

The minimal patch I can come up with is this, which seems safe enough
but Iʼd appreciate it if someone who knows the code could comment

diff --git a/lisp/emacs-lisp/loaddefs-gen.el b/lisp/emacs-lisp/loaddefs-gen.el
index a966b1e9f40..c25950a1afd 100644
--- a/lisp/emacs-lisp/loaddefs-gen.el
+++ b/lisp/emacs-lisp/loaddefs-gen.el
@@ -637,7 +637,7 @@ loaddefs-generate
                       (search-backward "\f\n" nil t))
                   ;; Delete the old version of the section.
                   (delete-region (match-beginning 0)
-                                 (and (search-forward "\n\f\n;;;")
+                                 (and (re-search-forward "\n\f\n;;;?")
                                       (match-beginning 0)))
                   (forward-line -2)))
               (insert head)

Robert
-- 

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Andreas Schwab]

On Mai 03 2023, Robert Pluim wrote:

> diff --git a/lisp/emacs-lisp/loaddefs-gen.el b/lisp/emacs-lisp/loaddefs-gen.el
> index a966b1e9f40..c25950a1afd 100644
> --- a/lisp/emacs-lisp/loaddefs-gen.el
> +++ b/lisp/emacs-lisp/loaddefs-gen.el
> @@ -637,7 +637,7 @@ loaddefs-generate
>                        (search-backward "\f\n" nil t))
>                    ;; Delete the old version of the section.
>                    (delete-region (match-beginning 0)
> -                                 (and (search-forward "\n\f\n;;;")
> +                                 (and (re-search-forward "\n\f\n;;;?")

This is equivalent to (search-forward "\n\f\n;;").

>                                        (match-beginning 0)))

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 03 May 2023 10:40:28 +0200
> 
> >>>>> On Tue, 02 May 2023 19:06:14 +0200, Robert Pluim <rpluim@gmail.com> said:
> 
>     Robert> I attempted to upgrade my packages today using `list-packages', which
>     Robert> errored out when regenerating the autoloads for vlf-write.el:
> 
>     Robert> Debugger entered--Lisp error: (search-failed "\n\f\n;;;")
>     Robert>   search-forward("\n\f\n;;;")
>     Robert>   loaddefs-generate("/home/rpluim/.emacs.d/elpa/vlf-20191126.2250" "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250/vlf-a..." nil "(add-to-list 'load-path (or (and load-file-name (f...")
>     Robert>   package-generate-autoloads(vlf "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250")
> 
> The minimal patch I can come up with is this, which seems safe enough
> but Iʼd appreciate it if someone who knows the code could comment

Can you describe what was special in your use case, and explain why
the original code didn't work in that case?

Thanks.

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

>>>>> On Wed, 03 May 2023 14:21:05 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Wed, 03 May 2023 10:40:28 +0200
    >> 
    >> >>>>> On Tue, 02 May 2023 19:06:14 +0200, Robert Pluim <rpluim@gmail.com> said:
    >> 
    Robert> I attempted to upgrade my packages today using `list-packages', which
    Robert> errored out when regenerating the autoloads for vlf-write.el:
    >> 
    Robert> Debugger entered--Lisp error: (search-failed "\n\f\n;;;")
    Robert> search-forward("\n\f\n;;;")
    Robert> loaddefs-generate("/home/rpluim/.emacs.d/elpa/vlf-20191126.2250" "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250/vlf-a..." nil "(add-to-list 'load-path (or (and load-file-name (f...")
    Robert> package-generate-autoloads(vlf "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250")
    >> 
    >> The minimal patch I can come up with is this, which seems safe enough
    >> but Iʼd appreciate it if someone who knows the code could comment

    Eli> Can you describe what was special in your use case, and explain why
    Eli> the original code didn't work in that case?

Itʼs not the use case thatʼs special, itʼs the package I was installing:
The autoloads file shipped with vlf-20191126.2250 didnʼt have a line
starting with ';;;' after the final entry in the file, which is what
the code was looking for.

Looking at the history, the autoloads generation was rewritten for
emacs-29, so this looks like a minor regression.

Robert
-- 

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

>>>>> On Wed, 03 May 2023 15:47:21 +0200, Robert Pluim <rpluim@gmail.com> said:

    Robert> Looking at the history, the autoloads generation was rewritten for
    Robert> emacs-29, so this looks like a minor regression.

And Iʼve just tried installing with emacs-26, emacs-27, and emacs-28,
and they all work, but none of them produce the format thatʼs in the
vlf package itself.

Robert
-- 

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 63236@debbugs.gnu.org
> Date: Wed, 03 May 2023 15:47:21 +0200
> 
> >>>>> On Wed, 03 May 2023 14:21:05 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> From: Robert Pluim <rpluim@gmail.com>
>     >> Date: Wed, 03 May 2023 10:40:28 +0200
>     >> 
>     >> >>>>> On Tue, 02 May 2023 19:06:14 +0200, Robert Pluim <rpluim@gmail.com> said:
>     >> 
>     Robert> I attempted to upgrade my packages today using `list-packages', which
>     Robert> errored out when regenerating the autoloads for vlf-write.el:
>     >> 
>     Robert> Debugger entered--Lisp error: (search-failed "\n\f\n;;;")
>     Robert> search-forward("\n\f\n;;;")
>     Robert> loaddefs-generate("/home/rpluim/.emacs.d/elpa/vlf-20191126.2250" "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250/vlf-a..." nil "(add-to-list 'load-path (or (and load-file-name (f...")
>     Robert> package-generate-autoloads(vlf "/home/rpluim/.emacs.d/elpa/vlf-20191126.2250")
>     >> 
>     >> The minimal patch I can come up with is this, which seems safe enough
>     >> but Iʼd appreciate it if someone who knows the code could comment
> 
>     Eli> Can you describe what was special in your use case, and explain why
>     Eli> the original code didn't work in that case?
> 
> Itʼs not the use case thatʼs special, itʼs the package I was installing:
> The autoloads file shipped with vlf-20191126.2250 didnʼt have a line
> starting with ';;;' after the final entry in the file, which is what
> the code was looking for.

But if the search for "\n\f\n;;;" fails, why should we assume that the
search for "\n\f\n;;" will succeed?  A broken autoloads file can be
broken in a much worse manner, no?  Is even a form-feed guaranteed?

Maybe we should assume, if the search for the canonical last line
fails, that there's no canonical last line, and just insert it
ourselves?

What did the old code do in these cases?

> Looking at the history, the autoloads generation was rewritten for
> emacs-29, so this looks like a minor regression.

Yes, loaddefs-gen.el is new, so we must fix it on the emacs-29 branch.

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

>>>>> On Wed, 03 May 2023 18:50:20 +0300, Eli Zaretskii <eliz@gnu.org> said:
    Eli> But if the search for "\n\f\n;;;" fails, why should we assume that the
    Eli> search for "\n\f\n;;" will succeed?  A broken autoloads file can be
    Eli> broken in a much worse manner, no?  Is even a form-feed guaranteed?

Nothing is guaranteed, but in the only example in the wild Iʼve found where the
emacs-29 code fails, searching for only two ';' works.

    Eli> Maybe we should assume, if the search for the canonical last line
    Eli> fails, that there's no canonical last line, and just insert it
    Eli> ourselves?

Yes, but the issue here is that the code is deciding where to end the
region that itʼs deleting from an existing autoloads file. If we donʼt
do the deletion, we get duplicate entries (or maybe even a broken
autoloads file).

    Eli> What did the old code do in these cases?

Iʼm not sure. A quick read of emacs-lisp/autoloads.el in emacs-28
seems to indicate itʼs looking for ';', but I havenʼt actually tested
it.

    >> Looking at the history, the autoloads generation was rewritten for
    >> emacs-29, so this looks like a minor regression.

    Eli> Yes, loaddefs-gen.el is new, so we must fix it on the emacs-29 branch.

Then the easiest fix would be to search for two ';' instead of three.

Robert
-- 

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 63236@debbugs.gnu.org
> Date: Thu, 04 May 2023 10:45:37 +0200
> 
>     >> Looking at the history, the autoloads generation was rewritten for
>     >> emacs-29, so this looks like a minor regression.
> 
>     Eli> Yes, loaddefs-gen.el is new, so we must fix it on the emacs-29 branch.
> 
> Then the easiest fix would be to search for two ';' instead of three.

OK, let's do that, and thanks.

bug#63236: 29.0.90; loaddefs-generate not 100% backwards compatible

[Robert Pluim]

tags 63236 fixed
close 63236 29.1
quit

>>>>> On Thu, 04 May 2023 13:42:05 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: 63236@debbugs.gnu.org
    >> Date: Thu, 04 May 2023 10:45:37 +0200
    >> 
    >> >> Looking at the history, the autoloads generation was rewritten for
    >> >> emacs-29, so this looks like a minor regression.
    >> 
    Eli> Yes, loaddefs-gen.el is new, so we must fix it on the emacs-29 branch.
    >> 
    >> Then the easiest fix would be to search for two ';' instead of three.

    Eli> OK, let's do that, and thanks.

Iʼve just pushed that. I tested upgrading my packages with the
resulting emacs, and nothing broke, as far as I can tell.

Closing.
Committed as 94e984e6700

Robert
--