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.