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

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 26, 2017, 20:57 UTC
Message-ID: <CAP8UFD1hRWa1YtgRPZxXvkqcHfUoKKxgVUuN_d9C36jbrGBBXA@mail.gmail.com>
In-Reply-To: <20171126191510.GA1501@sigill>

On Sun, Nov 26, 2017 at 8:15 PM, Jeff King <peff@peff.net> wrote:

Show 25 quoted lines

> On Tue, Nov 21, 2017 at 12:58:17AM +0100, Christian Couder wrote:
>
>> > Can you say more about where this comes up?
>>
>> The original discussion is:
>>
>> https://public-inbox.org/git/b6b12040-100f-5965-6dfd-344c84dddf96@teddy.ch/
>>
>> and here are discussions related to version 1 of this patch:
>>
>> https://public-inbox.org/git/20171115125200.17006-1-chriscool@tuxfamily.org/
>>
>> As Peff mentions in the original discussion, at the Bloomberg Git
>> sprint, we saw someone struggling to compile Git, because of these
>> msgfmt and Tcl/Tk issues.
>
> Actually, I think we had the _opposite_ problem there.
>
> The main problem your patch fixes is that we may silently build a
> version of gitk/git-gui that do not work. The "make" process completes,
> but they refer to a non-existent "wish" tool, and running them will
> fail.
>
> That's potentially annoying if you wanted those tools. But if you didn't
> care about them in the first place, it's fine.

I think it's a bit better to not install the tools if you don't care about them. So overall whether you care or not about them, there is still a bit of improvement.

Show 11 quoted lines

> The opposite problem is when you don't care about those tools, and they
> _do_ break the build. And then just to get the rest of Git built, you
> have to know about and set NO_TCLTK.
>
> AFAIK that only happens if you don't have msgfmt installed. Because then
> the gitk and git-gui Makefiles try to auto-fallback to implementing
> msgfmt in tcl _during the build_, and there a lack of "tclsh" will break
> the build.
>
> I think your patch does say "consider setting NO_TCLTK" in that case,
> which is an improvement.

Yeah, so my patch actually improve things in all the cases.

> But it might be nicer still if it Just Worked
> (either because we don't do tcl/tk by default, or because we respect
> NO_GETTEXT in the gitk/git-gui Makefiles, or because our msgfmt can
> fallback further to not even using tclsh).

Yeah, it might be nicer if it just worked, but as I already answered in another thread, it could break some environments if we just stopped installing gitk and git-gui by default.

About improving the way msgfmt is handled, I am not against it. In fact I might even give it a try, but I think it is a separate issue, and I don't want to mix those issues right now.

> So I'm not really against this patch, but IMHO it doesn't make the
> interesting case (you don't care about tcl and are just trying to build
> git for the first time) all that much better.

I agree that it doesn't solve all the issues, if you are trying to build git for the first time, but I do think that it makes it easier. If you don't have msgfmt, you get an error that is not so difficult to debug.

> I do also wonder if we
> want to start putting these kind of run-time checks into the Makefile
> itself. That's kind of what autoconf is for.

I don't quite agree that autoconf is for that, and there are already some checks in the Makefile.

Show 9 quoted lines

> As much as I hate autoconf,
> is it the right advice for somebody who doesn't want to look at the
> Makefile knobs to do:
>
>   autoconf
>   ./configure
>   make
>
> ?

I don't think so. I think it is just easier to advice to do as most of us do, and to just add a few checks in the Makefile to make it clear which dependencies should be installed or which knob should be tweaked.

> If there are deficiencies in configure.in (and I can well believe that
> there are), should we be fixing it there?

If most of us don't use autoconf, even if we fix the current deficiencies, there could still be some a few years from now. While if we add checks to the Makefile, there is a good chance that those who change the Makefile will see the existing tests and add more if necessary.