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

From: Jeff King <peff@peff.net>
Date: Nov 27, 2017, 04:35 UTC
Message-ID: <20171127043502.GA5946@sigill>
In-Reply-To: <xmqqshd0e41a.fsf@gitster.mtv.corp.google.com>

On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote:

Show 25 quoted lines

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

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.

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

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

-Peff