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

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

[Leo Georg Gaskin]

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

Tags: patch

Hello!

I've been using the new package-vc.el functionality to great effect,
but I have also come across a somewhat annoying bug.  If I use any of
the provided commands to rebuild my local package while a autoloads
file is already present, the newly generated autoloads file is
consistently either incomplete or empty.

The easiest way I've found to fix this is simply changing the
`package-generate-autoloads' function to always rebuild the autoloads
file by passing the relevant option to `loaddefs-generate'.  I think
this change also makes sense on a larger scale, as package generation
taking into account older build artifacts seems unintuitive.

The attached patch implements this change.

I've read that for small changes like this no copyright assignment
is needed.  If I have misunderstood, please point me to the relevant
documents so I can sign them.  Please also let me know if I have
messed something up or you need any additional information.


Best wishes

Leo Gaskin


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.16.0, Xaw3d scroll bars)
Repository revision: 9848ae17161828190cc0ba31e89ae54a2f08a2ef
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: NixOS 23.05 (Stoat)

Configured using:
 'configure
 --prefix=/nix/store/y9bxk3kqk4isr28jcy1bclkdr5a4zd1v-emacs-git-20230407.0
 --disable-build-details --with-modules --with-x-toolkit=lucid
 --with-xft --with-cairo --with-native-compilation'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Always-fully-rebuild-autoloads-in-package-generate-a.patch --]
[-- Type: text/patch, Size: 959 bytes --]

From dad173580c048cbe89d5287c4475e965c71a702e Mon Sep 17 00:00:00 2001
From: Leo Gaskin <leo.gaskin@le0.gs>
Date: Sat, 8 Apr 2023 23:13:59 +0200
Subject: [PATCH] Always fully rebuild autoloads in package-generate-autoloads

---
 lisp/emacs-lisp/package.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 685f983..09811f1 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1093,7 +1093,8 @@ untar into a directory named DIR; otherwise, signal an error."
         ;; the load path.  We don't hard-code `pkg-dir', to avoid
         ;; issues if the package directory is moved around.
         (or (and load-file-name (file-name-directory load-file-name))
-            (car load-path)))))
+            (car load-path))))
+     nil t)
     (let ((buf (find-buffer-visiting output-file)))
       (when buf (kill-buffer buf)))
     auto-name))
-- 
2.39.2

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

[Philip Kaludercic]

Leo Georg Gaskin <leo.gaskin@le0.gs> writes:

> Tags: patch
>
> Hello!
>
> I've been using the new package-vc.el functionality to great effect,
> but I have also come across a somewhat annoying bug.  If I use any of
> the provided commands to rebuild my local package while a autoloads
> file is already present, the newly generated autoloads file is
> consistently either incomplete or empty.

Can you reproduce this using emacs -Q?

> The easiest way I've found to fix this is simply changing the
> `package-generate-autoloads' function to always rebuild the autoloads
> file by passing the relevant option to `loaddefs-generate'.  I think
> this change also makes sense on a larger scale, as package generation
> taking into account older build artifacts seems unintuitive.

I guess this could work, but it might also be an inconvenience for
people with a lot of packages?  It seems to me that trying to better
understand the issue before proceeding to apply this quick fix would be
a better idea.  What do you think?

> The attached patch implements this change.
>
> I've read that for small changes like this no copyright assignment
> is needed.  If I have misunderstood, please point me to the relevant
> documents so I can sign them.  Please also let me know if I have
> messed something up or you need any additional information.

That should be the case.

>
> Best wishes
>
> Leo Gaskin
>
>
> In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
> version 1.16.0, Xaw3d scroll bars)
> Repository revision: 9848ae17161828190cc0ba31e89ae54a2f08a2ef
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
> System Description: NixOS 23.05 (Stoat)
>
> Configured using:
>  'configure
>  --prefix=/nix/store/y9bxk3kqk4isr28jcy1bclkdr5a4zd1v-emacs-git-20230407.0
>  --disable-build-details --with-modules --with-x-toolkit=lucid
>  --with-xft --with-cairo --with-native-compilation'
>
>>From dad173580c048cbe89d5287c4475e965c71a702e Mon Sep 17 00:00:00 2001
> From: Leo Gaskin <leo.gaskin@le0.gs>
> Date: Sat, 8 Apr 2023 23:13:59 +0200
> Subject: [PATCH] Always fully rebuild autoloads in package-generate-autoloads
>
> ---
>  lisp/emacs-lisp/package.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 685f983..09811f1 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1093,7 +1093,8 @@ untar into a directory named DIR; otherwise, signal an error."
>          ;; the load path.  We don't hard-code `pkg-dir', to avoid
>          ;; issues if the package directory is moved around.
>          (or (and load-file-name (file-name-directory load-file-name))
> -            (car load-path)))))
> +            (car load-path))))
> +     nil t)
>      (let ((buf (find-buffer-visiting output-file)))
>        (when buf (kill-buffer buf)))
>      auto-name))

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

[Leo Gaskin]

> Can you reproduce this using emacs -Q?

Yes I can. When I run the command `M-x package-vc-install helm RET',
then `M-x package-vc-rebuild helm RET' immediately after, in an Emacs
instance launched with the -Q option, the helm-autoloads.el file does
not contain the expected autoloads for the Helm package.

> I guess this could work, but it might also be an inconvenience for
> people with a lot of packages?

I think `package-generate-autoloads' already rarely or never reuses
previously generated autoloads files.
It seems to me that `package-vc-rebuild' is currently the only function
that calls `package-generate-autoloads' on a package with autoloads
files already present.  Other functions such as `package-update' first
delete the old package, meaning an old autoloads file is never present.

Also, for me at least, the amount of time it takes to generate these
autoload files is not really noticeable, at least compared the time it
takes to byte-compile the package.

> It seems to me that trying to better
> understand the issue before proceeding to apply this quick fix would be
> a better idea.  What do you think?

Sure, it would be a good idea to understand the underlying problem.
Unfortunately I do not understand the loaddefs generation code well
enough to offer a better fix right now. Investigating the issue further
would take more time, which I likely won't be able to spend on this
this month.

On Sun, Apr 23, 2023 at 3:15 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
> Leo Georg Gaskin <leo.gaskin@le0.gs> writes:
>
> > Tags: patch
> >
> > Hello!
> >
> > I've been using the new package-vc.el functionality to great effect,
> > but I have also come across a somewhat annoying bug.  If I use any of
> > the provided commands to rebuild my local package while a autoloads
> > file is already present, the newly generated autoloads file is
> > consistently either incomplete or empty.
>
> Can you reproduce this using emacs -Q?
>
> > The easiest way I've found to fix this is simply changing the
> > `package-generate-autoloads' function to always rebuild the autoloads
> > file by passing the relevant option to `loaddefs-generate'.  I think
> > this change also makes sense on a larger scale, as package generation
> > taking into account older build artifacts seems unintuitive.
>
> I guess this could work, but it might also be an inconvenience for
> people with a lot of packages?  It seems to me that trying to better
> understand the issue before proceeding to apply this quick fix would be
> a better idea.  What do you think?
>
> > The attached patch implements this change.
> >
> > I've read that for small changes like this no copyright assignment
> > is needed.  If I have misunderstood, please point me to the relevant
> > documents so I can sign them.  Please also let me know if I have
> > messed something up or you need any additional information.
>
> That should be the case.
>
> >
> > Best wishes
> >
> > Leo Gaskin
> >
> >
> > In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
> > version 1.16.0, Xaw3d scroll bars)
> > Repository revision: 9848ae17161828190cc0ba31e89ae54a2f08a2ef
> > Repository branch: master
> > Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
> > System Description: NixOS 23.05 (Stoat)
> >
> > Configured using:
> >  'configure
> >  --prefix=/nix/store/y9bxk3kqk4isr28jcy1bclkdr5a4zd1v-emacs-git-20230407.0
> >  --disable-build-details --with-modules --with-x-toolkit=lucid
> >  --with-xft --with-cairo --with-native-compilation'
> >
> >>From dad173580c048cbe89d5287c4475e965c71a702e Mon Sep 17 00:00:00 2001
> > From: Leo Gaskin <leo.gaskin@le0.gs>
> > Date: Sat, 8 Apr 2023 23:13:59 +0200
> > Subject: [PATCH] Always fully rebuild autoloads in package-generate-autoloads
> >
> > ---
> >  lisp/emacs-lisp/package.el | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> > index 685f983..09811f1 100644
> > --- a/lisp/emacs-lisp/package.el
> > +++ b/lisp/emacs-lisp/package.el
> > @@ -1093,7 +1093,8 @@ untar into a directory named DIR; otherwise, signal an error."
> >          ;; the load path.  We don't hard-code `pkg-dir', to avoid
> >          ;; issues if the package directory is moved around.
> >          (or (and load-file-name (file-name-directory load-file-name))
> > -            (car load-path)))))
> > +            (car load-path))))
> > +     nil t)
> >      (let ((buf (find-buffer-visiting output-file)))
> >        (when buf (kill-buffer buf)))
> >      auto-name))

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

[Philip Kaludercic]

Leo Gaskin <leo.gaskin@le0.gs> writes:

>> Can you reproduce this using emacs -Q?
>
> Yes I can. When I run the command `M-x package-vc-install helm RET',
> then `M-x package-vc-rebuild helm RET' immediately after, in an Emacs
> instance launched with the -Q option, the helm-autoloads.el file does
> not contain the expected autoloads for the Helm package.

Ok, thanks.

>> I guess this could work, but it might also be an inconvenience for
>> people with a lot of packages?
>
> I think `package-generate-autoloads' already rarely or never reuses
> previously generated autoloads files.

Why shouldn't it, shouldn't the list of autoloads of a package stay
relatively constant over package releases?

> It seems to me that `package-vc-rebuild' is currently the only function
> that calls `package-generate-autoloads' on a package with autoloads
> files already present.  

True, the only other case is `package-unpack' (via the function
`package--make-autoloads-and-stuff'), which are respectively only
invoked by `package-install-from-archive' and
`package-install-from-buffer', both of which do not re-use anything.

>                         Other functions such as `package-update' first
> delete the old package, meaning an old autoloads file is never present.

Right.

> Also, for me at least, the amount of time it takes to generate these
> autoload files is not really noticeable, at least compared the time it
> takes to byte-compile the package.

I do not know if this is always the case, but I wouldn't be surprised if
that was so.

>> It seems to me that trying to better
>> understand the issue before proceeding to apply this quick fix would be
>> a better idea.  What do you think?
>
> Sure, it would be a good idea to understand the underlying problem.
> Unfortunately I do not understand the loaddefs generation code well
> enough to offer a better fix right now. Investigating the issue further
> would take more time, which I likely won't be able to spend on this
> this month.

This might also very well be an issue with package-vc.  Either way, I
will take a look at it, perhaps there might be a better understood
solution (even though your fix looks like it should be safe).  If I
don't find anything, we can always "fall back" on to the patch you
provided.

> On Sun, Apr 23, 2023 at 3:15 PM Philip Kaludercic <philipk@posteo.net> wrote:
>>
>> Leo Georg Gaskin <leo.gaskin@le0.gs> writes:
>>
>> > Tags: patch
>> >
>> > Hello!
>> >
>> > I've been using the new package-vc.el functionality to great effect,
>> > but I have also come across a somewhat annoying bug.  If I use any of
>> > the provided commands to rebuild my local package while a autoloads
>> > file is already present, the newly generated autoloads file is
>> > consistently either incomplete or empty.
>>
>> Can you reproduce this using emacs -Q?
>>
>> > The easiest way I've found to fix this is simply changing the
>> > `package-generate-autoloads' function to always rebuild the autoloads
>> > file by passing the relevant option to `loaddefs-generate'.  I think
>> > this change also makes sense on a larger scale, as package generation
>> > taking into account older build artifacts seems unintuitive.
>>
>> I guess this could work, but it might also be an inconvenience for
>> people with a lot of packages?  It seems to me that trying to better
>> understand the issue before proceeding to apply this quick fix would be
>> a better idea.  What do you think?
>>
>> > The attached patch implements this change.
>> >
>> > I've read that for small changes like this no copyright assignment
>> > is needed.  If I have misunderstood, please point me to the relevant
>> > documents so I can sign them.  Please also let me know if I have
>> > messed something up or you need any additional information.
>>
>> That should be the case.
>>
>> >
>> > Best wishes
>> >
>> > Leo Gaskin
>> >
>> >
>> > In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
>> > version 1.16.0, Xaw3d scroll bars)
>> > Repository revision: 9848ae17161828190cc0ba31e89ae54a2f08a2ef
>> > Repository branch: master
>> > Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
>> > System Description: NixOS 23.05 (Stoat)
>> >
>> > Configured using:
>> >  'configure
>> >  --prefix=/nix/store/y9bxk3kqk4isr28jcy1bclkdr5a4zd1v-emacs-git-20230407.0
>> >  --disable-build-details --with-modules --with-x-toolkit=lucid
>> >  --with-xft --with-cairo --with-native-compilation'
>> >
>> >>From dad173580c048cbe89d5287c4475e965c71a702e Mon Sep 17 00:00:00 2001
>> > From: Leo Gaskin <leo.gaskin@le0.gs>
>> > Date: Sat, 8 Apr 2023 23:13:59 +0200
>> > Subject: [PATCH] Always fully rebuild autoloads in package-generate-autoloads
>> >
>> > ---
>> >  lisp/emacs-lisp/package.el | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> > index 685f983..09811f1 100644
>> > --- a/lisp/emacs-lisp/package.el
>> > +++ b/lisp/emacs-lisp/package.el
>> > @@ -1093,7 +1093,8 @@ untar into a directory named DIR; otherwise, signal an error."
>> >          ;; the load path.  We don't hard-code `pkg-dir', to avoid
>> >          ;; issues if the package directory is moved around.
>> >          (or (and load-file-name (file-name-directory load-file-name))
>> > -            (car load-path)))))
>> > +            (car load-path))))
>> > +     nil t)
>> >      (let ((buf (find-buffer-visiting output-file)))
>> >        (when buf (kill-buffer buf)))
>> >      auto-name))

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

[Philip Kaludercic]

Leo Georg Gaskin <leo.gaskin@le0.gs> writes:

> Tags: patch
>
> Hello!
>
> I've been using the new package-vc.el functionality to great effect,
> but I have also come across a somewhat annoying bug.  If I use any of
> the provided commands to rebuild my local package while a autoloads
> file is already present, the newly generated autoloads file is
> consistently either incomplete or empty.
>
> The easiest way I've found to fix this is simply changing the
> `package-generate-autoloads' function to always rebuild the autoloads
> file by passing the relevant option to `loaddefs-generate'.  I think
> this change also makes sense on a larger scale, as package generation
> taking into account older build artifacts seems unintuitive.
>
> The attached patch implements this change.
>
> I've read that for small changes like this no copyright assignment
> is needed.  If I have misunderstood, please point me to the relevant
> documents so I can sign them.  Please also let me know if I have
> messed something up or you need any additional information.

It seems to me that your fix is the easiest way to tackle the issue,
without having to do a deep-dive into loaddefs.  Eli, can this be
applied to emacs-29?  An alternative fix would be to restrict the
changes to package-vc, but that seems like an unnecessary duplication,
since the change doesn't affect package.

> Best wishes
>
> Leo Gaskin
>
>
> In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
> version 1.16.0, Xaw3d scroll bars)
> Repository revision: 9848ae17161828190cc0ba31e89ae54a2f08a2ef
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
> System Description: NixOS 23.05 (Stoat)
>
> Configured using:
>  'configure
>  --prefix=/nix/store/y9bxk3kqk4isr28jcy1bclkdr5a4zd1v-emacs-git-20230407.0
>  --disable-build-details --with-modules --with-x-toolkit=lucid
>  --with-xft --with-cairo --with-native-compilation'

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

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 62734@debbugs.gnu.org, "'Eli Zaretskii'" <eliz@gnu.org>
> Date: Fri, 28 Apr 2023 15:00:02 +0000
> 
> Leo Georg Gaskin <leo.gaskin@le0.gs> writes:
> 
> > Tags: patch
> >
> > Hello!
> >
> > I've been using the new package-vc.el functionality to great effect,
> > but I have also come across a somewhat annoying bug.  If I use any of
> > the provided commands to rebuild my local package while a autoloads
> > file is already present, the newly generated autoloads file is
> > consistently either incomplete or empty.
> >
> > The easiest way I've found to fix this is simply changing the
> > `package-generate-autoloads' function to always rebuild the autoloads
> > file by passing the relevant option to `loaddefs-generate'.  I think
> > this change also makes sense on a larger scale, as package generation
> > taking into account older build artifacts seems unintuitive.
> >
> > The attached patch implements this change.
> >
> > I've read that for small changes like this no copyright assignment
> > is needed.  If I have misunderstood, please point me to the relevant
> > documents so I can sign them.  Please also let me know if I have
> > messed something up or you need any additional information.
> 
> It seems to me that your fix is the easiest way to tackle the issue,
> without having to do a deep-dive into loaddefs.  Eli, can this be
> applied to emacs-29?  An alternative fix would be to restrict the
> changes to package-vc, but that seems like an unnecessary duplication,
> since the change doesn't affect package.

I don't understand the original problem (what does package-vc.el have
to do with rebuilding local packages? and why is a newly generated
loaddefs file incomplete or empty?), and I certainly don't think I
understand the effects of this change on the other usage scenarios.
Why would we want to unconditionally rebuild all the loaddefs files
every time package-generate-autoloads is invoked?  OTOH, that function
is not really documented, so maybe I don't understand what is it
supposed to do and in which conditions.

IOW, I think we need more details about the problem and the proposed
solution.

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: 62734@debbugs.gnu.org, "'Eli Zaretskii'" <eliz@gnu.org>
>> Date: Fri, 28 Apr 2023 15:00:02 +0000
>> 
>> Leo Georg Gaskin <leo.gaskin@le0.gs> writes:
>> 
>> > Tags: patch
>> >
>> > Hello!
>> >
>> > I've been using the new package-vc.el functionality to great effect,
>> > but I have also come across a somewhat annoying bug.  If I use any of
>> > the provided commands to rebuild my local package while a autoloads
>> > file is already present, the newly generated autoloads file is
>> > consistently either incomplete or empty.
>> >
>> > The easiest way I've found to fix this is simply changing the
>> > `package-generate-autoloads' function to always rebuild the autoloads
>> > file by passing the relevant option to `loaddefs-generate'.  I think
>> > this change also makes sense on a larger scale, as package generation
>> > taking into account older build artifacts seems unintuitive.
>> >
>> > The attached patch implements this change.
>> >
>> > I've read that for small changes like this no copyright assignment
>> > is needed.  If I have misunderstood, please point me to the relevant
>> > documents so I can sign them.  Please also let me know if I have
>> > messed something up or you need any additional information.
>> 
>> It seems to me that your fix is the easiest way to tackle the issue,
>> without having to do a deep-dive into loaddefs.  Eli, can this be
>> applied to emacs-29?  An alternative fix would be to restrict the
>> changes to package-vc, but that seems like an unnecessary duplication,
>> since the change doesn't affect package.
>
> I don't understand the original problem (what does package-vc.el have
> to do with rebuilding local packages? 

When package updates a package, it deletes the old code and downloads
the new stuff.  package-vc keeps the same code, but pulls the new
revisions, so it is necessary to re-generate the loaddef files for the
same files.

>                                       and why is a newly generated
> loaddefs file incomplete or empty?), and I certainly don't think I
> understand the effects of this change on the other usage scenarios.

From what I get, this is an issue in `loaddefs-generate'.  If we do not
force updating the file, and

--8<---------------cut here---------------start------------->8---
(time-less-p output-time
  (file-attribute-modification-time
  (file-attributes file)))
--8<---------------cut here---------------end--------------->8---

does not hold, then we do not collect any new definitions stored in
`defs'.  But since we do pass `extra-data' via the function
`package-generate-autoloads', that we evaluate this expression

--8<---------------cut here---------------start------------->8---
(with-temp-buffer
  (insert (loaddefs-generate--rubric output-file nil t))
  (search-forward "\f")
  (insert extra-data)
  (ensure-empty-lines 1)
  (write-region (point-min) (point-max) output-file nil 'silent))
--8<---------------cut here---------------end--------------->8---

that overwrites the entire file.

Thinking about this again, it might be possible to just fix this in
`loaddefs-generate', but trying to re-use an existing file

--8<---------------cut here---------------start------------->8---
(if (file-exists-p output-file)
    (insert-file-contents output-file)
  (insert (loaddefs-generate--rubric output-file nil t)))
--8<---------------cut here---------------end--------------->8---

but that assumes that `extra-data' (a string) would never contain a form
feed character...

Another idea is just to get rid of this faulty optimisation.  From my
tests this would also resolve the bug.

> Why would we want to unconditionally rebuild all the loaddefs files
> every time package-generate-autoloads is invoked?  OTOH, that function
> is not really documented, so maybe I don't understand what is it
> supposed to do and in which conditions.

The matter was that for regular packages, it was already rebuilt every
time `loaddefs-generate' was invoked, since there were never any old
loaddefs to update.

> IOW, I think we need more details about the problem and the proposed
> solution.

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

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
> Date: Fri, 28 Apr 2023 18:00:14 +0000
> 
> > I don't understand the original problem (what does package-vc.el have
> > to do with rebuilding local packages? 
> 
> When package updates a package, it deletes the old code and downloads
> the new stuff.  package-vc keeps the same code, but pulls the new
> revisions, so it is necessary to re-generate the loaddef files for the
> same files.

What is meant by "building the package"?  Is it just compiling the
Lisp files?

> >                                       and why is a newly generated
> > loaddefs file incomplete or empty?), and I certainly don't think I
> > understand the effects of this change on the other usage scenarios.
> 
> >From what I get, this is an issue in `loaddefs-generate'.  If we do not
> force updating the file, and
> 
> --8<---------------cut here---------------start------------->8---
> (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.

> 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.

> > Why would we want to unconditionally rebuild all the loaddefs files
> > every time package-generate-autoloads is invoked?  OTOH, that function
> > is not really documented, so maybe I don't understand what is it
> > supposed to do and in which conditions.
> 
> The matter was that for regular packages, it was already rebuilt every
> time `loaddefs-generate' was invoked, since there were never any old
> loaddefs to update.

This is only true as long as updating a package removes the previous
version entirely, including the generated files.  This is not
something I'd like us to assume in every corner of package.el, since
some day it can become false.

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:00:14 +0000
>> 
>> > I don't understand the original problem (what does package-vc.el have
>> > to do with rebuilding local packages? 
>> 
>> When package updates a package, it deletes the old code and downloads
>> the new stuff.  package-vc keeps the same code, but pulls the new
>> revisions, so it is necessary to re-generate the loaddef files for the
>> same files.
>
> 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.

>> >                                       and why is a newly generated
>> > loaddefs file incomplete or empty?), and I certainly don't think I
>> > understand the effects of this change on the other usage scenarios.
>> 
>> >From what I get, this is an issue in `loaddefs-generate'.  If we do not
>> force updating the file, and
>> 
>> --8<---------------cut here---------------start------------->8---
>> (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.

>> 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?  I am currently having difficulties
understanding how the `with-temp-buffer' code snippet I quoted above
/can/ do the right thing.  I am under the impression that wherever
`loaddefs-generate' is invoked, it avoids this execution path, either by
passing no EXTRA-DATA or by making sure that at least some autoload is
collected.

>> > Why would we want to unconditionally rebuild all the loaddefs files
>> > every time package-generate-autoloads is invoked?  OTOH, that function
>> > is not really documented, so maybe I don't understand what is it
>> > supposed to do and in which conditions.
>> 
>> The matter was that for regular packages, it was already rebuilt every
>> time `loaddefs-generate' was invoked, since there were never any old
>> loaddefs to update.
>
> This is only true as long as updating a package removes the previous
> version entirely, including the generated files.  This is not
> something I'd like us to assume in every corner of package.el, since
> some day it can become false.

Ok, fair point considering that package-vc currently does this.

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

[Eli Zaretskii]

> 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.

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

> >> 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.

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

[Philip Kaludercic]

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

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:


[-- Attachment #2: Type: text/plain, Size: 6814 bytes --]

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."

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

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
> Date: Sat, 29 Apr 2023 08:19:35 +0000
> 
> >>   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.

Compilation?  Setup?

"Building" is a strange term when we are talking about a Lisp package.

> > 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.

That is definitely a bug.  But a package without autoloads is still a
valid use case, and we should support it.

> 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:

What happens if a package has no autoloads?  The doc string says in
that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
Does your patch handle that?  (It's hard to tell, given all the
whitespace changes in the patch.)

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

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
>> Date: Sat, 29 Apr 2023 08:19:35 +0000
>> 
>> >>   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.
>
> Compilation?  Setup?

Scraping for auto-loads doesn't have anything to do with "compiling"
(and might be confused with byte compilation).  "Setup" might be
imaginable, I will think about it.

> "Building" is a strange term when we are talking about a Lisp package.

How come?  

>> > 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.
>
> That is definitely a bug.  But a package without autoloads is still a
> valid use case, and we should support it.
>
>> 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:
>
> What happens if a package has no autoloads?  The doc string says in
> that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
> Does your patch handle that?  (It's hard to tell, given all the
> whitespace changes in the patch.)

It would, as the else-case of the if branch I am proposing to eliminate
would still insert the EXTRA-DATA.

Here is a patch generated without any whitespace changes:


[-- Attachment #2: Type: text/plain, Size: 1303 bytes --]

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,15 +597,6 @@ 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)
-        (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)))
@@ -663,7 +654,7 @@ loaddefs-generate
             (write-region (point-min) (point-max) loaddefs-file nil 'silent)
             (byte-compile-info
              (file-relative-name loaddefs-file (car (ensure-list dir)))
-               t "GEN"))))))))
+             t "GEN")))))))
 
 (defun loaddefs-generate--print-form (def)
   "Print DEF in a format that makes sense for version control."

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

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
> Date: Sat, 29 Apr 2023 11:18:25 +0000
> 
> > "Building" is a strange term when we are talking about a Lisp package.
> 
> How come?  

There's nothing to "build".  Everything is already built.

> >> 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:
> >
> > What happens if a package has no autoloads?  The doc string says in
> > that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
> > Does your patch handle that?  (It's hard to tell, given all the
> > whitespace changes in the patch.)
> 
> It would, as the else-case of the if branch I am proposing to eliminate
> would still insert the EXTRA-DATA.

And if EXTRA-DATA is nil, then will we generate an empty OUTPUT file?

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: Sat, 29 Apr 2023 11:18:25 +0000
>> 
>> > "Building" is a strange term when we are talking about a Lisp package.
>> 
>> How come?  
>
> There's nothing to "build".  Everything is already built.

The idea is that this mirrors the building of a tarball on the ELPA
server, but as I said, I will think about this matter.

>> >> 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:
>> >
>> > What happens if a package has no autoloads?  The doc string says in
>> > that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
>> > Does your patch handle that?  (It's hard to tell, given all the
>> > whitespace changes in the patch.)
>> 
>> It would, as the else-case of the if branch I am proposing to eliminate
>> would still insert the EXTRA-DATA.
>
> And if EXTRA-DATA is nil, then will we generate an empty OUTPUT file?

No, we still generate the right file with the right information (in this
case just a `register-definition-prefixes' invocation).

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

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: leo.gaskin@le0.gs,  62734@debbugs.gnu.org
> Date: Sun, 30 Apr 2023 09:17:16 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > What happens if a package has no autoloads?  The doc string says in
> >> > that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
> >> > Does your patch handle that?  (It's hard to tell, given all the
> >> > whitespace changes in the patch.)
> >> 
> >> It would, as the else-case of the if branch I am proposing to eliminate
> >> would still insert the EXTRA-DATA.
> >
> > And if EXTRA-DATA is nil, then will we generate an empty OUTPUT file?
> 
> No, we still generate the right file with the right information (in this
> case just a `register-definition-prefixes' invocation).

OK, thanks.  Then please make sure this change survives both a
bootstrap and a "normal" build (where a bunch of *.el files need their
autoloads updated), and if so, please install on the emacs-29 branch.

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: Sun, 30 Apr 2023 09:17:16 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> > What happens if a package has no autoloads?  The doc string says in
>> >> > that case passing EXTRA-DATA will produce OUTPUT-FILE regardless.
>> >> > Does your patch handle that?  (It's hard to tell, given all the
>> >> > whitespace changes in the patch.)
>> >> 
>> >> It would, as the else-case of the if branch I am proposing to eliminate
>> >> would still insert the EXTRA-DATA.
>> >
>> > And if EXTRA-DATA is nil, then will we generate an empty OUTPUT file?
>> 
>> No, we still generate the right file with the right information (in this
>> case just a `register-definition-prefixes' invocation).
>
> OK, thanks.  Then please make sure this change survives both a
> bootstrap and a "normal" build (where a bunch of *.el files need their
> autoloads updated), and if so, please install on the emacs-29 branch.

Done, I have been testing it for a better part of the day and from what
I saw all behaved the way it should.

Thanks to Leo for noticing the problem in the first place, package-vc
also appears to be doing the right thing now.