patchMakefile: check that tcl/tk is installed

By default running `make install` in the root directory of the project will set TCLTK_PATH to `wish` and then go into the "git-gui" and "gitk-git" sub-directories to build and install these 2 sub-projects.

When Tcl/Tk is not installed, the above will succeed if gettext is installed, as Tcl/Tk is only required as a substitute for msgfmt when msgfmt is not installed. But then running the installed gitk and git-gui will fail.

If neither Tcl/Tk nor gettext are installed, then processing po files will fail in the git-gui directory. The error message when this happens is very confusing to new comers as it is difficult to understand that we tried to use Tcl/Tk as a substitute for msgfmt, and that the solution is to either install gettext or Tcl/Tk, or to set both NO_GETTEXT and NO_TCLTK.

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.

For people who don't want to use git-gui or gitk, this forces them to specify NO_TCLTK. If they don't have gettext, this will make it much easier to fix the problems they would have had to fix anyway. If they have gettext, setting NO_TCLTK is a small additional step they will have to make, but it might be a good thing as it will not install what they don't want and it will build a bit faster.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

Christian Couder <christian.couder@gmail.com> writes:

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.

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.

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

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

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.

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

Christian Couder wrote:

It's not a big burden, but it is a seemingly unnecessary build-time dependency.

Most distributions build packages in individual container or chroots, to increase the stability and reproducibility of the builds. So package builds don't run on systems where any deps have already been installed.

To be fair, it looks like pulling in tcl/tk would add only around 8MB to the Fedora build root for git. That's not egregious, to be sure. But it really isn't a necessary build-time dependency either. I don't know if there are other distros who would strongly object to pulling in tcl/tk. Some are much more sensitive to build root sizes and unnecessary dependencies.

I agree. Packagers also provide git builds to the vast majority of end-users, so we should make their task easier whenever possible. :)

Presuming this new BYPASS_TCLTK_CHECK is communicated well and that the failure when not using it is clear, this doesn't seem likely to cause problems. (I'll leave it to others whether there's a better way to solve the msgfmt fallthrough issue. I didn't even know such a fallthrough existed until yesterday.)

I think it's important to ensure that automated package builds of a newer git don't simply skip parts of the build which used to work and so packagers reading the failed builds logs can easily see what they need to adjust.

Just dropping the new variable in the Makefile and waiting for package builds to fail or not package gitk & git-gui at the next release would be a bit unkind, I think. Posting this to the git-packagers group[1] which Ævar created would be useful. It /might/ even be worth asking there if any distros have strong opinions on the subject.

[1] https://groups.google.com/forum/#!forum/git-packagers and
    git-packagers@googlegroups.com

If that's a motivation, wouldn't a check in the gitk and git-gui scripts handle it? That would provide an error at run time to the user. This change is about helping the user who builds their own git and then runs it, so if they built git without wish installed and then ran git-gui, they'd get a clear error that wish is missing and could easily install it. It's not needed for the build, so they wouldn't need to rebuild anything.

Something like this:

On Fri, Nov 17, 2017 at 12:42:58PM -0500, Todd Zullinger wrote:

I think the message is already OK:

  $ ./gitk
  ./gitk: 3: exec: wish: not found

The question is whether we would want to catch this at build time. And I think Junio's point is that we don't _know_ it's an error at build time. We could be building gitk for use on a system that isn't quite like the build system, so any "solution" here is going to have to make an assumption either way.

It's also not foolproof. You could build when wish is present, and then later uninstall it and receive the same error message.

I also think all of this is largely orthogonal to gettext. It just so happens that if you don't have gettext installed, we'll try to run wish as part of the build process, but detecting broken tcl setups was definitely not part of the intent there.

And the failure actually runs the other way, too. If you have neither gettext nor tcl, you get this confusing output:

  $ make NO_GETTEXT=1
  ...
  MSGFMT po/pt_pt.msg     MSGFMT    po/hu.msg Makefile:252: recipe for target 'po/pt_pt.msg' failed
  make[1]: *** [po/pt_pt.msg] Error 127

(the problem is not msgfmt, but our tcl substitute which cannot run).

I'm actually tempted to say that we should not be building the tcl parts by default. IOW, instead of NO_TCLTK we should have USE_TCLTK. That would also require an adjustment by package builders, but it would hopefully be a really obvious one. And once the user has told our Makefile that they definitely want to build the tcl parts, we'd presumably just trust that the tcl path they give us is sane.

But it's possible I'm underestimating how many people actually use the tcl scripts. Certainly I don't, and git-gui seems fairly primitive to me these days compared to 3rd party tools. But then I don't use any of them either. ;)

-Peff

On Fri, Nov 17, 2017 at 11:02 PM, Jeff King <peff@peff.net> wrote:

As I wrote in the version 2 of the patch (yeah I forgot to mark it as version 2) I agree that removing NO_TCLTK while introducing USE_TCLTK and not building git-gui and gitk by default is simpler and probably the right direction for the future, but I think it might be too big a change for now or until the next major release (Git 3.0.0).

Especially it can cause issues to people who were building and installing gitk and git-gui because they use it. They might not realize that USE_TCLTK has been introduced and still think that they are building and installing the newest versions, when in fact they would not and keep using old versions.

On Fri, Nov 17, 2017 at 6:42 PM, Todd Zullinger <tmz@pobox.com> wrote:

Yeah, I still think that when packaging graphical tools, packagers should be used to managing builds that need a lot of dependencies (especially X related dependencies). I used to be a KDE developer in a previous life and the amount of dependencies to build KDE was much larger than what is required for everything in the git repo (git, gitk, git-gui, git-svn, etc).

Yeah, but you might have noticed that such checks might be a good thing for packagers, as it makes the build fail right away with a clear error message. So in the long run, I think this kind of patches will make it easier also for packagers.

Yeah I agree.

I might also send a similar patch for the msgfmt issue, otherwise it may be a good #leftoverbit for someone starting to hack on Git. But anyway it is a separate issue.

I agree and the patch doesn't skip parts of the build that used to work, and actually it makes logs easier to understand and build failures easier to fix.

I cc'ed this list when I sent version 2 of the patch.

This change is not just about people who want to build and run those tools, but it already helps them by improving build error messages a lot.