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

2 messages, 2 authors, 2021-01-22 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-01-21 23:00:47

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
From: Johannes Schindelin <redacted>

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

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

From: Johannes Schindelin <hidden>
Date: 2021-01-22 21:20:09

Hi Junio,

On Thu, 21 Jan 2021, Junio C Hamano wrote:
"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
From: Johannes Schindelin <redacted>

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 <redacted>
---
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help