On Mon, Nov 27, 2017 at 5:35 AM, Jeff King <peff@xxxxxxxx> wrote: > On Mon, Nov 27, 2017 at 01:31:13PM +0900, Junio C Hamano wrote: > >> Junio C Hamano <gitster@xxxxxxxxx> 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.
