SKIP_DASHED_BUILT_INS: respect config.mak

[dscho]

I stumbled over this the other day. I thought I had tested this a long time ago, but apparently it never worked in the way I want it to work.

[dscho]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-840/dscho/SKIP_DASHED_BUILT_INS-and-config.mak-v1

To fetch this version to local tag pr-840/dscho/SKIP_DASHED_BUILT_INS-and-config.mak-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-840/dscho/SKIP_DASHED_BUILT_INS-and-config.mak-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> 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 <[email protected]>
> ---

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 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?

Thanks.

[gitgitgadget]

This branch is now known as js/skip-dashed-built-ins-from-config-mak.

[gitgitgadget]

This patch series was integrated into seen via git@7e829da.

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > 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 <[email protected]>
> > ---
>
> 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.

> 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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

>> 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?
> ...
> 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.

Oh, I wasn't expecting all the work be done by you in your busy
schedule ;-) The primary thing I was looking for was to sanity check
the idea of the general rule.  Implementation of it can start as
something the reviewers would keep in their heads.  A script with
false positives that authors can use to be reminded may come next.

We do not have to jump to the perfection from day one, in other
words.

[gitgitgadget]

This patch series was integrated into seen via git@9230bf7.

[gitgitgadget]

This patch series was integrated into seen via git@17c3e3d.

[gitgitgadget]

This patch series was integrated into seen via git@f7a85c3.

[gitgitgadget]

This patch series was integrated into seen via git@85dbca7.

[gitgitgadget]

This patch series was integrated into seen via git@c8d0f8a.

[gitgitgadget]

This patch series was integrated into seen via git@0c1e2d6.

[gitgitgadget]

This patch series was integrated into seen via git@d3d2142.

[gitgitgadget]

This patch series was integrated into next via git@2d727a3.

[gitgitgadget]

This patch series was integrated into seen via git@f8c9bd7.

[gitgitgadget]

This patch series was integrated into seen via git@ef0a0a8.

[gitgitgadget]

This patch series was integrated into seen via git@2d727a3.

[gitgitgadget]

This patch series was integrated into seen via git@d79169c.

[gitgitgadget]

This patch series was integrated into seen via git@6cd7f9d.

[gitgitgadget]

This patch series was integrated into next via git@6cd7f9d.

[gitgitgadget]

This patch series was integrated into master via git@6cd7f9d.

[gitgitgadget]

Closed via 6cd7f9d.