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

[Christian Couder <christian.couder@gmail.com>]

On Mon, Nov 27, 2017 at 5:35 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Perhaps the "else" part of the above should become a bit more
>> > careful, something along the lines of...
>> >
>> >     else
>> >             MSGFMT ?= msgfmt
>> > -           ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; echo $$?),0)
>> > -                   MSGFMT := $(TCL_PATH) po/po2msg.sh
>> > -           endif
>> > +           MSGFMT_DRYRUN = --tcl -l C -d . /dev/null 2>/dev/null
>> > +            ifneq ($(shell $(MSGFMT) $(MSGFMT_DRYRUN); echo $$?),0)
>> > +               ifneq ($(shell $(TCL_PATH) po/po2msg.sh $(MSGFMT_DRYRUN); echo $$?),0)
>> > +                    MSGFMT := $(TCL_PATH) po/po2msg.sh
>> > +               else
>> > +                   $(error "no usable msgfmt to build gitk; set NO_TCLTK perhaps?")
>> > +               endif
>> >             endif
>> >     endif
>>
>> Actually, at this point, I think the suggestion should primarily be
>> to install either msgfmt or tclsh; offering another choice to set
>> NO_TCLTK is OK, but it should be secondary, as the fact that the
>> make utility is running this recipe is a sign that the builder wants
>> to build gitk/git-gui.

What if the user actually don't care about internationalization?

The problem is that inside git-gui, the option to disable msgfmt is
NO_MSGFMT, while in the git repo it is NO_GETTEXT, so if we make this
change in git-gui, we should suggest using NO_MSGFMT to disable
msgfmt. But then the user building git and setting NO_MSGFMT will get
another msgfmt related error later in the git build (as NO_MSGFMT will
have no effect in the git build) and will not understand at all why
there is still a msgfmt error despite setting NO_MSGFMT as the build
suggested.

> I think that's the rub, though. We hit this code path by default, so
> it's _not_ a sign that the builder cares about gitk.

Yeah, I agree. That's why I think it is a good idea if Tcl/Tk is not
installed to ask for either setting NO_TCLTK or installing Tcl/Tk or
setting BYPASS_TCLTK_CHECK.

> I do agree that outlining "install one of these or disable tcl" as the
> options is a good idea, though.

The problem is that we should suggest disabling msgfmt as it is a
valid solution, but as explained above there is an issue related to
NO_MSGFMT in git-gui vs NO_GETTEXT in git.

That's also one of the reason why I don't want to mix the Tcl/Tk issue
and the msgfmt issue. In the end I think we should fix both, but if it
is not possible to fix the simpler Tcl/Tk issue first, it's not even
worth spending time to take a deep look at the msgfmt issue.

>> Whether the builder wants to run the result on the same box is a
>> separate and irrelevant matter.  Once built and installed, a box
>> without "wish" may not be able to run the result, but installing it
>> after the fact will fix it without need to rebuild anything.

Yeah, people have not complained about that and it is not a really bad
situation, but I don't think this is the best practice nor the best we
can do, and I think the situation in this regard is better after my
patch.

For example if someone builds a box that should be used afterwards in
an internal network where this no way to install Tcl/Tk, then users
will be screwed when they will try to run gitk or git-gui. The same
thing can happen if someone installs git just before a long plane trip
with no Internet access. Also the person using git-gui or gitk may not
be the same person as the person installing the box, so the user might
just not have the rights to actually install things.

As I wrote in a previous email, in general it is best to try to fix
issues as soon possible, so it is better to invite people to decide if
they want to install Tcl/Tk at build time rather than at run time.
Yeah, I know that it is not best for packagers and maybe a few other
special cases, but I think setting BYPASS_TCLTK_CHECK should be a good
enough workaround in those cases.

> Yeah, this side-steps the "other half" of the issue that Christian's
> patch addresses, which seems like the more controversial part (I don't
> have a strong opinion myself, though).

I don't think any part of my patch should be controversial. I
repeatedly wrote very long messages to show all the possible cases, so
that it is easy to see that we are not worse in any case. And all the
competing suggestions, even the above from Junio either have
significant problems or address a different problem.