{"thread":{"id":"49133","subject":"[PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","startedAt":"2018-08-15T15:15:20Z","lastAt":"2018-08-17T06:43:04Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"355689","messageId":"20180815151505.12011-1-avarab@gmail.com","threadId":"49133","inReplyTo":null,"subject":"[PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-15T15:15:05Z","receivedAt":"2018-08-15T15:15:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the NO_TCLTK=NoThanks flag to be understood by the\nDocumentation Makefile.\n\nBefore this change compiling and installing with NO_TCLTK would result\nin no git-gui, gitk or git-citool being installed, but their\nrespective manual pages would still be installed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/Makefile | 23 ++++++++++++++++++-----\n Makefile               | 39 +++++++++++++++++++++------------------\n 2 files changed, 39 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex d079d7c73a..d53979939e 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -1,5 +1,7 @@\n # Guard against environment variables\n MAN1_TXT =\n+MAN1_TXT_WIP =\n+TCLTK_FILES =\n MAN5_TXT =\n MAN7_TXT =\n TECH_DOCS =\n@@ -7,13 +9,24 @@ ARTICLES =\n SP_ARTICLES =\n OBSOLETE_HTML =\n \n-MAN1_TXT += $(filter-out \\\n+MAN1_TXT_WIP += $(filter-out \\\n \t\t$(addsuffix .txt, $(ARTICLES) $(SP_ARTICLES)), \\\n \t\t$(wildcard git-*.txt))\n-MAN1_TXT += git.txt\n-MAN1_TXT += gitk.txt\n-MAN1_TXT += gitremote-helpers.txt\n-MAN1_TXT += gitweb.txt\n+MAN1_TXT_WIP += git.txt\n+MAN1_TXT_WIP += gitremote-helpers.txt\n+MAN1_TXT_WIP += gitweb.txt\n+\n+ifndef NO_TCLTK\n+MAN1_TXT_WIP += gitk.txt\n+MAN1_TXT = $(MAN1_TXT_WIP)\n+else\n+TCLTK_FILES += git-gui.txt\n+TCLTK_FILES += gitk.txt\n+TCLTK_FILES += git-citool.txt\n+MAN1_TXT = $(filter-out \\\n+\t\t$(TCLTK_FILES), \\\n+\t\t$(MAN1_TXT_WIP))\n+endif\n \n MAN5_TXT += gitattributes.txt\n MAN5_TXT += githooks.txt\ndiff --git a/Makefile b/Makefile\nindex bc4fc8eeab..8abb23f6ce 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2372,21 +2372,21 @@ export DEFAULT_EDITOR DEFAULT_PAGER\n \n .PHONY: doc man man-perl html info pdf\n doc: man-perl\n-\t$(MAKE) -C Documentation all\n+\t$(MAKE) -C Documentation all NO_TCLTK='$(NO_TCLTK)'\n \n man: man-perl\n-\t$(MAKE) -C Documentation man\n+\t$(MAKE) -C Documentation man NO_TCLTK='$(NO_TCLTK)'\n \n man-perl: perl/build/man/man3/Git.3pm\n \n html:\n-\t$(MAKE) -C Documentation html\n+\t$(MAKE) -C Documentation html NO_TCLTK='$(NO_TCLTK)'\n \n info:\n-\t$(MAKE) -C Documentation info\n+\t$(MAKE) -C Documentation info NO_TCLTK='$(NO_TCLTK)'\n \n pdf:\n-\t$(MAKE) -C Documentation pdf\n+\t$(MAKE) -C Documentation pdf NO_TCLTK='$(NO_TCLTK)'\n \n XGETTEXT_FLAGS = \\\n \t--force-po \\\n@@ -2802,10 +2802,10 @@ install-gitweb:\n \t$(MAKE) -C gitweb install\n \n install-doc: install-man-perl\n-\t$(MAKE) -C Documentation install\n+\t$(MAKE) -C Documentation install NO_TCLTK='$(NO_TCLTK)'\n \n install-man: install-man-perl\n-\t$(MAKE) -C Documentation install-man\n+\t$(MAKE) -C Documentation install-man NO_TCLTK='$(NO_TCLTK)'\n \n install-man-perl: man-perl\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3'\n@@ -2813,22 +2813,22 @@ install-man-perl: man-perl\n \t(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)\n \n install-html:\n-\t$(MAKE) -C Documentation install-html\n+\t$(MAKE) -C Documentation install-html NO_TCLTK='$(NO_TCLTK)'\n \n install-info:\n-\t$(MAKE) -C Documentation install-info\n+\t$(MAKE) -C Documentation install-info NO_TCLTK='$(NO_TCLTK)'\n \n install-pdf:\n-\t$(MAKE) -C Documentation install-pdf\n+\t$(MAKE) -C Documentation install-pdf NO_TCLTK='$(NO_TCLTK)'\n \n quick-install-doc:\n-\t$(MAKE) -C Documentation quick-install\n+\t$(MAKE) -C Documentation quick-install NO_TCLTK='$(NO_TCLTK)'\n \n quick-install-man:\n-\t$(MAKE) -C Documentation quick-install-man\n+\t$(MAKE) -C Documentation quick-install-man NO_TCLTK='$(NO_TCLTK)'\n \n quick-install-html:\n-\t$(MAKE) -C Documentation quick-install-html\n+\t$(MAKE) -C Documentation quick-install-html NO_TCLTK='$(NO_TCLTK)'\n \n \n \n@@ -2875,13 +2875,16 @@ manpages = git-manpages-$(GIT_VERSION)\n dist-doc:\n \t$(RM) -r .doc-tmp-dir\n \tmkdir .doc-tmp-dir\n-\t$(MAKE) -C Documentation WEBDOC_DEST=../.doc-tmp-dir install-webdoc\n+\t$(MAKE) -C Documentation NO_TCLTK='$(NO_TCLTK)' \\\n+\t\tWEBDOC_DEST=../.doc-tmp-dir install-webdoc\n \tcd .doc-tmp-dir && $(TAR) cf ../$(htmldocs).tar .\n \tgzip -n -9 -f $(htmldocs).tar\n \t:\n \t$(RM) -r .doc-tmp-dir\n \tmkdir -p .doc-tmp-dir/man1 .doc-tmp-dir/man5 .doc-tmp-dir/man7\n-\t$(MAKE) -C Documentation DESTDIR=./ \\\n+\t$(MAKE) -C Documentation \\\n+\t\tNO_TCLTK='$(NO_TCLTK)' \\\n+\t\tDESTDIR=./ \\\n \t\tman1dir=../.doc-tmp-dir/man1 \\\n \t\tman5dir=../.doc-tmp-dir/man5 \\\n \t\tman7dir=../.doc-tmp-dir/man7 \\\n@@ -2915,7 +2918,7 @@ clean: profile-clean coverage-clean\n \t$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz\n \t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n \t$(RM) contrib/coccinelle/*.cocci.patch*\n-\t$(MAKE) -C Documentation/ clean\n+\t$(MAKE) -C Documentation/ clean NO_TCLTK='$(NO_TCLTK)'\n ifndef NO_PERL\n \t$(MAKE) -C gitweb clean\n \t$(RM) -r perl/build/\n@@ -2944,7 +2947,7 @@ ALL_COMMANDS += git-gui git-citool\n \n .PHONY: check-docs\n check-docs::\n-\t$(MAKE) -C Documentation lint-docs\n+\t$(MAKE) -C Documentation lint-docs NO_TCLTK='$(NO_TCLTK)'\n \t@(for v in $(ALL_COMMANDS); \\\n \tdo \\\n \t\tcase \"$$v\" in \\\n@@ -2968,7 +2971,7 @@ check-docs::\n \t\t    -e '/^#/d' \\\n \t\t    -e 's/[ \t].*//' \\\n \t\t    -e 's/^/listed /' command-list.txt; \\\n-\t\t$(MAKE) -C Documentation print-man1 | \\\n+\t\t$(MAKE) -C Documentation print-man1  NO_TCLTK='$(NO_TCLTK)' | \\\n \t\tgrep '\\.txt$$' | \\\n \t\tsed -e 's|Documentation/|documented |' \\\n \t\t    -e 's/\\.txt//'; \\\n-- \n2.18.0.865.gffc8e1a3cd6\n\n"},{"id":"355707","messageId":"xmqq7ekrk1q0.fsf@gitster-ct.c.googlers.com","threadId":"49133","inReplyTo":"20180815151505.12011-1-avarab@gmail.com","subject":"Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-15T16:56:39Z","receivedAt":"2018-08-15T16:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Extend the NO_TCLTK=NoThanks flag to be understood by the\n> Documentation Makefile.\n>\n> Before this change compiling and installing with NO_TCLTK would result\n> in no git-gui, gitk or git-citool being installed, but their\n> respective manual pages would still be installed.\n\nThanks, but I personally do not perceive it to be a problem.\n\nIt is perfectly OK to install programs without installing docs, and\nalso it is OK to install docs without programs.  I do not see why\ngitk.html and the reference to it from the main ToC in git.html must\nbe removed when you are not installing gitk.\n\nLack of an option to skip them from the documentation is something\nwe might want to improve, but you should be able to install the docs\nfor programs you do not happen to have, and I think it should be the\ndefault.\n\nBy the way, to be more explicit than merely to hint, I think this\npatch needs to also update Documentation/cmd-list.perl so that under\nNO_TCLTK, the entry for gitk is omitted from cmds-mainporcelain.txt,\netc. to be a complete solution to your original problem.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  Documentation/Makefile | 23 ++++++++++++++++++-----\n>  Makefile               | 39 +++++++++++++++++++++------------------\n>  2 files changed, 39 insertions(+), 23 deletions(-)\n>\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index d079d7c73a..d53979939e 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -1,5 +1,7 @@\n>  # Guard against environment variables\n>  MAN1_TXT =\n> +MAN1_TXT_WIP =\n> +TCLTK_FILES =\n\nThe latter name loses the fact that it is to hold candidates to be\non MAN1_TXT that happen to be conditionally included; calling it\nMAN1_TXT_TCLTK or something, perhaps, may be an improvement.\n\nThe former name makes it look it is work-in-progress, but in fact\nthey are definite and unconditional part of MAN1_TXT.  Perhaps\nMAN1_TXT_CORE or something?\n\n> ...\n> +MAN1_TXT_WIP += git.txt\n> +MAN1_TXT_WIP += gitremote-helpers.txt\n> +MAN1_TXT_WIP += gitweb.txt\n> +\n> +ifndef NO_TCLTK\n> +MAN1_TXT_WIP += gitk.txt\n> +MAN1_TXT = $(MAN1_TXT_WIP)\n> +else\n> +TCLTK_FILES += git-gui.txt\n> +TCLTK_FILES += gitk.txt\n> +TCLTK_FILES += git-citool.txt\n> +MAN1_TXT = $(filter-out \\\n> +\t\t$(TCLTK_FILES), \\\n> +\t\t$(MAN1_TXT_WIP))\n> +endif\n>  \n>  MAN5_TXT += gitattributes.txt\n>  MAN5_TXT += githooks.txt\n> diff --git a/Makefile b/Makefile\n> index bc4fc8eeab..8abb23f6ce 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2372,21 +2372,21 @@ export DEFAULT_EDITOR DEFAULT_PAGER\n>  \n>  .PHONY: doc man man-perl html info pdf\n>  doc: man-perl\n> -\t$(MAKE) -C Documentation all\n> +\t$(MAKE) -C Documentation all NO_TCLTK='$(NO_TCLTK)'\n> ...\n>  pdf:\n> -\t$(MAKE) -C Documentation pdf\n> +\t$(MAKE) -C Documentation pdf NO_TCLTK='$(NO_TCLTK)'\n\nSince we are assuming GNU make anyway, can we just say \"export\nNO_TCLTK\" just once, or would it be too magical and create\nmaintenance burden?\n"},{"id":"355711","messageId":"xmqq1sazk12m.fsf@gitster-ct.c.googlers.com","threadId":"49133","inReplyTo":"xmqq7ekrk1q0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-15T17:10:41Z","receivedAt":"2018-08-15T17:10:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>  # Guard against environment variables\n>>  MAN1_TXT =\n>> +MAN1_TXT_WIP =\n>> +TCLTK_FILES =\n>\n> The latter name loses the fact that it is to hold candidates to be\n> on MAN1_TXT that happen to be conditionally included; calling it\n> MAN1_TXT_TCLTK or something, perhaps, may be an improvement.\n>\n> The former name makes it look it is work-in-progress, but in fact\n> they are definite and unconditional part of MAN1_TXT.  Perhaps\n> MAN1_TXT_CORE or something?\n\nSorry, I misread the patch.  You collect all possible MAN1_TXT\ncandidates on _WIP, so \"this is unconditional core part\" is wrong.\nWork-in-progress still sounds a bit funny, but now I know what is\ngoing on a bit better, it has become at last understandable ;-)\n\n>> +ifndef NO_TCLTK\n>> +MAN1_TXT_WIP += gitk.txt\n>> +MAN1_TXT = $(MAN1_TXT_WIP)\n>> +else\n>> +TCLTK_FILES += git-gui.txt\n>> +TCLTK_FILES += gitk.txt\n>> +TCLTK_FILES += git-citool.txt\n>> +MAN1_TXT = $(filter-out \\\n>> +\t\t$(TCLTK_FILES), \\\n>> +\t\t$(MAN1_TXT_WIP))\n>> +endif\n\nI didn't notice it when I read it for the first time, but asymmetry\nbetween these two looks somewhat strange.  If we are adding gitk.txt\nwhen we are not declining TCLTK based programs, why can we do\nwithout adding git-gui and git-citool at the same time?  If we know\nwe must add gitk.txt when we are not declining TCLTK based programs\nto MAN1_TXT_WIP in this section, it must mean that when we do not\nwant TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on\nit, so why do we even need it on TCLTK_FILES list to filter it out?\n"},{"id":"355730","messageId":"87lg97qzr4.fsf@evledraar.gmail.com","threadId":"49133","inReplyTo":"xmqq1sazk12m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-15T17:57:19Z","receivedAt":"2018-08-15T17:57:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 15 2018, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>>  # Guard against environment variables\n>>>  MAN1_TXT =\n>>> +MAN1_TXT_WIP =\n>>> +TCLTK_FILES =\n>>\n>> The latter name loses the fact that it is to hold candidates to be\n>> on MAN1_TXT that happen to be conditionally included; calling it\n>> MAN1_TXT_TCLTK or something, perhaps, may be an improvement.\n>>\n>> The former name makes it look it is work-in-progress, but in fact\n>> they are definite and unconditional part of MAN1_TXT.  Perhaps\n>> MAN1_TXT_CORE or something?\n>\n> Sorry, I misread the patch.  You collect all possible MAN1_TXT\n> candidates on _WIP, so \"this is unconditional core part\" is wrong.\n> Work-in-progress still sounds a bit funny, but now I know what is\n> going on a bit better, it has become at last understandable ;-)\n\nYeah maybe it should be *_TMP. It's because you can't assign to a make\nvariable twice (or rather, define a variable in terms of its previous\nvalue via filter). Otherwise I would just munge it in-place.\n\n>>> +ifndef NO_TCLTK\n>>> +MAN1_TXT_WIP += gitk.txt\n>>> +MAN1_TXT = $(MAN1_TXT_WIP)\n>>> +else\n>>> +TCLTK_FILES += git-gui.txt\n>>> +TCLTK_FILES += gitk.txt\n>>> +TCLTK_FILES += git-citool.txt\n>>> +MAN1_TXT = $(filter-out \\\n>>> +\t\t$(TCLTK_FILES), \\\n>>> +\t\t$(MAN1_TXT_WIP))\n>>> +endif\n>\n> I didn't notice it when I read it for the first time, but asymmetry\n> between these two looks somewhat strange.  If we are adding gitk.txt\n> when we are not declining TCLTK based programs, why can we do\n> without adding git-gui and git-citool at the same time?  If we know\n> we must add gitk.txt when we are not declining TCLTK based programs\n> to MAN1_TXT_WIP in this section, it must mean that when we do not\n> want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on\n> it, so why do we even need it on TCLTK_FILES list to filter it out?\n\nThe only explicitly listed files are those that don't match the wildcard\ngit-*.txt. Therefore if we want gitk.txt we need to explicitly list only\nit, but if we don't want the TCL programs we also need to list the ones\nthat match git-*.txt.\n"},{"id":"355732","messageId":"xmqq7ekrijw2.fsf@gitster-ct.c.googlers.com","threadId":"49133","inReplyTo":"87lg97qzr4.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-15T18:07:09Z","receivedAt":"2018-08-15T18:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>>> +ifndef NO_TCLTK\n>>>> +MAN1_TXT_WIP += gitk.txt\n>>>> +MAN1_TXT = $(MAN1_TXT_WIP)\n>>>> +else\n>>>> +TCLTK_FILES += git-gui.txt\n>>>> +TCLTK_FILES += gitk.txt\n>>>> +TCLTK_FILES += git-citool.txt\n>>>> +MAN1_TXT = $(filter-out \\\n>>>> +\t\t$(TCLTK_FILES), \\\n>>>> +\t\t$(MAN1_TXT_WIP))\n>>>> +endif\n>>\n>> I didn't notice it when I read it for the first time, but asymmetry\n>> between these two looks somewhat strange.  If we are adding gitk.txt\n>> when we are not declining TCLTK based programs, why can we do\n>> without adding git-gui and git-citool at the same time?  If we know\n>> we must add gitk.txt when we are not declining TCLTK based programs\n>> to MAN1_TXT_WIP in this section, it must mean that when we do not\n>> want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on\n>> it, so why do we even need it on TCLTK_FILES list to filter it out?\n>\n> The only explicitly listed files are those that don't match the wildcard\n> git-*.txt. Therefore if we want gitk.txt we need to explicitly list only\n> it, but if we don't want the TCL programs we also need to list the ones\n> that match git-*.txt.\n\nWhich means that gitk.txt does not have to be on TCLTK_FILES list,\nno?\n"},{"id":"355892","messageId":"20180817064259.GA131749@aiede.svl.corp.google.com","threadId":"49133","inReplyTo":"20180815151505.12011-1-avarab@gmail.com","subject":"Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-17T06:42:59Z","receivedAt":"2018-08-17T06:43:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n\n> Extend the NO_TCLTK=NoThanks flag to be understood by the\n> Documentation Makefile.\n>\n> Before this change compiling and installing with NO_TCLTK would result\n> in no git-gui, gitk or git-citool being installed, but their\n> respective manual pages would still be installed.\n\nLike Junio mentioned, what this commit message is missing is a\ndescription of why that is a bad thing.\n\nE.g. is this affecting some distro or some end user?  Are the docs\ntaking up too much space?  What is the motivation that leads to this\npatch?\n\n(I may be a little more sympathetic to the goal than Junio was if I\nunderstood correctly, but I think it's useful and important to spell\nthat goal out.  When building for a distro with NO_PYTHON=YesPlease,\nmy strategy was to simply rm the unwanted man pages after the fact in\nthe packaging rules and that worked fine.)\n\n[...]\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -1,5 +1,7 @@\n>  # Guard against environment variables\n>  MAN1_TXT =\n> +MAN1_TXT_WIP =\n\nIt took me a while to understand this MAN1_TXT_WIP.\n\nCould this just use MAN1_TXT and have the filtered one be\nMAN1_TXT_FILTERED?  Since the latter gets a value using =, it doesn't\nneed to be cleared up front.\n\nIt seems a little odd to see this specific to Tcl/Tk.  Do other\nlanguages with NO_x options need the same treatment?\n\nCurious,\nJonathan\n"}]}