[PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`

[PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`

[Johannes Schindelin via GitGitGadget]

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>
---
    SKIP_DASHED_BUILT_INS: respect config.mak
    
    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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-840%2Fdscho%2FSKIP_DASHED_BUILT_INS-and-config.mak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-840/dscho/SKIP_DASHED_BUILT_INS-and-config.mak-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/840

 Makefile | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 7b64106930a..0c7437e08c5 100644
--- a/Makefile
+++ b/Makefile
@@ -777,20 +777,6 @@ BUILT_INS += git-status$X
 BUILT_INS += git-switch$X
 BUILT_INS += git-whatchanged$X
 
-# what 'all' will build and 'install' will install in gitexecdir,
-# excluding programs for built-in commands
-ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
-ALL_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
-ifeq (,$(SKIP_DASHED_BUILT_INS))
-ALL_COMMANDS_TO_INSTALL += $(BUILT_INS)
-else
-# git-upload-pack, git-receive-pack and git-upload-archive are special: they
-# are _expected_ to be present in the `bin/` directory in their dashed form.
-ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)
-ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
-ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
-endif
-
 # what 'all' will build but not install in gitexecdir
 OTHER_PROGRAMS = git$X
 
@@ -1226,6 +1212,20 @@ ifdef DEVELOPER
 include config.mak.dev
 endif
 
+# what 'all' will build and 'install' will install in gitexecdir,
+# excluding programs for built-in commands
+ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
+ALL_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
+ifeq (,$(SKIP_DASHED_BUILT_INS))
+ALL_COMMANDS_TO_INSTALL += $(BUILT_INS)
+else
+# git-upload-pack, git-receive-pack and git-upload-archive are special: they
+# are _expected_ to be present in the `bin/` directory in their dashed form.
+ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
+endif
+
 ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
-- 
gitgitgadget

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

[Junio C Hamano]

"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 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`

[Johannes Schindelin]

Hi Junio,

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

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

> 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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.