Junio C Hamano [off-list ref] 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.
On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
quoted
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
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
On Mon, Nov 27, 2017 at 5:35 AM, Jeff King [off-list ref] wrote:
On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote:
quoted
Junio C Hamano [off-list ref] writes:
quoted
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.
quoted
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.
On Mon, Nov 27, 2017 at 09:24:47AM +0100, Christian Couder wrote:
quoted
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