Re: [PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 22, 2021, 21:16 UTC
Message-ID: <nycvar.QRO.7.76.6.2101221935230.52@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqft2tc2w7.fsf@gitster.c.googlers.com>

Hi Junio,

On Thu, 21 Jan 2021, Junio C Hamano wrote:

Show 17 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When `SKIP_DASHED_BUILT_INS` is specified in `config.mak`, the dashed
> > form of the built-ins was still generated.
> >
> > By moving the `SKIP_DASHED_BUILT_INS` handling after `config.mak` was
> > read, this can be avoided.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> OK.  So the problem is that the moved block that sets ALL_PROGRAMS,
> ALL_COMMANDS_TO_INSTALL, etc. depends on $(SKIP_DASHED_BUILT_INS),
> and that happens before we "include config.mak".

That matches my understanding, yes.

Show 12 quoted lines

> That makes sense.  Will apply (I do not know if you want this also
> on the maint tracks and if so which ones---I think it would matter
> if you want to cut a maint release from 2.29.x or 2.30.x tracks).
>
> By the way, I wonder if we can (semi-)automate looking for such a
> mistake in the future.  Does a simple rule like:
>
>     No variable that has "Define X if you want to distim the doshes"
>     at the beginning of the Makefile must be referenced before we
>     include config.mak
>
> work?

That would work, but we do not have a consistent format there. Exhibit A:

	# When using RUNTIME_PREFIX, define HAVE_BSD_KERN_PROC_SYSCTL if your platform
	# supports the KERN_PROC BSD sysctl function.

Therefore, automating this check would probably be a bit of a challenge. The best I could come up with (and which is still not complete) within few dozen minutes was this:

	terms=$(sed -ne '/^#$/{N;s/^#\n# [^ ].*\? \([A-Z][A-Z0-9_]\{4,\}\) .*/\1\\|/p}' -e '/GIT-VERSION-FILE/q' Makefile)
	sed -n "/^GIT-VERSION-FILE:/,/^-include config.mak/s/\\($(echo "$terms" | tr -d '\012')NO-MATCH\\)/&/p" Makefile

The only thing that sticks out in this output is that we use SHELL_PATH a couple times before including config.mak.

And I don't think that this hack of mine can be converted into a robust check that we'd want to run to verify that the Makefile does not use constants before they are potentially defined in config.mak, unfortunately.

Ciao, Dscho