Re: VCSWITNESS = fail ** 2

[Eli Zaretskii]

> From: address@hidden (Eric S. Raymond)
> Date: Sun, 12 Jan 2014 09:37:33 -0500 (EST)
> 
> Nobody has stepped up to own or defend this code in Makefile.in:

You didn't wait nowhere near enough time to draw that conclusion.
Some of us live in different time zones than you do.  Moreover, for
some of us today is a weekday, not a weekend, and I, for instance,
cannot reply to my gnu.org email during office hours, for boring
reasons.

Please always wait for at least a day, if not longer, before you
conclude that no one responded.

> src: Makefile FRC
>       dirstate='.bzr/checkout/dirstate';                              \
>       vcswitness='$$(srcdir)/../'$$dirstate;                          \
>       [ -r "$(srcdir)/$$dirstate" ] || vcswitness='';                 \
>       cd $@ || exit;                                                  \
>       boot=bootstrap-emacs$(EXEEXT);                                  \
>       [ ! -x "$$boot" ] || boot='';                                   \
>       $(MAKE) all $(MFLAGS)                                           \
>         CC='${CC}' CFLAGS='${CFLAGS}' CPPFLAGS='${CPPFLAGS}'          \
>         LDFLAGS='${LDFLAGS}' MAKE='${MAKE}' BOOTSTRAPEMACS="$$boot"   \
>         VCSWITNESS="$$vcswitness"
> 
> Which is just as well, because the second through fourth lines
> 
>       dirstate='.bzr/checkout/dirstate';                              \
>       vcswitness='$$(srcdir)/../'$$dirstate;                          \
>       [ -r "$(srcdir)/$$dirstate" ] || vcswitness='';                 \
> 
> are a case study in how not to do things.  First, they're broken.  The
> readability test on line 4 is not checking the same file path that is
> set on line 3.

AFAICS, that's because the test is done first from $(srcdir), and then
from $(srcdir)/src.  Thus, the value passed to src/Makefile needs an
extra "..".

> The contingent result with the current repo tree structure
> is that vcswitness is never set.

Isn't the problem in this line:

          VCSWITNESS="$$vcswitness"

Shouldn't that be

          VCSWITNESS="$vcswitness"

instead?

> Now let's look at what VCSWITNESS is used for.  The key lines are
> in src/Makefile.in:
> 
> ## VCSWITNESS points to the file that holds info about the current checkout.
> ## We use it as a heuristic to decide when to rebuild loaddefs.el.
> ## If empty it is ignored; the parent makefile can set it to some other value.
> VCSWITNESS =
> 
> $(lispsource)/loaddefs.el: $(BOOTSTRAPEMACS) $(VCSWITNESS)
>       cd ../lisp; $(MAKE) $(MFLAGS) autoloads EMACS="$(bootstrap_exe)"
> 
> This test is wrong.  The correct prerequisites would be every elisp
> file that declares an autoload cookie.

That possibility was considered and rejected, for the reasons that
Stefan explained.  So you have just made things worse, not better.

> I'm going to delete the broken code.

I'm going to revert that change, unless I hear some objections.  Then
we can discuss how to fix this (assuming it is broken, which I'm not
sure it is yet).