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

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

[Christian Couder]

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.

For packagers who want to build git-gui and gitk even if Tcl/Tk
is not installed, we provide the new BYPASS_TCLTK_CHECK variable.

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

The changes compared to the previous version are related to the newly
introduced BYPASS_TCLTK_CHECK variable.

I agree that removing NO_TCLTK while introducing USE_TCLTK and not
building git-gui and gitk by default is perhaps 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).

diff --git a/Makefile b/Makefile
index ee9d5eb11e..1637725780 100644
--- a/Makefile
+++ b/Makefile
@@ -318,6 +318,10 @@ all::
 # If not set it defaults to the bare 'wish'. If it is set to the empty
 # string then NO_TCLTK will be forced (this is used by configure script).
 #
+# The BYPASS_TCLTK_CHECK variable can be used when you want to build
+# the Tcl/Tk GUI but don't want to install Tcl/Tk on the build
+# machine.
+#
 # Define INTERNAL_QSORT to use Git's implementation of qsort(), which
 # is a simplified version of the merge sort used in glibc. This is
 # recommended if Git triggers O(n^2) behavior in your platform's qsort().
@@ -1636,6 +1640,15 @@ ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
 
+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
+
 ifeq ($(PERL_PATH),)
 NO_PERL = NoThanks
 endif
-- 
2.15.0.165.g82024f6603.dirty

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

[Christian Couder]

(Sorry I forgot to mark this as v2.)

On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> 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.

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

[Jonathan Nieder]

Hi,

Christian Couder wrote:

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

Hm, I am not sure I understand the point of this change.  E.g.
if I run "make install" for git and install tk later, wouldn't I
want gitk to work?

Can you say more about where this comes up? gitk is a wrapper
script

	#!/bin/sh
	# Tcl ignores the next line -*- tcl -*- \
	exec wish "$0" -- "$@"

Would some error handling there help?  E.g. something like

	#!/bin/sh
	# Tcl ignores the next line -*- tcl -*- \
	exec wish "$0" -- "$@" || \
	{ echo >&2 "Cannot run gitk without tk"; exit 127; }

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

Hm, is this the motivating problem?  This is a condition where
the rationale for failing the build seems clearer.

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

At first glance I had thought this might set NO_TCLTK automatically,
which I think would be problematic for the reasons mentioned above.

Erroring out like this patch does and asking the user to explicitly
confirm that they want to install gitk without Tcl/Tk is less
problematic, so I am not *against* this patch, just interested in more
background.

Thanks and hope that helps,
Jonathan

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

[Christian Couder]

Hi,

On Mon, Nov 20, 2017 at 8:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Christian Couder wrote:
>
>> 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.
>
> Hm, I am not sure I understand the point of this change.  E.g.
> if I run "make install" for git and install tk later, wouldn't I
> want gitk to work?

Yeah, if you know how it all works and want to decide after installing
gitk and git-gui if you actually want to use them, then things are a
bit less straightforward with this patch, though you can still do what
you want by setting the BYPASS_TCLTK_CHECK variable.
But I doubt that many people who are actually building Git are in this case.

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

> gitk is a wrapper
> script
>
>         #!/bin/sh
>         # Tcl ignores the next line -*- tcl -*- \
>         exec wish "$0" -- "$@"
>
> Would some error handling there help?  E.g. something like
>
>         #!/bin/sh
>         # Tcl ignores the next line -*- tcl -*- \
>         exec wish "$0" -- "$@" || \
>         { echo >&2 "Cannot run gitk without tk"; exit 127; }

I think Peff already gave his opinion about this in the above discussions.

>> 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.
>
> Hm, is this the motivating problem?  This is a condition where
> the rationale for failing the build seems clearer.

This is the issue as well as the above thread that prompted me to take
a look at this.

>> 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.
>
> At first glance I had thought this might set NO_TCLTK automatically,
> which I think would be problematic for the reasons mentioned above.
>
> Erroring out like this patch does and asking the user to explicitly
> confirm that they want to install gitk without Tcl/Tk is less
> problematic,

Yeah that is also my opinion.

> so I am not *against* this patch, just interested in more
> background.

Thanks for taking a look at this,
Christian.

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

[Christian Couder]

On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> 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.

Has this patch fallen through the cracks or is there an unresolved issue?

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

[Junio C Hamano]

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

> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> 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.
>
> Has this patch fallen through the cracks or is there an unresolved issue?

I had an impression that the conclusion was that the existing error
message at runtime already does an adequate job and there is no
issue to be addressed by this patch.  Am I mistaken?

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

[Christian Couder]

On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> 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.
>>
>> Has this patch fallen through the cracks or is there an unresolved issue?
>
> I had an impression that the conclusion was that the existing error
> message at runtime already does an adequate job and there is no
> issue to be addressed by this patch.  Am I mistaken?

This patch is mostly about what happens at the build step. Its goal is
not much to improve what happens at runtime, though that is improved a
bit too. If the build step was good enough, then I would agree that
what happens at run time is adequate.

Let's consider only people installing git using "make install" to use
it on their machine, as I think I already discussed the case of
packagers and added the BYPASS_TCLTK_CHECK variable for them.

When those users run "make install", let's suppose they don't have
Tcl/Tk installed. (If it is already installed my patch changes
nothing.)
Let's also suppose that NO_TCLTK is not set. (If it is set my patch
changes nothing.)

Then there are 2 cases:

- msgfmt is already installed

Without my patch, the "make install" step works and installs git,
git-gui and gitk. But the user might not want to have git-gui and gitk
installed in the first place. As Jeff says there are a lot of other
graphical tools, that are more advanced these days, so there is a big
chance that they just forgot to set NO_TCLTK or just didn't know about
this variable. Now if they actually want to use git-gui and gitk, they
will get an error at run time (which is adequate) and they will have
to install Tck/Tk then before they can git-gui and gitk.

With my patch, the "make install" step fails right away telling them
to either set NO_TCLTK or install Tcl/Tk. If they don't want to use
git-gui and gitk, they will set NO_TCLTK and then run "make install"
again and things will be exactly as they want. If they do want to use
git-gui and gitk, they will install Tcl/Tk and then run "make install"
again and then things will be exactly as they want and there will be
no error at runtime.

So my opinion is that whether they actually want to use git-gui and
gitk or not, forcing them to decide is probably a good thing because
it ensures that when "make install" succeeds things are exactly how
they want them to be. The only case when it might not be a good thing
is if they don't know yet if they will actually want to use gitk and
git-gui. But even in this case, which is not the most common, they are
not much worse.

- msgfmt is not installed

Without my patch, the "make install" step produces an error message
while building git-gui. Debugging and fixing the error is quite
difficult especially for new comers. They will have to either set
NO_GETTEXT or install gettext, and to either set NO_TCLTK or to
install Tcl/Tk before the build can fully succeed.

With my patch, the "make install" step fails right away telling them
to either set NO_TCLTK or install Tcl/Tk. Then the build will fail
again and they will have to either set NO_GETTEXT or install gettext,
but this error is easier to understand and to fix than without my
patch.

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

[Ramsay Jones]

On 26/11/17 14:00, Christian Couder wrote:
> On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
>>> <christian.couder@gmail.com> wrote:
>>>> 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.
>>>
>>> Has this patch fallen through the cracks or is there an unresolved issue?
>>
>> I had an impression that the conclusion was that the existing error
>> message at runtime already does an adequate job and there is no
>> issue to be addressed by this patch.  Am I mistaken?
> 
> This patch is mostly about what happens at the build step. Its goal is
> not much to improve what happens at runtime, though that is improved a
> bit too. If the build step was good enough, then I would agree that
> what happens at run time is adequate.
> 
> Let's consider only people installing git using "make install" to use
> it on their machine, as I think I already discussed the case of
> packagers and added the BYPASS_TCLTK_CHECK variable for them.
> 

I haven't been following this thread too closely, but I have the
feeling that the best course of action is to simply not fall back
to using a tcl version of msgfmt in the first place!. ;-)

If a given platform does not have gettext/msgfmt, then you just
don't get an i18n-ed version of git. (no need for BYPASS_ ...).

Am I missing something?

ATB,
Ramsay Jones

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

[Christian Couder]

On Sun, Nov 26, 2017 at 6:43 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 26/11/17 14:00, Christian Couder wrote:
>> On Sun, Nov 26, 2017 at 4:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> On Mon, Nov 20, 2017 at 6:15 PM, Christian Couder
>>>> <christian.couder@gmail.com> wrote:
>>>>> 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.
>>>>
>>>> Has this patch fallen through the cracks or is there an unresolved issue?
>>>
>>> I had an impression that the conclusion was that the existing error
>>> message at runtime already does an adequate job and there is no
>>> issue to be addressed by this patch.  Am I mistaken?
>>
>> This patch is mostly about what happens at the build step. Its goal is
>> not much to improve what happens at runtime, though that is improved a
>> bit too. If the build step was good enough, then I would agree that
>> what happens at run time is adequate.
>>
>> Let's consider only people installing git using "make install" to use
>> it on their machine, as I think I already discussed the case of
>> packagers and added the BYPASS_TCLTK_CHECK variable for them.
>>
>
> I haven't been following this thread too closely, but I have the
> feeling that the best course of action is to simply not fall back
> to using a tcl version of msgfmt in the first place!. ;-)

Well, another possibility would be to try to use the tcl version of
msgfmt in the build of git itself if msgfmt is not available.
This way the build behavior of git and git-gui could be similar.

> If a given platform does not have gettext/msgfmt, then you just
> don't get an i18n-ed version of git. (no need for BYPASS_ ...).

Right now without gettext/msgfmt you get an error unless you set
NO_GETTEXT. If we try to use the tcl version of msgfmt in the build of
git itself, then you could still get an i18n-ed version of git if you
have Tcl/Tk.

But anyway even if this is related, I think it is a different issue.

> Am I missing something?

I still think it is not the best outcome to just install git-gui and
gitk by default when Tcl/Tk is not installed. In general it is best to
fix potential errors at build time rather than at run time (even if
the run time error is adequate).

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

[Jeff King]

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.

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

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

?

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

-Peff

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

[Christian Couder]

On Sun, Nov 26, 2017 at 8:15 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...
> I think your patch does say "consider setting NO_TCLTK" in that case,
> which is an improvement. 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).
>
> 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 was in agreement with that line of argument while the patch was
discussed, and I still think the above is true.

And if the primary thing we want to address with this patch is the
build breakage for those who do not have tclsh nor msgfmt, perhaps
it is a more direct fix to update this part in gitk/Makefile:

    ## po-file creation rules
    XGETTEXT   ?= xgettext
    ifdef NO_MSGFMT
            MSGFMT ?= $(TCL_PATH) po/po2msg.sh
    else
            MSGFMT ?= msgfmt
            ifneq ($(shell $(MSGFMT) --tcl -l C -d . /dev/null 2>/dev/null; echo $$?),0)
                    MSGFMT := $(TCL_PATH) po/po2msg.sh
            endif
    endif

There is an identical copy-and-pasted copy of this in
git-gui/Makefile, and both of them silently assume that TCL_PATH
points at something usable when msgfmt is not available.

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


Another reason why I think the Makefiles in gitk and git-gui
projects are better targets for the fix is because they are designed
to be independent projects.  We should be able to do

	cd git-gui && make

in our tree, but an update to our top-level Makefile does not help
those people.

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

[Junio C Hamano]

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.  

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.

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

[Jeff King]

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.

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

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

[Todd Zullinger]

Jeff King wrote:
> 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 either.  The general motivation there, as far as I understand, 
is that it's undesirable to have 'make install' install tools that 
cannot run.

Perhaps it's worth noting that there are other commands installed by 
default which won't work out of the box.  For example, 'git svn'
requires subversion at run time but not at build time.

If there aren't many such commands, then maybe checking for them in 
the Makefile is reasonable to make installing git from source easier 
for new users.  Without looking closely, I can't do more than take a 
wild guess.

As a package maintainer and an aging sysadmin, I'd bet that there are 
more dependencies than one might initially guess.  I also tend to 
think that by the time most users want something newer than their OS 
provides that they're a bit more able to deal with these sort of 
issues.  But I could easily be wrong on all counts.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
    -- Hunter S. Thompson

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

[Christian Couder]

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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

OK.

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

Is that controversial at all?  In the sense that it is addressing a
non-issue (i.e. it is perfectly fine if installed gitk fails at
runtime complaining that it cannot find wish), it might be, but to
me, it appears there isn't any room for controversy there.

But an out-of-box build that fails _is_ a problem worth addressing
to, so I view it primarily as the "how do we do msgfmt (or its
substitute that is good enough for tcl script)" issue.  Perhaps we
can export NO_GETTEXT from the top-level Makefile and have Makefiles
in these two subdirectories to pay attention to it, in addition to
NO_MSGFMT, and build them without i18n support instead of failing,
or something?

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

[Jeff King]

On Mon, Nov 27, 2017 at 09:24:47AM +0100, Christian Couder wrote:

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

Sorry, controversial may not have been the right word. It's just that
the review discussion focused around packagers who may want to build git
without having "wish" on the build machine. If everybody is happy with
the BYPASS mechanism you added to address that, then I'm perfectly fine
with it.

-Peff

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

[Jeff King]

On Sun, Nov 26, 2017 at 09:57:14PM +0100, Christian Couder wrote:

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

I guess I can buy that line of reasoning (and in particular your later
comment that autoconf is doomed to bitrot, since most of us don't use
it). I'm just slightly concerned that we're going to end up
reimplementing autoconf inside our Makefile. ;)

But that is a slippery slope argument; we could switch our approach any
time if it starts to look bad.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> without having "wish" on the build machine. If everybody is happy with
> the BYPASS mechanism you added to address that, then I'm perfectly fine
> with it.

OK.  The topic was queued on 'pu' yesterday; lets move it forward
after waiting for a few more days to see if there are further
improvements and/or objections.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> without having "wish" on the build machine. If everybody is happy with
>> the BYPASS mechanism you added to address that, then I'm perfectly fine
>> with it.
>
> OK.  The topic was queued on 'pu' yesterday; lets move it forward
> after waiting for a few more days to see if there are further
> improvements and/or objections.

It seems that TravisCI objects ;-)

    https://travis-ci.org/git/git/jobs/307745929

[PATCH] travis-ci: avoid new tcl/tk build requirement

[Todd Zullinger]

A build requirement on tcl/tk was added in 01c54284f1 (Makefile: check
that tcl/tk is installed, 2017-11-20).  For building and running the
tests, we don't need tcl/tk installed.  Bypass the requirement.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Junio C Hamano wrote:
> It seems that TravisCI objects ;-)
>
>    https://travis-ci.org/git/git/jobs/307745929

Interesting that the main builds passed.  I don't know what the default
64-bit linuxinstall looks like in travis, so I presume it includes
tcl/tk or something.

In any case, perhaps something like this is what we want?  We could use
NO_TCLTK or ensure that tcl/tk is installed in all environments.  I used
the BYPASS_TCLTK_CHECK option since the tests have been running without
tcl/tk previously.  If they become required for the tests, this can be
adjusted.

I have a travis job running with this change here:

    https://travis-ci.org/tmzullinger/git/builds/308452464

So far the only failure is (what looks like) an unrelated one in the
GETTEXT_POISON build.

 .travis.yml              | 1 +
 ci/run-linux32-docker.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 281f101f31..9e57caa83d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -23,6 +23,7 @@ addons:
 
 env:
   global:
+    - BYPASS_TCLTK_CHECK=1
     - DEVELOPER=1
     # The Linux build installs the defined dependency versions below.
     # The OS X build installs the latest available versions. Keep that
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 0edf63acfa..8c2b32f7b3 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -13,6 +13,7 @@ docker pull daald/ubuntu32:xenial
 
 docker run \
 	--interactive \
+	--env BYPASS_TCLTK_CHECK \
 	--env DEVELOPER \
 	--env DEFAULT_TEST_TARGET \
 	--env GIT_PROVE_OPTS \
-- 
2.15.0

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Christian Couder]

On Tue, Nov 28, 2017 at 3:37 PM, Todd Zullinger <tmz@pobox.com> wrote:
> A build requirement on tcl/tk was added in 01c54284f1 (Makefile: check
> that tcl/tk is installed, 2017-11-20).  For building and running the
> tests, we don't need tcl/tk installed.  Bypass the requirement.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
>
> Junio C Hamano wrote:
>> It seems that TravisCI objects ;-)
>>
>>    https://travis-ci.org/git/git/jobs/307745929
>
> Interesting that the main builds passed.  I don't know what the default
> 64-bit linuxinstall looks like in travis, so I presume it includes
> tcl/tk or something.

Yeah, interesting. I am cc'ing Lars who perhaps knows.

> In any case, perhaps something like this is what we want?  We could use
> NO_TCLTK or ensure that tcl/tk is installed in all environments.  I used
> the BYPASS_TCLTK_CHECK option since the tests have been running without
> tcl/tk previously.  If they become required for the tests, this can be
> adjusted.
>
> I have a travis job running with this change here:
>
>     https://travis-ci.org/tmzullinger/git/builds/308452464
>
> So far the only failure is (what looks like) an unrelated one in the
> GETTEXT_POISON build.

Yeah, I can't see how test failures in the t/ directory could be related.

>  .travis.yml              | 1 +
>  ci/run-linux32-docker.sh | 1 +
>  2 files changed, 2 insertions(+)

The patch looks good to me. Thanks!

I wonder if it would be better to squash it into my patch or to keep
it separate. I am ok with both ways.

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Todd Zullinger]

Christian Couder wrote:
>> Junio C Hamano wrote:
>>> It seems that TravisCI objects ;-)
>>>
>>>    https://travis-ci.org/git/git/jobs/307745929
>>
>> Interesting that the main builds passed.  I don't know what the default
>> 64-bit linuxinstall looks like in travis, so I presume it includes
>> tcl/tk or something.
>
> Yeah, interesting. I am cc'ing Lars who perhaps knows.

I pulled the travis docker image used in clang/gcc builds[1] and can
see it has both tcl and tk packages installed.  The linux32 builds use
a docker image[2] which does not contain tcl or tk.

[1] travisci/ci-garnet:packer-1503972846
[2] daald/ubuntu32:xenial

If we wanted, we could set BYPASS_TCLTK_CHECK only for the linux32
builds.  I think it's probably better to do it globally rather than
rely on the travis containers implicitly having tcl/tk installed.

> The patch looks good to me. Thanks!
>
> I wonder if it would be better to squash it into my patch or to keep
> it separate. I am ok with both ways.

I fine either way too.  This is still just on pu, so squashing it in
seems like the way to go.  I only made it separate to cause travis to
run the tests. :)

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In the beginning the Universe was created. This has made a lot of
people very angry and has been widely regarded as a bad move.
    -- Douglas Adams

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> I pulled the travis docker image used in clang/gcc builds[1] and can
> see it has both tcl and tk packages installed.  The linux32 builds use
> a docker image[2] which does not contain tcl or tk.
>
> [1] travisci/ci-garnet:packer-1503972846
> [2] daald/ubuntu32:xenial
>
> If we wanted, we could set BYPASS_TCLTK_CHECK only for the linux32
> builds.  I think it's probably better to do it globally rather than
> rely on the travis containers implicitly having tcl/tk installed.
>
>> The patch looks good to me. Thanks!
>>
>> I wonder if it would be better to squash it into my patch or to keep
>> it separate. I am ok with both ways.
>
> I fine either way too.  This is still just on pu, so squashing it in
> seems like the way to go.  I only made it separate to cause travis to
> run the tests. :)

I am OK either way but this may be giving an early warning that a
typical mix of packages installed on a box will lead many people to
encounter this glitch in the real world, and we may need a more
seamless solution than a workaround.

Keeping this "fix" separate from the original commit that introduced
the issue would allow us more easily to refer real users who will be
hit by this glitch to it, telling them to do something similar to
their build environment.

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

[Christian Couder]

On Fri, Nov 17, 2017 at 6:42 PM, Todd Zullinger <tmz@pobox.com> wrote:
> Christian Couder wrote:
>>
>> On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> I suspect that this change will hurt those who package Git for other
>>> people.
>>
>>
>> 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.
>
> It's not a big burden, but it is a seemingly unnecessary build-time
> dependency.
>
>>> 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.
>>
>> 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.
>
> 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.

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

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

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.

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

Yeah I agree.

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

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.

> 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

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

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

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.

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

[Christian Couder]

On Fri, Nov 17, 2017 at 11:02 PM, Jeff King <peff@peff.net> wrote:
>
> 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. ;)

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.

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

[Jeff King]

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

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

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

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

[Todd Zullinger]

Christian Couder wrote:
> On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I suspect that this change will hurt those who package Git for other 
>> people.
>
> 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.

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

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

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.

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

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

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

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

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

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:

diff --git i/gitk-git/gitk w/gitk-git/gitk
index a14d7a16b2..f9f28a164a 100755
--- i/gitk-git/gitk
+++ w/gitk-git/gitk
@@ -1,5 +1,6 @@
 #!/bin/sh
-# Tcl ignores the next line -*- tcl -*- \
+# Tcl ignores the next 2 lines -*- tcl -*- \
+type wish >/dev/null 2>&1 || { echo "error: wish not found" >&2; exit 1; }; \
 exec wish "$0" -- "$@"
 
 # Copyright © 2005-2016 Paul Mackerras.  All rights reserved.

maybe?  (The error message is certainly open for improvement.)

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A fool's brain digests philosophy into folly, science into
superstition, and art into pedantry.  Hence University education.
    -- George Bernard Shaw

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

[Christian Couder]

On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> 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.
>
> 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.

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.

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

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

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.

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

[Junio C Hamano]

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

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

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?

> diff --git a/Makefile b/Makefile
> index ee9d5eb11e..ada6164e15 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1636,6 +1636,13 @@ ifeq ($(TCLTK_PATH),)
>  NO_TCLTK = NoThanks
>  endif
>  
> +ifndef NO_TCLTK
> +	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
> +
>  ifeq ($(PERL_PATH),)
>  NO_PERL = NoThanks
>  endif

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

[Christian Couder]

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(+)

diff --git a/Makefile b/Makefile
index ee9d5eb11e..ada6164e15 100644
--- a/Makefile
+++ b/Makefile
@@ -1636,6 +1636,13 @@ ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
 
+ifndef NO_TCLTK
+	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
+
 ifeq ($(PERL_PATH),)
 NO_PERL = NoThanks
 endif
-- 
2.15.0.165.g42c5887