threads / patch / 8183

patchgit-gui: Build even if tclsh is not available

Subject: [PATCH] git-gui: Build even if tclsh is not available

## tl;dr

6 messages between May 17, 2007 and May 17, 2007. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Petr Baudis· May 17, 2007, 02:06 UTC · lore

As of now, git fails to build with default config if tclsh is not available, thus requiring manual config tweaking just because of some optimizations done at build time; this is a needless hassle when building git on any kind of a server.

This patch makes git-gui's build process to handle this gracefully; unless TCL_PATH is explicitly set, if tclsh cannot be executed only a warning is printed and the build goes on. I have tested this only on that server with no tclsh, but hopefully it shouldn't break the build process with tclsh available either.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---
 Makefile         |    8 +++++---
 git-gui/Makefile |    2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 2 files +6 −4

Makefile, git-gui/Makefile

diff --git a/Makefile b/Makefile
index 07411ff..ed12577 100644
--- a/Makefile
+++ b/Makefile
@@ -178,10 +178,12 @@ AR = ar
 TAR = tar
 INSTALL = install
 RPMBUILD = rpmbuild
-TCL_PATH = tclsh
-TCLTK_PATH = wish
 
-export TCL_PATH TCLTK_PATH
+# If TCL_PATH is not defined here, it will default to tclsh later
+# with the exception that if tclsh cannot be executed, the optimization
+# step is skipped.
+# export TCL_PATH = tclsh
+export TCLTK_PATH = wish
 
 # sparse is architecture-neutral, which means that we need to tell it
 # explicitly what architecture to check for. Fix this up for yours..
diff --git a/git-gui/Makefile b/git-gui/Makefile
index e73b645..2b1bc18 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -34,7 +34,7 @@ ifndef V
 	QUIET_INDEX    = @echo '   ' INDEX $(dir $@);
 endif
 
-TCL_PATH   ?= tclsh
+TCL_PATH   ?= tclsh || echo "Warning: Cannot execute tclsh, not optimizing git-gui" >&2
 TCLTK_PATH ?= wish
 
 ifeq ($(findstring $(MAKEFLAGS),s),s)
Petr Baudis· May 17, 2007, 02:14 UTC · re: Petr Baudis · lore

As of now, git fails to build with default config if tclsh is not available, thus requiring manual config tweaking just because of some optimizations done at build time; this is a needless hassle when building git on any kind of a server.

This patch makes git-gui's build process to handle this gracefully; unless TCL_PATH is explicitly set, if tclsh cannot be executed only a warning is printed and the build goes on. I have tested this only on that server with no tclsh, but hopefully it shouldn't break the build process with tclsh available either.

Version 2, sorry - the previous version of the patch had the install hunk missing. And apologies for the duplicate submission... :-)

Signed-off-by: Petr Baudis <pasky@suse.cz>
---
 Makefile         |    8 +++++---
 git-gui/Makefile |    4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)
Show changes to 2 files +7 −5

Makefile, git-gui/Makefile

diff --git a/Makefile b/Makefile
index 07411ff..ed12577 100644
--- a/Makefile
+++ b/Makefile
@@ -178,10 +178,12 @@ AR = ar
 TAR = tar
 INSTALL = install
 RPMBUILD = rpmbuild
-TCL_PATH = tclsh
-TCLTK_PATH = wish
 
-export TCL_PATH TCLTK_PATH
+# If TCL_PATH is not defined here, it will default to tclsh later
+# with the exception that if tclsh cannot be executed, the optimization
+# step is skipped.
+# export TCL_PATH = tclsh
+export TCLTK_PATH = wish
 
 # sparse is architecture-neutral, which means that we need to tell it
 # explicitly what architecture to check for. Fix this up for yours..
diff --git a/git-gui/Makefile b/git-gui/Makefile
index e73b645..be3cfbb 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -34,7 +34,7 @@ ifndef V
 	QUIET_INDEX    = @echo '   ' INDEX $(dir $@);
 endif
 
-TCL_PATH   ?= tclsh
+TCL_PATH   ?= tclsh || echo "Warning: Cannot execute tclsh, not optimizing git-gui" >&2
 TCLTK_PATH ?= wish
 
 ifeq ($(findstring $(MAKEFLAGS),s),s)
@@ -92,7 +92,7 @@ install: all
 	$(INSTALL) git-gui '$(DESTDIR_SQ)$(gitexecdir_SQ)'
 	$(foreach p,$(GITGUI_BUILT_INS), rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;)
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(libdir_SQ)'
-	$(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
+	[ ! -e lib/tclIndex ] || $(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
 	$(foreach p,$(ALL_LIBFILES), $(INSTALL) -m644 $p '$(DESTDIR_SQ)$(libdir_SQ)' ;)
 
 dist-version:
Shawn O. Pearce· May 17, 2007, 02:18 UTC · re: Petr Baudis · lore

Re: [PATCH] git-gui: Build even if tclsh is not available

Petr Baudis <pasky@suse.cz> wrote:
Show 20 quoted lines
> As of now, git fails to build with default config if tclsh is not
> available, thus requiring manual config tweaking just because of some
> optimizations done at build time; this is a needless hassle when building
> git on any kind of a server.
> 
> This patch makes git-gui's build process to handle this gracefully; unless
> TCL_PATH is explicitly set, if tclsh cannot be executed only a warning is
> printed and the build goes on. I have tested this only on that server with
> no tclsh, but hopefully it shouldn't break the build process with tclsh
> available either.
> 
> Version 2, sorry - the previous version of the patch had the install hunk
> missing. And apologies for the duplicate submission... :-)
> 
> Signed-off-by: Petr Baudis <pasky@suse.cz>
> ---
> 
>  Makefile         |    8 +++++---
>  git-gui/Makefile |    4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)

I have a couple of problems with the patch as-is. The first is of course that the patch needs to be split into two; one patch for the git-gui subdirectory itself and one for git.git.

My other problem is
Show 8 quoted lines
>  ifeq ($(findstring $(MAKEFLAGS),s),s)
> @@ -92,7 +92,7 @@ install: all
>  	$(INSTALL) git-gui '$(DESTDIR_SQ)$(gitexecdir_SQ)'
>  	$(foreach p,$(GITGUI_BUILT_INS), rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;)
>  	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(libdir_SQ)'
> -	$(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
> +	[ ! -e lib/tclIndex ] || $(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
>  	$(foreach p,$(ALL_LIBFILES), $(INSTALL) -m644 $p '$(DESTDIR_SQ)$(libdir_SQ)' ;)

git-gui won't work if lib/tclIndex is missing or invalid. So not installing it means we should just disable git-gui entirely.

-- 
Shawn.
Petr Baudis· May 17, 2007, 02:36 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] git-gui: Build even if tclsh is not available

On Thu, May 17, 2007 at 04:18:58AM CEST, Shawn O. Pearce wrote:
> I have a couple of problems with the patch as-is.  The first is
> of course that the patch needs to be split into two; one patch for
> the git-gui subdirectory itself and one for git.git.

Hmm, why? It's an atomic change, one part doesn't make sense without the other.

Show 13 quoted lines
> My other problem is 
> 
> >  ifeq ($(findstring $(MAKEFLAGS),s),s)
> > @@ -92,7 +92,7 @@ install: all
> >  	$(INSTALL) git-gui '$(DESTDIR_SQ)$(gitexecdir_SQ)'
> >  	$(foreach p,$(GITGUI_BUILT_INS), rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;)
> >  	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(libdir_SQ)'
> > -	$(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
> > +	[ ! -e lib/tclIndex ] || $(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)'
> >  	$(foreach p,$(ALL_LIBFILES), $(INSTALL) -m644 $p '$(DESTDIR_SQ)$(libdir_SQ)' ;)
> 
> git-gui won't work if lib/tclIndex is missing or invalid.  So not
> installing it means we should just disable git-gui entirely.
Aha, ouch - I understood that it is only an optimization. :-(
So AIUI, there are several possibilities:
  (i) Makefile will autodecide on whether git-gui will be
built+installed or not
  (ii) ./configure will, people not using configure and building on
servers will be left to tweak config manually
  (iii) ./configure will, git-gui will default to not to be built and
people not using configure and wanting git-gui will be left to tweak
config manually

I suspect that (ii) will be chosen, and even though I don't like it *personally* I guess it's the most reasonable approach for the general public. I didn't know that tclIndex is vital for git-gui when I submitted the patch, the /Makefile comment suggests otherwise.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett
Shawn O. Pearce· May 17, 2007, 02:49 UTC · re: Petr Baudis · lore

Re: [PATCH] git-gui: Build even if tclsh is not available

Petr Baudis <pasky@suse.cz> wrote:
Show 7 quoted lines
> On Thu, May 17, 2007 at 04:18:58AM CEST, Shawn O. Pearce wrote:
> > I have a couple of problems with the patch as-is.  The first is
> > of course that the patch needs to be split into two; one patch for
> > the git-gui subdirectory itself and one for git.git.
> 
> Hmm, why? It's an atomic change, one part doesn't make sense without the
> other.

Because git-gui is actually a project maintained external from git.git. It just happens that Junio pulls various versions of it into git.git to distribute it along with git.git releases.

I deal with these sorts of "atomic changes" by making an evil merge
in git.git and asking Junio to pull the evil merge.  But we've only
had one such case thus far.
Show 12 quoted lines
> > git-gui won't work if lib/tclIndex is missing or invalid.  So not
> > installing it means we should just disable git-gui entirely.
> 
>   (i) Makefile will autodecide on whether git-gui will be
> built+installed or not
> 
>   (ii) ./configure will, people not using configure and building on
> servers will be left to tweak config manually
> 
>   (iii) ./configure will, git-gui will default to not to be built and
> people not using configure and wanting git-gui will be left to tweak
> config manually

(iv) if tclsh is not available then create a simpler lib/tclIndex that loads all of the lib directory, even if it isn't needed. That makes tclsh being available strictly an optimization, and yet git-gui still is installable.

-- 
Shawn.
Shawn O. Pearce· May 17, 2007, 22:16 UTC · re: Petr Baudis · lore

Re: [PATCH] git-gui: Build even if tclsh is not available

Petr Baudis <pasky@suse.cz> wrote:
Show 14 quoted lines
>   (i) Makefile will autodecide on whether git-gui will be
> built+installed or not
> 
>   (ii) ./configure will, people not using configure and building on
> servers will be left to tweak config manually
> 
>   (iii) ./configure will, git-gui will default to not to be built and
> people not using configure and wanting git-gui will be left to tweak
> config manually
> 
> I suspect that (ii) will be chosen, and even though I don't like it
> *personally* I guess it's the most reasonable approach for the general
> public. I didn't know that tclIndex is vital for git-gui when I
> submitted the patch, the /Makefile comment suggests otherwise.

(iv) git-gui 0.7.1, which is now available from repo.or.cz, and I think you know where that is ;-), makes TCL_PATH strictly an optimization at compile time.

If TCL_PATH is present and creates a valid tclIndex file then we use the Tcl auto_load "optimization" to only load the Tcl code we actually need when we run. But if it does fail for any reason we hide the error (can be unhidden with make V=1) and we generate a listing of the files instead. In this latter case we load *everything* on git-gui startup.

This way git-gui works for the user either way, just like before, but its startup may be slightly slower if the user didn't give us a good TCL_PATH at build time.

Junio, would you consider merging 0.7.1 soon? No changes are needed to git.git's own Makefile, just a subtree pull.

-- 
Shawn.

← back to recent threads