Re: [PATCH] Added configure options --with-tcltk/--without-tcltk.

Eygene Ryabinkin [off-list ref] writes:

Configure's options --with-tcltk and --without-tcltk were added and
configure script teached to search for the Tcl/Tk interpreter.

The default behaviour to install Tcl/Tk dependant parts is left
intact: Tcl/Tk detection will be enabled only if --with-tcltk option
is given to configure.

Makefiles got two external options:
- TCLTK_PATH: the path to the Tcl/Tk interpreter.
- NO_TCLCK: disables the installation of Tcl/Tk dependend parts.

Signed-off-by: Eygene Ryabinkin <redacted>

Thanks.

This seems to contradict the log message that makes these two
options sound as if they are not dependent of each other.

This is a no-no. "make $args; su make $args install" should
never cause anything built by root with the second invocation of
the make command.  Don't assume you can write into the build
directory while you are running "make install" (root user can be
mapped nobody on a nfs mounted build directory, while the local
target directory is writable by it).

Also please quote $(TCLTK_PATH) like everybody else does in the
Makefile.  For that purpose, I think the way $(SCRIPT_SH) are
built using $(SHELL_PATH_SQ) can be learned from.

I suspect that the change to allow not installing gitk/git-gui
and the change to allow using specific "wish" are two
independent tasks.  You seem to have a grip on the use of
conditional in Makefile to do the former task, and I do not
think there is any need for further commenting.

For the latter task, you can probably do something like this:

	gitk-wish: gitk 
        	rm -f $@+ $@
		sed -e '3s| wish | ...' <gitk >$@+
                mv $@+ $@

	all:: gitk-wish
	install: all
        	...
                $(INSTALL) gitk-wish '$(DESTDIR_SQ)$(bindir_SQ)'/gitk

Also you need to rebuild gitk-wish when the builder gives
different TCLTK_PATH; I suspect the easiest way is tack it to
TRACK_CFLAGS and make gitk-wish depend on GIT-CFLAGS.

Junio, good day.

Tue, Mar 27, 2007 at 03:53:33AM -0700, Junio C Hamano wrote:

quoted

+ifeq ($(TCLTK_PATH),)
+NO_TCLTK=YesPlease
+endif
+

This seems to contradict the log message that makes these two
options sound as if they are not dependent of each other.

OK, will add a sentence about the dependency to the log.

quoted

@@ -918,10 +930,16 @@ install: all
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
-	$(INSTALL) git$X gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) git$X '$(DESTDIR_SQ)$(bindir_SQ)'
+ifndef NO_TCLTK
+	sed -i .bak -e'1,3s|^exec .* "$$0"|exec '"$(TCLTK_PATH)"' "$$0"|' gitk && rm -f gitk.bak
+	$(INSTALL) gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+endif

This is a no-no. "make $args; su make $args install" should
never cause anything built by root with the second invocation of
the make command.  Don't assume you can write into the build
directory while you are running "make install" (root user can be
mapped nobody on a nfs mounted build directory, while the local
target directory is writable by it).
Also please quote $(TCLTK_PATH) like everybody else does in the
Makefile.  For that purpose, I think the way $(SCRIPT_SH) are
built using $(SHELL_PATH_SQ) can be learned from.

OK.

I suspect that the change to allow not installing gitk/git-gui
and the change to allow using specific "wish" are two
independent tasks.

But then the configure will be first teached to recognise only
'--with-tcltk/--without-tcltk' and the second modification will
add '--with-tcltk=/path/to/wish', right?

You seem to have a grip on the use of
conditional in Makefile to do the former task, and I do not
think there is any need for further commenting.

For the latter task, you can probably do something like this:

	gitk-wish: gitk 
        	rm -f $@+ $@
		sed -e '3s| wish | ...' <gitk >$@+
                mv $@+ $@

	all:: gitk-wish
	install: all
        	...
                $(INSTALL) gitk-wish '$(DESTDIR_SQ)$(bindir_SQ)'/gitk

Also you need to rebuild gitk-wish when the builder gives
different TCLTK_PATH; I suspect the easiest way is tack it to
TRACK_CFLAGS and make gitk-wish depend on GIT-CFLAGS.

OK, will try to provide the splitted patches.
-- 
Eygene

Eygene Ryabinkin [off-list ref] writes:

quoted

I suspect that the change to allow not installing gitk/git-gui
and the change to allow using specific "wish" are two
independent tasks.

But then the configure will be first teached to recognise only
'--with-tcltk/--without-tcltk' and the second modification will
add '--with-tcltk=/path/to/wish', right?

That sounds sensible to me.

Thanks.

--with-tcltk enables the search of the Tcl/Tk interpreter. If no
interpreter is found then Tcl/Tk dependend parts are disabled.

--without-tcltk unconditionally disables Tcl/Tk dependent parts.

The original behaviour is not changed: bare './configure' just
installs the Tcl/Tk part doing no checks for the interpreter.

Makefile knob named NO_TCLTK was introduced. It prevents the build
and installation of the Tcl/Tk dependent parts.

Signed-off-by: Eygene Ryabinkin <redacted>
---
 Makefile         |   31 +++++++++++++++++++++++++++++--
 config.mak.in    |    1 +
 configure.ac     |   26 ++++++++++++++++++++++++++
 git-gui/Makefile |    3 +++
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index a294ec8..bfde029 100644
--- a/Makefile
+++ b/Makefile

@@ -112,6 +112,12 @@ all::
 #
 # Define WITH_P4IMPORT to build and install Python git-p4import script.
 #
+# Define NO_TCLTK if you do not want Tcl/Tk GUI.
+#
+# The TCLTK_PATH variable governs the location of the Tck/Tk interpreter.
+# 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).
+#
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -161,6 +167,7 @@ AR = ar
 TAR = tar
 INSTALL = install
 RPMBUILD = rpmbuild
+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..

@@ -624,6 +631,10 @@ ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
 
+ifeq ($(TCLTK_PATH),)
+NO_TCLTK=YesPlease
+endif
+
 QUIET_SUBDIR0  = $(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 

@@ -663,6 +674,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
+TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 

@@ -684,7 +696,9 @@ ifneq (,$X)
 endif
 
 all::
+ifndef NO_TCLTK
 	$(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) all
+endif
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' all
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1)
 

@@ -918,10 +932,15 @@ install: all
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
-	$(INSTALL) git$X gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) git$X '$(DESTDIR_SQ)$(bindir_SQ)'
+ifndef NO_TCLTK
+	$(INSTALL) gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+endif
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C perl prefix='$(prefix_SQ)' install
-	$(MAKE) -C git-gui install
+ifndef NO_TCLTK
+	$(MAKE) -C git-gui TCLTK_PATH='$(TCLTK_PATH_SQ)' install
+endif
 	if test 'z$(bindir_SQ)' != 'z$(gitexecdir_SQ)'; \
 	then \
 		ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \

@@ -955,11 +974,17 @@ dist: git.spec git-archive
 	@mkdir -p $(GIT_TARNAME)
 	@cp git.spec $(GIT_TARNAME)
 	@echo $(GIT_VERSION) > $(GIT_TARNAME)/version
+ifndef NO_TCLTK
 	@$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
 	$(TAR) rf $(GIT_TARNAME).tar \
 		$(GIT_TARNAME)/git.spec \
 		$(GIT_TARNAME)/version \
 		$(GIT_TARNAME)/git-gui/version
+else
+	$(TAR) rf $(GIT_TARNAME).tar \
+		$(GIT_TARNAME)/git.spec \
+		$(GIT_TARNAME)/version
+endif
 	@rm -rf $(GIT_TARNAME)
 	gzip -f -9 $(GIT_TARNAME).tar
 

@@ -1000,7 +1025,9 @@ clean:
 	rm -f gitweb/gitweb.cgi
 	$(MAKE) -C Documentation/ clean
 	$(MAKE) -C perl clean
+ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
+endif
 	$(MAKE) -C templates/ clean
 	$(MAKE) -C t/ clean
 	rm -f GIT-VERSION-FILE GIT-CFLAGS

diff --git a/config.mak.in b/config.mak.in
index 9a57840..eb9d7a5 100644
--- a/config.mak.in
+++ b/config.mak.in

@@ -6,6 +6,7 @@ CFLAGS = @CFLAGS@
 AR = @AR@
 TAR = @TAR@
 #INSTALL = @INSTALL@		# needs install-sh or install.sh in sources
+TCLTK_PATH = @TCLTK_PATH@
 
 prefix = @prefix@
 exec_prefix = @exec_prefix@

diff --git a/configure.ac b/configure.ac
index 3a8e778..43a6769 100644
--- a/configure.ac
+++ b/configure.ac

@@ -75,6 +75,14 @@ GIT_ARG_SET_PATH(shell)
 # Define PERL_PATH to provide path to Perl.
 GIT_ARG_SET_PATH(perl)
 #
+# Declare the with-tcltk/without-tcltk options.
+AC_ARG_WITH(tcltk,
+AS_HELP_STRING([--with-tcltk],[use Tcl/Tk GUI (default is YES)])
+AS_HELP_STRING([],[ARG is the full path to the Tcl/Tk interpreter.])
+AS_HELP_STRING([],[Bare --with-tcltk will make the GUI part only if])
+AS_HELP_STRING([],[Tcl/Tk interpreter will be found in a system.]),\
+GIT_PARSE_WITH(tcltk))
+#
 
 
 ## Checks for programs.

@@ -84,6 +92,24 @@ AC_PROG_CC([cc gcc])
 #AC_PROG_INSTALL		# needs install-sh or install.sh in sources
 AC_CHECK_TOOL(AR, ar, :)
 AC_CHECK_PROGS(TAR, [gtar tar])
+# TCLTK_PATH will be set to some value if we want Tcl/Tk
+# or will be empty otherwise.
+if test -z "$NO_TCLTK"; then
+  if test "$with_tcltk" = ""; then
+  # No Tcl/Tk switches given. Do not check for Tcl/Tk, use bare 'wish'.
+    TCLTK_PATH=wish
+    AC_SUBST(TCLTK_PATH)
+  elif test "$with_tcltk" = "yes"; then
+  # Tcl/Tk check requested.
+    AC_CHECK_PROGS(TCLTK_PATH, [wish], )
+  elif ! test -x "$with_tcltk"; then
+    AC_MSG_ERROR([Tcl/Tk interpreter was not found in $with_tcltk])
+  else
+    AC_MSG_RESULT([Using Tcl/Tk interpreter $with_tcltk])
+    TCLTK_PATH="$with_tcltk"
+    AC_SUBST(TCLTK_PATH)
+  fi
+fi
 
 ## Checks for libraries.
 AC_MSG_NOTICE([CHECKS for libraries])

diff --git a/git-gui/Makefile b/git-gui/Makefile
index b82789e..733c07e 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile

@@ -28,6 +28,8 @@ ifndef V
 	QUIET_BUILT_IN = @echo '   ' BUILTIN $@;
 endif
 
+TCLTK_PATH ?= wish
+
 ifeq ($(findstring $(MAKEFLAGS),s),s)
 QUIET_GEN =
 QUIET_BUILT_IN =

@@ -36,6 +38,7 @@ endif
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	$(QUIET_GEN)rm -f $@ $@+ && \

-- 
1.5.0.3-dirty

--with-tcltk=/path/to/wish sets the TCLTK_PATH variable that is
used to substitute the location of the wish interpreter in the
Tcl/Tk programs.

New tracking file, GIT-GUI-VARS, was introduced: it tracks the
location of the Tcl/Tk interpreter and activates the GUI tools
rebuild if the interpreter path was changed. The separate tracker
is better than the GIT-CFLAGS: there is no need to rebuild the whole
git if the interpreter path was changed.

Signed-off-by: Eygene Ryabinkin <redacted>
---
 Makefile         |   31 ++++++++++++++++++++++++++++---
 git-gui/Makefile |   17 +++++++++++++++--
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index bfde029..0034666 100644
--- a/Makefile
+++ b/Makefile

@@ -690,11 +690,15 @@ export prefix gitexecdir TAR INSTALL DESTDIR SHELL_PATH template_dir
 
 ### Build rules
 
-all:: $(ALL_PROGRAMS) $(BUILT_INS) git$X gitk gitweb/gitweb.cgi
+all:: $(ALL_PROGRAMS) $(BUILT_INS) git$X gitweb/gitweb.cgi
 ifneq (,$X)
 	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), rm -f '$p';)
 endif
 
+ifndef NO_TCLTK
+all:: gitk-wish
+endif
+
 all::
 ifndef NO_TCLTK
 	$(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) all

@@ -705,6 +709,12 @@ endif
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
+gitk-wish: gitk GIT-GUI-VARS
+	$(QUIET_GEN)rm -f $@ $@+ && \
+	sed -e'1,3s|^exec .* "$$0"|exec $(subst |,'\|',$(TCLTK_PATH_SQ)) "$$0"|' < gitk > $@+ && \
+	chmod +x $@+ && \
+	mv -f $@+ $@
+
 git$X: git.c common-cmds.h $(BUILTIN_OBJS) $(GITLIBS) GIT-CFLAGS
 	$(QUIET_LINK)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
 		$(ALL_CFLAGS) -o $@ $(filter %.c,$^) \

@@ -892,6 +902,18 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 		echo "$$FLAGS" >GIT-CFLAGS; \
             fi
 
+### Detect Tck/Tk interpreter path changes
+ifndef NO_TCLTK
+TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
+
+GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+	@VARS='$(TRACK_VARS)'; \
+	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
+		echo 1>&2 "    * new Tcl/Tk interpreter location"; \
+		echo "$$VARS" >$@; \
+            fi
+endif
+
 ### Testing rules
 
 # GNU make supports exporting all variables by "export" without parameters.

@@ -934,7 +956,7 @@ install: all
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
 	$(INSTALL) git$X '$(DESTDIR_SQ)$(bindir_SQ)'
 ifndef NO_TCLTK
-	$(INSTALL) gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) gitk-wish '$(DESTDIR_SQ)$(bindir_SQ)'/gitk
 endif
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C perl prefix='$(prefix_SQ)' install

@@ -1030,10 +1052,13 @@ ifndef NO_TCLTK
 endif
 	$(MAKE) -C templates/ clean
 	$(MAKE) -C t/ clean
-	rm -f GIT-VERSION-FILE GIT-CFLAGS
+	rm -f GIT-VERSION-FILE GIT-CFLAGS GIT-GUI-VARS
 
 .PHONY: all install clean strip
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags .FORCE-GIT-CFLAGS
+ifndef NO_TCLTK
+.PHONY: .FORCE-GIT-GUI-VARS
+endif
 
 ### Check documentation
 #

diff --git a/git-gui/Makefile b/git-gui/Makefile
index 733c07e..4ef6fac 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile

@@ -40,10 +40,13 @@ gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 
+git-gui: GIT-GUI-VARS
+
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	$(QUIET_GEN)rm -f $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 		-e 's/@@GITGUI_VERSION@@/$(GITGUI_VERSION)/g' \
+		-e'1,3s|^exec .* "$$0"|exec $(subst |,'\|',$(TCLTK_PATH_SQ)) "$$0"|' \
 		$@.sh >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@

@@ -66,7 +69,17 @@ dist-version:
 	@echo $(GITGUI_VERSION) > $(TARDIR)/version
 
 clean::
-	rm -f $(ALL_PROGRAMS) GIT-VERSION-FILE
+	rm -f $(ALL_PROGRAMS) GIT-VERSION-FILE GIT-GUI-VARS
+
+### Detect Tck/Tk interpreter path changes
+TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
+
+GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+	@VARS='$(TRACK_VARS)'; \
+	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
+		echo 1>&2 "    * new Tcl/Tk interpreter location"; \
+		echo "$$VARS" >$@; \
+            fi
 
 .PHONY: all install dist-version clean
-.PHONY: .FORCE-GIT-VERSION-FILE
+.PHONY: .FORCE-GIT-VERSION-FILE .FORCE-GIT-GUI-VARS

-- 
1.5.0.3-dirty

Eygene Ryabinkin [off-list ref] writes:

--with-tcltk enables the search of the Tcl/Tk interpreter. If no
interpreter is found then Tcl/Tk dependend parts are disabled.

--without-tcltk unconditionally disables Tcl/Tk dependent parts.

The original behaviour is not changed: bare './configure' just
installs the Tcl/Tk part doing no checks for the interpreter.

Makefile knob named NO_TCLTK was introduced. It prevents the build
and installation of the Tcl/Tk dependent parts.

Signed-off-by: Eygene Ryabinkin <redacted>
---

Thanks.

Is this supposed to be the first in the series?  I thought you
said you were going to do NO_TCLTK without anything else at all
first, and then TCLTK_PATH patch.  I am a bit lost here.

+# Define NO_TCLTK if you do not want Tcl/Tk GUI.
+#
+# The TCLTK_PATH variable governs the location of the Tck/Tk interpreter.
+# 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).
+#

Grumble.  If you are doing this, then there is not much point to
have two separate patches, is it?

Although you were not supposed to be talking about paths, since
you've already introduced TCLTK_PATH, it should be passed down
to git-gui here, I think.

Why should a source distribution exclude git-gui/ directory?  I
think it is sensible to ship a source that contains all.  You
are shipping gitk even without NO_TCLTK anyway, too.

And from the part 2:

This subst() is a nice attention to the detail.  I like it,
although in practice I do not think anybody is insane enough to
have a pipe character in the directory name that leads to wish.

I separated your two patches into three with minor modifications
and parked them in 'pu'.  We need to arrange with Shawn when to
apply the git-gui/ parts of the patch to his tree, but we are
not in a rush.

Junio, good day.

Wed, Mar 28, 2007 at 12:48:45PM -0700, Junio C Hamano wrote:

Eygene Ryabinkin [off-list ref] writes:

quoted

--with-tcltk enables the search of the Tcl/Tk interpreter. If no
interpreter is found then Tcl/Tk dependend parts are disabled.

--without-tcltk unconditionally disables Tcl/Tk dependent parts.

The original behaviour is not changed: bare './configure' just
installs the Tcl/Tk part doing no checks for the interpreter.

Makefile knob named NO_TCLTK was introduced. It prevents the build
and installation of the Tcl/Tk dependent parts.

Signed-off-by: Eygene Ryabinkin <redacted>
---

Thanks.

Is this supposed to be the first in the series?  I thought you
said you were going to do NO_TCLTK without anything else at all
first, and then TCLTK_PATH patch.  I am a bit lost here.

quoted

+# Define NO_TCLTK if you do not want Tcl/Tk GUI.
+#
+# The TCLTK_PATH variable governs the location of the Tck/Tk interpreter.
+# 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).
+#

Grumble.  If you are doing this, then there is not much point to
have two separate patches, is it?

I cheated, sorry: first patch prepared the configure's infrastructure
for the --with-tcltk/--without-tcltk including --with-tcltk=PATH.

And the second one introduced the TCLTK_PATH usage for substituting
the 'wish' in the Tcl/Tk tools.

Sorry for the confusion.

quoted

@@ -684,7 +696,9 @@ ifneq (,$X)
 endif
 
 all::
+ifndef NO_TCLTK
 	$(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) all
+endif
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' all
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1)

Although you were not supposed to be talking about paths, since
you've already introduced TCLTK_PATH, it should be passed down
to git-gui here, I think.

Yes, you're perfectly right.

quoted

@@ -955,11 +974,17 @@ dist: git.spec git-archive
 	@mkdir -p $(GIT_TARNAME)
 	@cp git.spec $(GIT_TARNAME)
 	@echo $(GIT_VERSION) > $(GIT_TARNAME)/version
+ifndef NO_TCLTK
 	@$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
 	$(TAR) rf $(GIT_TARNAME).tar \
 		$(GIT_TARNAME)/git.spec \
 		$(GIT_TARNAME)/version \
 		$(GIT_TARNAME)/git-gui/version
+else
+	$(TAR) rf $(GIT_TARNAME).tar \
+		$(GIT_TARNAME)/git.spec \
+		$(GIT_TARNAME)/version
+endif
 	@rm -rf $(GIT_TARNAME)
 	gzip -f -9 $(GIT_TARNAME).tar
 

Why should a source distribution exclude git-gui/ directory?  I
think it is sensible to ship a source that contains all.  You
are shipping gitk even without NO_TCLTK anyway, too.

Oops: didn't noticed that it is the tarball construction.

And from the part 2:

quoted

@@ -705,6 +709,12 @@ endif
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
+gitk-wish: gitk GIT-GUI-VARS
+	$(QUIET_GEN)rm -f $@ $@+ && \
+	sed -e'1,3s|^exec .* "$$0"|exec $(subst |,'\|',$(TCLTK_PATH_SQ)) "$$0"|' < gitk > $@+ && \
+	chmod +x $@+ && \
+	mv -f $@+ $@
+

This subst() is a nice attention to the detail.  I like it,
although in practice I do not think anybody is insane enough to
have a pipe character in the directory name that leads to wish.

Thanks! And for the sanity: I do not think that the single quote
in the path it sane too. But as I was teached, "if we should
quote something, we must quote it". ;))

I separated your two patches into three with minor modifications
and parked them in 'pu'.  We need to arrange with Shawn when to
apply the git-gui/ parts of the patch to his tree, but we are
not in a rush.

Thank you. Examined the 'origin/pu' and saw that you're already
incorporated the git.spec.in patch. I've found a glitch in it:
the right PYTHON_PATH should be passed. The patch follows.

By the way, when I was creating the git.spec from the git.spec.in,
I had the 'Version' field equal to the '1.5.1-rc1.GIT' and RPM
does not like the '-' characters inside the versions. Did
'tr - _' for specfile version and tarball name. The patch
follows.
-- 
Eygene

Signed-off-by: Eygene Ryabinkin <redacted>
---
 git.spec.in |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/git.spec.in b/git.spec.in
index e469f21..1e96eaa 100644
--- a/git.spec.in
+++ b/git.spec.in

@@ -1,4 +1,7 @@
 # Pass --without docs to rpmbuild if you don't want the documentation
+
+%define python_path /usr/bin/python
+
 Name: 		git
 Version: 	@@VERSION@@
 Release: 	1%{?dist}

@@ -93,12 +96,14 @@ Perl interface to Git
 
 %build
 make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" WITH_OWN_SUBPROCESS_PY=YesPlease \
-     WITH_P4IMPORT=YesPlease prefix=%{_prefix} all %{!?_without_docs: doc}
+     WITH_P4IMPORT=YesPlease PYTHON_PATH=%{python_path} prefix=%{_prefix} all \
+     %{!?_without_docs: doc}
 
 %install
 rm -rf $RPM_BUILD_ROOT
 make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" DESTDIR=$RPM_BUILD_ROOT \
-     WITH_OWN_SUBPROCESS_PY=YesPlease WITH_P4IMPORT=YesPlease \
+     WITH_OWN_SUBPROCESS_PY=YesPlease \
+     WITH_P4IMPORT=YesPlease PYTHON_PATH=%{python_path} \
      prefix=%{_prefix} mandir=%{_mandir} INSTALLDIRS=vendor \
      install %{!?_without_docs: install-doc}
 find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} ';'

-- 
1.5.0.3-dirty