On Fri, Nov 17, 2017 at 6:42 PM, Todd Zullinger <tmz@xxxxxxxxx> wrote: > Christian Couder wrote: >> >> On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx 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.
