| Message ID | pull.840.git.1611234585889.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a5ec7d166369bb537d31e2920651d40538511b3 |
| Headers | show |
| Series | SKIP_DASHED_BUILT_INS: respect `config.mak` | expand |
"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.
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
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.
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)