Re: [PATCH] Makefile: check that tcl/tk is installed

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 17, 2017, 15:35 UTC
Message-ID: <CAP8UFD1hcFLMvNsXONPNbxZhTbHVzSMdRgCB9m=ZGeSTpMsYew@mail.gmail.com>
In-Reply-To: <xmqqbmk3xaxg.fsf@gitster.mtv.corp.google.com>

On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> To improve the current behavior when Tcl/Tk is not installed,
>> let's just check that TCLTK_PATH points to something and error
>> out right away if this is not the case.
>>
>> This should benefit people who actually want to install and use
>> git-gui or gitk as they will have to install Tcl/Tk anyway, and
>> it is better for them if they are told about installing it soon
>> in the build process, rather than if they have to debug it after
>> installing.
>
> Not objecting, but thinking aloud if this change makes sense.
>
> If you are building Git for your own use on the same box, which is
> presumably the majority of "build failed and I have no clue how to
> fix" case that needs help, if you want gui tools, you need to have
> tcl/tk installed anyway, whether you have msgfmt installed.  This
> seems to be the _only_ class of users this patch wants to cater to.
>
> I wonder if we are hurting people who are not in that category.
>
>  - To run gui tools, tcl/tk must be available at runtime, but tcl/tk
>    is not necessary in the packager's environment to produce a
>    package of Git that contains working git-gui and gitk that will
>    be used on an end-user box with tcl/tk installed, as long as the
>    packager's environment has a working msgfmt.
>
>  - To process .po files for the gui tools in the packager's
>    environment without msgfmt, tcl/tk is required.
>
> I suspect that this change will hurt those who package Git for other
> people.

Maybe a little bit, but in my opinion it should not be a big problem for them to install Tcl/Tk and its dependencies on the build machine.

Show 8 quoted lines

> It used to be that, as long as they have msgfmt installed, they only
> needed to _know_ what the path on the users' box to "wish" is, and
> set it to TCLTK_PATH, and if they are distro packagers, most likely
> they already have such an automated set-up working.  Now with this
> change, they are forced to install tcl/tk on their possibly headless
> box where tcl/tk is useless, and worse yet, an attempt to install it
> may bring in tons of unwanted stuff related to X that is irrelevant
> on such a headless development environment.

Yeah, but if they build gitk and git-gui, there is a significant chance that they build other graphical software too, and that this will require installing stuff related to X anyway.

> I doubt that this is quite a good trade-off; it feels that this
> burdens packagers a bit too much, and we may need a way to override
> this new check further.

I am ok to let packagers override this new check. For example they could set a flag like BYPASS_TCLTK_CHECK and the new check would be:

ifndef NO_TCLTK
      ifndef BYPASS_TCLTK_CHECK
             has_tcltk := $(shell type $(TCLTK_PATH) 2>/dev/null)
             ifndef has_tcltk
$(error "Tcl/Tk is not installed ('$(TCLTK_PATH)' not found). Consider
setting NO_TCLTK or installing Tcl/Tk")
             endif
      endif
endif

Of course BYPASS_TCLTK_CHECK would have to be documented at the same place where NO_TCLTK and TCLTK_PATH are already documented.

In general I think packagers are much more able to deal with those kinds of problems than most regular developers who want to hack on Git. So asking packagers to either set NO_TCLTK or BYPASS_TCLTK_CHECK or to install Tcl/Tk would not burden them much, especially compared to what regular developers have to deal with these days when trying to build Git.

> I think "If I cannot run either wish or
> msgfmt, then barf and give an error message" might at least be
> needed.  Am I misinterpreting the motivation of the patch?

I'd rather add a separate check for msgfmt than mixing the 2 issues, because I think that unless it has been explicitly told to do so, Git should not try to build git-gui and gitk in the first place if there is a big chance that those tools will not work.