Re: VCSWITNESS = fail ** 2

[Eli Zaretskii <eliz@gnu.org>]

> From: esr@thyrsus.com (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).