{"thread":{"id":"55023","subject":"[PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`","startedAt":"2021-01-21T13:15:19Z","lastAt":"2021-01-22T21:52:16Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"414899","messageId":"pull.840.git.1611234585889.gitgitgadget@gmail.com","threadId":"55023","inReplyTo":null,"subject":"[PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-21T13:09:45Z","receivedAt":"2021-01-21T13:15:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen `SKIP_DASHED_BUILT_INS` is specified in `config.mak`, the dashed\nform of the built-ins was still generated.\n\nBy moving the `SKIP_DASHED_BUILT_INS` handling after `config.mak` was\nread, this can be avoided.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    SKIP_DASHED_BUILT_INS: respect config.mak\n    \n    I stumbled over this the other day. I thought I had tested this a long\n    time ago, but apparently it never worked in the way I want it to work.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-840%2Fdscho%2FSKIP_DASHED_BUILT_INS-and-config.mak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-840/dscho/SKIP_DASHED_BUILT_INS-and-config.mak-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/840\n\n Makefile | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 7b64106930a..0c7437e08c5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -777,20 +777,6 @@ BUILT_INS += git-status$X\n BUILT_INS += git-switch$X\n BUILT_INS += git-whatchanged$X\n \n-# what 'all' will build and 'install' will install in gitexecdir,\n-# excluding programs for built-in commands\n-ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)\n-ALL_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)\n-ifeq (,$(SKIP_DASHED_BUILT_INS))\n-ALL_COMMANDS_TO_INSTALL += $(BUILT_INS)\n-else\n-# git-upload-pack, git-receive-pack and git-upload-archive are special: they\n-# are _expected_ to be present in the `bin/` directory in their dashed form.\n-ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)\n-ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n-ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n-endif\n-\n # what 'all' will build but not install in gitexecdir\n OTHER_PROGRAMS = git$X\n \n@@ -1226,6 +1212,20 @@ ifdef DEVELOPER\n include config.mak.dev\n endif\n \n+# what 'all' will build and 'install' will install in gitexecdir,\n+# excluding programs for built-in commands\n+ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)\n+ALL_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)\n+ifeq (,$(SKIP_DASHED_BUILT_INS))\n+ALL_COMMANDS_TO_INSTALL += $(BUILT_INS)\n+else\n+# git-upload-pack, git-receive-pack and git-upload-archive are special: they\n+# are _expected_ to be present in the `bin/` directory in their dashed form.\n+ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)\n+ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n+ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n+endif\n+\n ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n \n\nbase-commit: 71ca53e8125e36efbda17293c50027d31681a41f\n-- \ngitgitgadget\n"},{"id":"414935","messageId":"xmqqft2tc2w7.fsf@gitster.c.googlers.com","threadId":"55023","inReplyTo":"pull.840.git.1611234585889.gitgitgadget@gmail.com","subject":"Re: [PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-21T22:59:36Z","receivedAt":"2021-01-21T23:00:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> When `SKIP_DASHED_BUILT_INS` is specified in `config.mak`, the dashed\n> form of the built-ins was still generated.\n>\n> By moving the `SKIP_DASHED_BUILT_INS` handling after `config.mak` was\n> read, this can be avoided.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n\nOK.  So the problem is that the moved block that sets ALL_PROGRAMS,\nALL_COMMANDS_TO_INSTALL, etc. depends on $(SKIP_DASHED_BUILT_INS),\nand that happens before we \"include config.mak\".\n\nThat makes sense.  Will apply (I do not know if you want this also\non the maint tracks and if so which ones---I think it would matter\nif you want to cut a maint release from 2.29.x or 2.30.x tracks).\n\nBy the way, I wonder if we can (semi-)automate looking for such a\nmistake in the future.  Does a simple rule like:\n\n    No variable that has \"Define X if you want to distim the doshes\"\n    at the beginning of the Makefile must be referenced before we\n    include config.mak\n\nwork?\n\nThanks.\n"},{"id":"415000","messageId":"nycvar.QRO.7.76.6.2101221935230.52@tvgsbejvaqbjf.bet","threadId":"55023","inReplyTo":"xmqqft2tc2w7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-22T21:16:52Z","receivedAt":"2021-01-22T21:20:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 21 Jan 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > When `SKIP_DASHED_BUILT_INS` is specified in `config.mak`, the dashed\n> > form of the built-ins was still generated.\n> >\n> > By moving the `SKIP_DASHED_BUILT_INS` handling after `config.mak` was\n> > read, this can be avoided.\n> >\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n>\n> OK.  So the problem is that the moved block that sets ALL_PROGRAMS,\n> ALL_COMMANDS_TO_INSTALL, etc. depends on $(SKIP_DASHED_BUILT_INS),\n> and that happens before we \"include config.mak\".\n\nThat matches my understanding, yes.\n\n> That makes sense.  Will apply (I do not know if you want this also\n> on the maint tracks and if so which ones---I think it would matter\n> if you want to cut a maint release from 2.29.x or 2.30.x tracks).\n>\n> By the way, I wonder if we can (semi-)automate looking for such a\n> mistake in the future.  Does a simple rule like:\n>\n>     No variable that has \"Define X if you want to distim the doshes\"\n>     at the beginning of the Makefile must be referenced before we\n>     include config.mak\n>\n> work?\n\nThat would work, but we do not have a consistent format there. Exhibit A:\n\n\t# When using RUNTIME_PREFIX, define HAVE_BSD_KERN_PROC_SYSCTL if your platform\n\t# supports the KERN_PROC BSD sysctl function.\n\nTherefore, automating this check would probably be a bit of a challenge.\nThe best I could come up with (and which is still not complete) within few\ndozen minutes was this:\n\n\tterms=$(sed -ne '/^#$/{N;s/^#\\n# [^ ].*\\? \\([A-Z][A-Z0-9_]\\{4,\\}\\) .*/\\1\\\\|/p}' -e '/GIT-VERSION-FILE/q' Makefile)\n\tsed -n \"/^GIT-VERSION-FILE:/,/^-include config.mak/s/\\\\($(echo \"$terms\" | tr -d '\\012')NO-MATCH\\\\)/&/p\" Makefile\n\nThe only thing that sticks out in this output is that we use SHELL_PATH a\ncouple times before including config.mak.\n\nAnd I don't think that this hack of mine can be converted into a robust\ncheck that we'd want to run to verify that the Makefile does not use\nconstants before they are potentially defined in config.mak,\nunfortunately.\n\nCiao,\nDscho\n"},{"id":"415005","messageId":"xmqqft2s7iac.fsf@gitster.c.googlers.com","threadId":"55023","inReplyTo":"nycvar.QRO.7.76.6.2101221935230.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] SKIP_DASHED_BUILT_INS: respect `config.mak`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T21:50:35Z","receivedAt":"2021-01-22T21:52:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> By the way, I wonder if we can (semi-)automate looking for such a\n>> mistake in the future.  Does a simple rule like:\n>>\n>>     No variable that has \"Define X if you want to distim the doshes\"\n>>     at the beginning of the Makefile must be referenced before we\n>>     include config.mak\n>>\n>> work?\n> ...\n> The only thing that sticks out in this output is that we use SHELL_PATH a\n> couple times before including config.mak.\n> ...\n> And I don't think that this hack of mine can be converted into a robust\n> check that we'd want to run to verify that the Makefile does not use\n> constants before they are potentially defined in config.mak,\n> unfortunately.\n\nOh, I wasn't expecting all the work be done by you in your busy\nschedule ;-) The primary thing I was looking for was to sanity check\nthe idea of the general rule.  Implementation of it can start as\nsomething the reviewers would keep in their heads.  A script with\nfalse positives that authors can use to be reminded may come next.\n\nWe do not have to jump to the perfection from day one, in other\nwords.\n"}]}