threads / patch / 55023

patchSKIP_DASHED_BUILT_INS: respect `config.mak`

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

## tl;dr

4 messages between Jan 21, 2021 and Jan 22, 2021. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Johannes Schindelin via GitGitGadget· Jan 21, 2021, 13:09 UTC · lore
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(-)
Show changes to Makefile +14 −14
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
Junio C Hamano· Jan 21, 2021, 22:59 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines
> 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.
Johannes Schindelin· Jan 22, 2021, 21:16 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Jan 22, 2021, 21:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 16 quoted lines
>> 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.

← back to recent threads