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

On Sun, Nov 26, 2017 at 8:15 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 21, 2017 at 12:58:17AM +0100, Christian Couder wrote:

quoted

quoted

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.

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.

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.