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

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

[Ævar Arnfjörð Bjarmason]

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(-)

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.

> 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?

> ...
> +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?

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

[Junio C Hamano]

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 ;-)

>> +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?

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

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 15 2018, Junio C Hamano wrote:

> 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.

>>> +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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>>> +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?

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

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> 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.)

[...]
> --- 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