bug#62099: 29.0.60; Intentional change to *Help* source code links?

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: 62099@debbugs.gnu.org,  larsi@gnus.org
> Date: Mon, 15 May 2023 01:11:05 +0200
> 
> On Sun, 14 May 2023 20:41:20 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
> 
> >> 43b0210f83c only changed how loaddefs.el is generated, and for
> >> out-of-tree builds, also how the names of the files under lisp are
> >> represented in loaddefs.el.  Prior to the change, in out-of-tree builds
> >> these file names appeared the same as those in in-tree builds, e.g.:
> >>
> >> ;;; Generated autoloads from play/5x5.el
> >>
> >> But after 43b0210f83c, in my out-of-tree builds, they now look like
> >> this:
> >>
> >> ;;; Generated autoloads from
> >> ../../../../../../home/steve/src/emacs/test-29/lisp/play/5x5.el
> >
> > Isn't the above difference the root cause, right there?  Why should
> > the output in loaddefs.el depend on whether this is an in-tree or
> > out-of-tree build?  Eventually, when the produced loaddefs.el is
> > installed, the *.el files from the tree and the *.elc files from both
> > the tree and out-of-tree will end up in the same directory, so there
> > should be no ../../../..  etc. in the file names recorded in
> > loaddefs.el, they should be all relative to <source-directory>/lisp,
> > no?
> 
> I'm not sure I understand: there are no *.elc files in the out-of-tree
> build directory, all byte-compiled files reside in the source tree
> together with the *.el files.  Also, FWIW I do not install my Emacs
> builds and haven't tested whether the problematic file names appear in
> an installed build.

These aspects don't matter.  My point is that the format of file names
in loaddefs.el should not depend on whether it is an in-tree or
out-of-tree build.  If you don't agree, please explain why.

> >      It seems like loaddefs-gen.el does this instead:
> >
> >             (let* ((relfile (file-relative-name
> >                              (cadar section)
> >                              (file-name-directory loaddefs-file)))
> >                    (head (concat "\n\f\n;;; Generated autoloads from "
> >                                  relfile "\n\n")))
> >
> > which uses the wrong directory as the second arg of
> > file-relative-name.
> >
> > Am I missing something?
> 
> Well, for one thing this part of the code was not touched by
> 43b0210f83c.

I don't think this matters, either.  If the same code produces two
different results in these two revisions, then either the first or the
second argument (or both) to file-relative-name were modified by
commit 43b0210f83c.  I think we should find out which change caused
that, and then try to fix that so that the results are identical.

> Also, while I haven't found a way to step through
> loaddefs-generate while loaddefs.el is being generated, since that's
> part of the build process, note that evaluating the following sexp:
> 
> (file-relative-name "/datadisk/steve/src/emacs/test-29/lisp/play/5x5.el" 
> "~/build/test-29/lisp/play")
> 
> produces the same number of "../" as appear in loaddefs.el:
> 
> "../../../../../../datadisk/steve/src/emacs/test-29/lisp/play/5x5.el"

Why is that important?  Wed don't suspect a bug in file-relative-name,
do we?

Again, my point is that the second argument to file-relative-name
should not be the build directory, it should be the source directory.

> The difference is that loaddefs.el has "home" instead of "datadisk".
> But on my system, ~/src is a symlink to /datadisk/steve/src.  Perhaps
> this an irrelevant coincidence, but I found it striking in this context.

Why striking?  And why do you think it's important for fixing this
problem?

> > Or maybe step through the code in autoloads.el and see how it succeeds
> > in computing the correct relative file name even in out-of-tree
> > builds, and let's take it from there.
> 
> Do you mean autoload.el?  But this file was obsoleted in commit
> 22a2ad13f50 not long after commit 43b0210f83c and I believe
> loaddefs-gen.el is meant to be its replacement.

Sure, but it did the job, didn't it?

You could as an alternative step through loaddefs-gen.el before commit
43b0210f83c, and see what happened there.  The idea is to see what
should be the correct value of relfile and why doesn't the current
code yield that.

> > However, I think we should concentrate on the first place with the
> > deviation, which I think is loaddefs.el.
> 
> I'm not sure: it seems to me those file names with "../../../../../../"
> are intended, in order, as the commit message of 43b0210f83c says, to
> "Fix out-of-tree build problems with loaddefs.el".

What are those problems?  And why does using "../../.." fix them?