threads / patch / 49133

patchMakefile: extend NO_TCLTK=NoThanks to cover docs

Subject: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

## tl;dr

6 messages between Aug 15, 2018 and Aug 17, 2018. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Aug 15, 2018, 15:15 UTC · lore

Extend the NO_TCLTK=NoThanks flag to be understood by the Documentation Makefile.

Before this change compiling and installing with NO_TCLTK would result in no git-gui, gitk or git-citool being installed, but their respective manual pages would still be installed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/Makefile | 23 ++++++++++++++++++-----
 Makefile               | 39 +++++++++++++++++++++------------------
 2 files changed, 39 insertions(+), 23 deletions(-)
Show changes to 2 files +39 −23

Documentation/Makefile, Makefile

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d079d7c73a..d53979939e 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -1,5 +1,7 @@
 # Guard against environment variables
 MAN1_TXT =
+MAN1_TXT_WIP =
+TCLTK_FILES =
 MAN5_TXT =
 MAN7_TXT =
 TECH_DOCS =
@@ -7,13 +9,24 @@ ARTICLES =
 SP_ARTICLES =
 OBSOLETE_HTML =
 
-MAN1_TXT += $(filter-out \
+MAN1_TXT_WIP += $(filter-out \
 		$(addsuffix .txt, $(ARTICLES) $(SP_ARTICLES)), \
 		$(wildcard git-*.txt))
-MAN1_TXT += git.txt
-MAN1_TXT += gitk.txt
-MAN1_TXT += gitremote-helpers.txt
-MAN1_TXT += gitweb.txt
+MAN1_TXT_WIP += git.txt
+MAN1_TXT_WIP += gitremote-helpers.txt
+MAN1_TXT_WIP += gitweb.txt
+
+ifndef NO_TCLTK
+MAN1_TXT_WIP += gitk.txt
+MAN1_TXT = $(MAN1_TXT_WIP)
+else
+TCLTK_FILES += git-gui.txt
+TCLTK_FILES += gitk.txt
+TCLTK_FILES += git-citool.txt
+MAN1_TXT = $(filter-out \
+		$(TCLTK_FILES), \
+		$(MAN1_TXT_WIP))
+endif
 
 MAN5_TXT += gitattributes.txt
 MAN5_TXT += githooks.txt
diff --git a/Makefile b/Makefile
index bc4fc8eeab..8abb23f6ce 100644
--- a/Makefile
+++ b/Makefile
@@ -2372,21 +2372,21 @@ export DEFAULT_EDITOR DEFAULT_PAGER
 
 .PHONY: doc man man-perl html info pdf
 doc: man-perl
-	$(MAKE) -C Documentation all
+	$(MAKE) -C Documentation all NO_TCLTK='$(NO_TCLTK)'
 
 man: man-perl
-	$(MAKE) -C Documentation man
+	$(MAKE) -C Documentation man NO_TCLTK='$(NO_TCLTK)'
 
 man-perl: perl/build/man/man3/Git.3pm
 
 html:
-	$(MAKE) -C Documentation html
+	$(MAKE) -C Documentation html NO_TCLTK='$(NO_TCLTK)'
 
 info:
-	$(MAKE) -C Documentation info
+	$(MAKE) -C Documentation info NO_TCLTK='$(NO_TCLTK)'
 
 pdf:
-	$(MAKE) -C Documentation pdf
+	$(MAKE) -C Documentation pdf NO_TCLTK='$(NO_TCLTK)'
 
 XGETTEXT_FLAGS = \
 	--force-po \
@@ -2802,10 +2802,10 @@ install-gitweb:
 	$(MAKE) -C gitweb install
 
 install-doc: install-man-perl
-	$(MAKE) -C Documentation install
+	$(MAKE) -C Documentation install NO_TCLTK='$(NO_TCLTK)'
 
 install-man: install-man-perl
-	$(MAKE) -C Documentation install-man
+	$(MAKE) -C Documentation install-man NO_TCLTK='$(NO_TCLTK)'
 
 install-man-perl: man-perl
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mandir_SQ)/man3'
@@ -2813,22 +2813,22 @@ install-man-perl: man-perl
 	(cd '$(DESTDIR_SQ)$(mandir_SQ)/man3' && umask 022 && $(TAR) xof -)
 
 install-html:
-	$(MAKE) -C Documentation install-html
+	$(MAKE) -C Documentation install-html NO_TCLTK='$(NO_TCLTK)'
 
 install-info:
-	$(MAKE) -C Documentation install-info
+	$(MAKE) -C Documentation install-info NO_TCLTK='$(NO_TCLTK)'
 
 install-pdf:
-	$(MAKE) -C Documentation install-pdf
+	$(MAKE) -C Documentation install-pdf NO_TCLTK='$(NO_TCLTK)'
 
 quick-install-doc:
-	$(MAKE) -C Documentation quick-install
+	$(MAKE) -C Documentation quick-install NO_TCLTK='$(NO_TCLTK)'
 
 quick-install-man:
-	$(MAKE) -C Documentation quick-install-man
+	$(MAKE) -C Documentation quick-install-man NO_TCLTK='$(NO_TCLTK)'
 
 quick-install-html:
-	$(MAKE) -C Documentation quick-install-html
+	$(MAKE) -C Documentation quick-install-html NO_TCLTK='$(NO_TCLTK)'
 
 
 
@@ -2875,13 +2875,16 @@ manpages = git-manpages-$(GIT_VERSION)
 dist-doc:
 	$(RM) -r .doc-tmp-dir
 	mkdir .doc-tmp-dir
-	$(MAKE) -C Documentation WEBDOC_DEST=../.doc-tmp-dir install-webdoc
+	$(MAKE) -C Documentation NO_TCLTK='$(NO_TCLTK)' \
+		WEBDOC_DEST=../.doc-tmp-dir install-webdoc
 	cd .doc-tmp-dir && $(TAR) cf ../$(htmldocs).tar .
 	gzip -n -9 -f $(htmldocs).tar
 	:
 	$(RM) -r .doc-tmp-dir
 	mkdir -p .doc-tmp-dir/man1 .doc-tmp-dir/man5 .doc-tmp-dir/man7
-	$(MAKE) -C Documentation DESTDIR=./ \
+	$(MAKE) -C Documentation \
+		NO_TCLTK='$(NO_TCLTK)' \
+		DESTDIR=./ \
 		man1dir=../.doc-tmp-dir/man1 \
 		man5dir=../.doc-tmp-dir/man5 \
 		man7dir=../.doc-tmp-dir/man7 \
@@ -2915,7 +2918,7 @@ clean: profile-clean coverage-clean
 	$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
 	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
 	$(RM) contrib/coccinelle/*.cocci.patch*
-	$(MAKE) -C Documentation/ clean
+	$(MAKE) -C Documentation/ clean NO_TCLTK='$(NO_TCLTK)'
 ifndef NO_PERL
 	$(MAKE) -C gitweb clean
 	$(RM) -r perl/build/
@@ -2944,7 +2947,7 @@ ALL_COMMANDS += git-gui git-citool
 
 .PHONY: check-docs
 check-docs::
-	$(MAKE) -C Documentation lint-docs
+	$(MAKE) -C Documentation lint-docs NO_TCLTK='$(NO_TCLTK)'
 	@(for v in $(ALL_COMMANDS); \
 	do \
 		case "$$v" in \
@@ -2968,7 +2971,7 @@ check-docs::
 		    -e '/^#/d' \
 		    -e 's/[ 	].*//' \
 		    -e 's/^/listed /' command-list.txt; \
-		$(MAKE) -C Documentation print-man1 | \
+		$(MAKE) -C Documentation print-man1  NO_TCLTK='$(NO_TCLTK)' | \
 		grep '\.txt$$' | \
 		sed -e 's|Documentation/|documented |' \
 		    -e 's/\.txt//'; \

-- 
2.18.0.865.gffc8e1a3cd6
Junio C Hamano· Aug 15, 2018, 16:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 6 quoted lines
> Extend the NO_TCLTK=NoThanks flag to be understood by the
> Documentation Makefile.
>
> Before this change compiling and installing with NO_TCLTK would result
> in no git-gui, gitk or git-citool being installed, but their
> respective manual pages would still be installed.
Thanks, but I personally do not perceive it to be a problem.

It is perfectly OK to install programs without installing docs, and also it is OK to install docs without programs. I do not see why gitk.html and the reference to it from the main ToC in git.html must be removed when you are not installing gitk.

Lack of an option to skip them from the documentation is something we might want to improve, but you should be able to install the docs for programs you do not happen to have, and I think it should be the default.

By the way, to be more explicit than merely to hint, I think this patch needs to also update Documentation/cmd-list.perl so that under NO_TCLTK, the entry for gitk is omitted from cmds-mainporcelain.txt, etc. to be a complete solution to your original problem.

Show 15 quoted lines
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/Makefile | 23 ++++++++++++++++++-----
>  Makefile               | 39 +++++++++++++++++++++------------------
>  2 files changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index d079d7c73a..d53979939e 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -1,5 +1,7 @@
>  # Guard against environment variables
>  MAN1_TXT =
> +MAN1_TXT_WIP =
> +TCLTK_FILES =

The latter name loses the fact that it is to hold candidates to be on MAN1_TXT that happen to be conditionally included; calling it MAN1_TXT_TCLTK or something, perhaps, may be an improvement.

The former name makes it look it is work-in-progress, but in fact they are definite and unconditional part of MAN1_TXT. Perhaps MAN1_TXT_CORE or something?

Show 33 quoted lines
> ...
> +MAN1_TXT_WIP += git.txt
> +MAN1_TXT_WIP += gitremote-helpers.txt
> +MAN1_TXT_WIP += gitweb.txt
> +
> +ifndef NO_TCLTK
> +MAN1_TXT_WIP += gitk.txt
> +MAN1_TXT = $(MAN1_TXT_WIP)
> +else
> +TCLTK_FILES += git-gui.txt
> +TCLTK_FILES += gitk.txt
> +TCLTK_FILES += git-citool.txt
> +MAN1_TXT = $(filter-out \
> +		$(TCLTK_FILES), \
> +		$(MAN1_TXT_WIP))
> +endif
>  
>  MAN5_TXT += gitattributes.txt
>  MAN5_TXT += githooks.txt
> diff --git a/Makefile b/Makefile
> index bc4fc8eeab..8abb23f6ce 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2372,21 +2372,21 @@ export DEFAULT_EDITOR DEFAULT_PAGER
>  
>  .PHONY: doc man man-perl html info pdf
>  doc: man-perl
> -	$(MAKE) -C Documentation all
> +	$(MAKE) -C Documentation all NO_TCLTK='$(NO_TCLTK)'
> ...
>  pdf:
> -	$(MAKE) -C Documentation pdf
> +	$(MAKE) -C Documentation pdf NO_TCLTK='$(NO_TCLTK)'

Since we are assuming GNU make anyway, can we just say "export NO_TCLTK" just once, or would it be too magical and create maintenance burden?

Junio C Hamano· Aug 15, 2018, 17:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>>  # Guard against environment variables
>>  MAN1_TXT =
>> +MAN1_TXT_WIP =
>> +TCLTK_FILES =
>
> The latter name loses the fact that it is to hold candidates to be
> on MAN1_TXT that happen to be conditionally included; calling it
> MAN1_TXT_TCLTK or something, perhaps, may be an improvement.
>
> The former name makes it look it is work-in-progress, but in fact
> they are definite and unconditional part of MAN1_TXT.  Perhaps
> MAN1_TXT_CORE or something?

Sorry, I misread the patch. You collect all possible MAN1_TXT candidates on _WIP, so "this is unconditional core part" is wrong. Work-in-progress still sounds a bit funny, but now I know what is going on a bit better, it has become at last understandable ;-)

Show 11 quoted lines
>> +ifndef NO_TCLTK
>> +MAN1_TXT_WIP += gitk.txt
>> +MAN1_TXT = $(MAN1_TXT_WIP)
>> +else
>> +TCLTK_FILES += git-gui.txt
>> +TCLTK_FILES += gitk.txt
>> +TCLTK_FILES += git-citool.txt
>> +MAN1_TXT = $(filter-out \
>> +		$(TCLTK_FILES), \
>> +		$(MAN1_TXT_WIP))
>> +endif

I didn't notice it when I read it for the first time, but asymmetry between these two looks somewhat strange. If we are adding gitk.txt when we are not declining TCLTK based programs, why can we do without adding git-gui and git-citool at the same time? If we know we must add gitk.txt when we are not declining TCLTK based programs to MAN1_TXT_WIP in this section, it must mean that when we do not want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on it, so why do we even need it on TCLTK_FILES list to filter it out?

Ævar Arnfjörð Bjarmason· Aug 15, 2018, 17:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

On Wed, Aug 15 2018, Junio C Hamano wrote:
Show 19 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>>  # Guard against environment variables
>>>  MAN1_TXT =
>>> +MAN1_TXT_WIP =
>>> +TCLTK_FILES =
>>
>> The latter name loses the fact that it is to hold candidates to be
>> on MAN1_TXT that happen to be conditionally included; calling it
>> MAN1_TXT_TCLTK or something, perhaps, may be an improvement.
>>
>> The former name makes it look it is work-in-progress, but in fact
>> they are definite and unconditional part of MAN1_TXT.  Perhaps
>> MAN1_TXT_CORE or something?
>
> Sorry, I misread the patch.  You collect all possible MAN1_TXT
> candidates on _WIP, so "this is unconditional core part" is wrong.
> Work-in-progress still sounds a bit funny, but now I know what is
> going on a bit better, it has become at last understandable ;-)

Yeah maybe it should be *_TMP. It's because you can't assign to a make variable twice (or rather, define a variable in terms of its previous value via filter). Otherwise I would just munge it in-place.

Show 20 quoted lines
>>> +ifndef NO_TCLTK
>>> +MAN1_TXT_WIP += gitk.txt
>>> +MAN1_TXT = $(MAN1_TXT_WIP)
>>> +else
>>> +TCLTK_FILES += git-gui.txt
>>> +TCLTK_FILES += gitk.txt
>>> +TCLTK_FILES += git-citool.txt
>>> +MAN1_TXT = $(filter-out \
>>> +		$(TCLTK_FILES), \
>>> +		$(MAN1_TXT_WIP))
>>> +endif
>
> I didn't notice it when I read it for the first time, but asymmetry
> between these two looks somewhat strange.  If we are adding gitk.txt
> when we are not declining TCLTK based programs, why can we do
> without adding git-gui and git-citool at the same time?  If we know
> we must add gitk.txt when we are not declining TCLTK based programs
> to MAN1_TXT_WIP in this section, it must mean that when we do not
> want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on
> it, so why do we even need it on TCLTK_FILES list to filter it out?

The only explicitly listed files are those that don't match the wildcard git-*.txt. Therefore if we want gitk.txt we need to explicitly list only it, but if we don't want the TCL programs we also need to list the ones that match git-*.txt.

Junio C Hamano· Aug 15, 2018, 18:07 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 25 quoted lines
>>>> +ifndef NO_TCLTK
>>>> +MAN1_TXT_WIP += gitk.txt
>>>> +MAN1_TXT = $(MAN1_TXT_WIP)
>>>> +else
>>>> +TCLTK_FILES += git-gui.txt
>>>> +TCLTK_FILES += gitk.txt
>>>> +TCLTK_FILES += git-citool.txt
>>>> +MAN1_TXT = $(filter-out \
>>>> +		$(TCLTK_FILES), \
>>>> +		$(MAN1_TXT_WIP))
>>>> +endif
>>
>> I didn't notice it when I read it for the first time, but asymmetry
>> between these two looks somewhat strange.  If we are adding gitk.txt
>> when we are not declining TCLTK based programs, why can we do
>> without adding git-gui and git-citool at the same time?  If we know
>> we must add gitk.txt when we are not declining TCLTK based programs
>> to MAN1_TXT_WIP in this section, it must mean that when we do not
>> want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on
>> it, so why do we even need it on TCLTK_FILES list to filter it out?
>
> The only explicitly listed files are those that don't match the wildcard
> git-*.txt. Therefore if we want gitk.txt we need to explicitly list only
> it, but if we don't want the TCL programs we also need to list the ones
> that match git-*.txt.

Which means that gitk.txt does not have to be on TCLTK_FILES list, no?

Jonathan Nieder· Aug 17, 2018, 06:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs

Hi,
Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> Extend the NO_TCLTK=NoThanks flag to be understood by the
> Documentation Makefile.
>
> Before this change compiling and installing with NO_TCLTK would result
> in no git-gui, gitk or git-citool being installed, but their
> respective manual pages would still be installed.

Like Junio mentioned, what this commit message is missing is a description of why that is a bad thing.

E.g. is this affecting some distro or some end user? Are the docs taking up too much space? What is the motivation that leads to this patch?

(I may be a little more sympathetic to the goal than Junio was if I understood correctly, but I think it's useful and important to spell that goal out. When building for a distro with NO_PYTHON=YesPlease, my strategy was to simply rm the unwanted man pages after the fact in the packaging rules and that worked fine.)

[...]
Show 6 quoted lines
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -1,5 +1,7 @@
>  # Guard against environment variables
>  MAN1_TXT =
> +MAN1_TXT_WIP =
It took me a while to understand this MAN1_TXT_WIP.

Could this just use MAN1_TXT and have the filtered one be MAN1_TXT_FILTERED? Since the latter gets a value using =, it doesn't need to be cleared up front.

It seems a little odd to see this specific to Tcl/Tk. Do other languages with NO_x options need the same treatment?

Curious, Jonathan

← back to recent threads