SKIP_DASHED_BUILT_INS: respect `config.mak`

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.

---
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(-)

index 7b64106930a..0c7437e08c5 100644
@@ -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

"Johannes Schindelin via GitGitGadget" <[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.

Hi Junio,

That matches my understanding, yes.

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

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.