threads / patch / 7332

patchAdded make options NO_GUI and WITH_P4IMPORT.

Subject: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

## tl;dr

47 messages between Mar 20, 2007 and Apr 5, 2007. Diffs are folded; open one to read it.

replies: 46people: 8as markdown or json

Eygene Ryabinkin· Mar 20, 2007, 11:45 UTC · lore

NO_GUI disables the building and installation of the git GUI part. WITH_P4IMPORT enables the installation of the Perforce import script.

This patch was originally developed for the FreeBSD port of git, but I think that it will not harm to integrate this patch into the development tree.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 Makefile |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
Show changes to Makefile +44 −0
diff --git a/Makefile b/Makefile
index 51c1fed..8b142f0 100644
--- a/Makefile
+++ b/Makefile
@@ -110,6 +110,10 @@ all::
 # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's
 # MakeMaker (e.g. using ActiveState under Cygwin).
 #
+# Define NO_GUI if you do not want Tcl/Tk GUI.
+#
+# Define WITH_P4IMPORT to build and install Python git-p4import script.
+#
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -196,9 +200,20 @@ SCRIPT_PERL = \
 	git-svnimport.perl git-cvsexportcommit.perl \
 	git-send-email.perl git-svn.perl
 
+SCRIPT_PYTHON = \
+	git-p4import.py
+
+ifdef WITH_P4IMPORT
+SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
+	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
+	  git-status git-instaweb
+else
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	  git-status git-instaweb
+endif
+
 
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS = \
@@ -241,6 +256,9 @@ endif
 ifndef PERL_PATH
 	PERL_PATH = /usr/bin/perl
 endif
+ifndef PYTHON_PATH
+	PYTHON_PATH = /usr/local/bin/python
+endif
 
 export PERL_PATH
 
@@ -646,6 +664,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
+PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
@@ -667,7 +686,9 @@ ifneq (,$X)
 endif
 
 all::
+ifndef NO_GUI
 	$(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)
 
@@ -699,6 +720,15 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
+$(patsubst %.py,%,$(SCRIPT_PYTHON)) : % : %.py
+	rm -f $@ $@+
+	sed -e '1s|#!.*/python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+	    $@.py >$@+
+	chmod +x $@+
+	mv $@+ $@
+
 perl/perl.mak: GIT-CFLAGS
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
@@ -892,10 +922,16 @@ install: all
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
 	$(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
+ifndef NO_GUI
 	$(INSTALL) git$X gitk '$(DESTDIR_SQ)$(bindir_SQ)'
+else
+	$(INSTALL) git$X '$(DESTDIR_SQ)$(bindir_SQ)'
+endif
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C perl prefix='$(prefix_SQ)' install
+ifndef NO_GUI
 	$(MAKE) -C git-gui install
+endif
 	if test 'z$(bindir_SQ)' != 'z$(gitexecdir_SQ)'; \
 	then \
 		ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
@@ -929,11 +965,17 @@ dist: git.spec git-archive
 	@mkdir -p $(GIT_TARNAME)
 	@cp git.spec $(GIT_TARNAME)
 	@echo $(GIT_VERSION) > $(GIT_TARNAME)/version
+ifndef NO_GUI
 	@$(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
 
@@ -974,7 +1016,9 @@ clean:
 	rm -f gitweb/gitweb.cgi
 	$(MAKE) -C Documentation/ clean
 	$(MAKE) -C perl clean
+ifndef NO_GUI
 	$(MAKE) -C git-gui clean
+endif
 	$(MAKE) -C templates/ clean
 	$(MAKE) -C t/ clean
 	rm -f GIT-VERSION-FILE GIT-CFLAGS

-- 
1.5.0.3-dirty
-- 
Eygene
Jakub Narebski· Mar 21, 2007, 00:35 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Eygene Ryabinkin wrote:
> NO_GUI disables the building and installation of the git GUI part.

By the way, it would be nice for ./configure script (generated from configure.ac) to detect if Tcl/Tk is available and disable git-gui and gitk installation if it is not found.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Eygene Ryabinkin· Mar 21, 2007, 05:14 UTC · re: Jakub Narebski · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Jakub, good day.
Wed, Mar 21, 2007 at 01:35:22AM +0100, Jakub Narebski wrote:
Show 7 quoted lines
> Eygene Ryabinkin wrote:
> 
> > NO_GUI disables the building and installation of the git GUI part.
> 
> By the way, it would be nice for ./configure script (generated from
> configure.ac) to detect if Tcl/Tk is available and disable
> git-gui and gitk installation if it is not found.

Will try to implement. But still, NO_GUI should ban the GUI tools from being built and installed, because user can have the Tcl/Tk available, but have no intention to use the git GUI. Am I right?

-- 
Eygene
Johannes Schindelin· Mar 21, 2007, 11:16 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Hi,
On Wed, 21 Mar 2007, Eygene Ryabinkin wrote:
> Will try to implement. But still, NO_GUI should ban the GUI tools from 
> being built and installed, because user can have the Tcl/Tk available, 
> but have no intention to use the git GUI. Am I right?

I am not quite certain if I agree. With a similar reasoning, you could introduce a flag to prevent pull-request from being installed, and git-tag, or other rarely used functions. Is it so bad to have gitk and git-gui installed? I mean, you are likely to just try them (and possibly like them!) at some stage, because the graphical representation is so much clearer than what _any_ text representation can do.

Ciao, Dscho

Eygene Ryabinkin· Mar 21, 2007, 11:50 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Johannes, good day.
Wed, Mar 21, 2007 at 12:16:40PM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> > Will try to implement. But still, NO_GUI should ban the GUI tools from 
> > being built and installed, because user can have the Tcl/Tk available, 
> > but have no intention to use the git GUI. Am I right?
> 
> I am not quite certain if I agree. With a similar reasoning, you could 
> introduce a flag to prevent pull-request from being installed, and 
> git-tag, or other rarely used functions. Is it so bad to have gitk and 
> git-gui installed?

I am happening to develop on some machines on which I have no X-Windows or any GUI providers at all, so I prefer not to have the Tcl/Tk dependency at all. Once again, the patch was originally done for the FreeBSD where the port system installs the dependencies automatically. And I do not need the Tcl/Tk on some machines: imagine the server that uses Git for its configuration tracking. It is server, there is absolutely no need for any GUIs there. And one of my developing machines has no X-Windows, so Tcl/Tk is again useless. This is the reasons for the NO_GUI knob. As it is turned off by default it does not breaks the expectations of the users who are used to the git GUI tools.

> I mean, you are likely to just try them (and possibly 
> like them!) at some stage, because the graphical representation is so much 
> clearer than what _any_ text representation can do.

Yes, sometimes I use the graphical representation. But there are use-cases when no GUI is needed at all.

-- 
Eygene
Johannes Schindelin· Mar 21, 2007, 14:25 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Hi,
On Wed, 21 Mar 2007, Eygene Ryabinkin wrote:
Show 13 quoted lines
> Wed, Mar 21, 2007 at 12:16:40PM +0100, Johannes Schindelin wrote:
> > > Will try to implement. But still, NO_GUI should ban the GUI tools from 
> > > being built and installed, because user can have the Tcl/Tk available, 
> > > but have no intention to use the git GUI. Am I right?
> > 
> > I am not quite certain if I agree. With a similar reasoning, you could 
> > introduce a flag to prevent pull-request from being installed, and 
> > git-tag, or other rarely used functions. Is it so bad to have gitk and 
> > git-gui installed?
> 
> I am happening to develop on some machines on which I have no
> X-Windows or any GUI providers at all, so I prefer not to have the
> Tcl/Tk dependency at all.

My point (and I think it's the same point as Jakub's) is that NO_GUI is a misnomer. It should be NO_TCL, since the only reason you state to skip installation of these parts is that they depend on X11, which is not installed on the machine.

If you don't do something, it is often interesting to state why: if you don't install something to prevent a dependency you don't want to have, it is different from saying that you do not want to have a GUI, _even if_ the dependency is there already.

Conclusion: I am in favour of NO_TCL, but not of NO_GUI.

Ciao, Dscho

Paolo Bonzini· Mar 21, 2007, 14:38 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Show 8 quoted lines
>> I am happening to develop on some machines on which I have no
>> X-Windows or any GUI providers at all, so I prefer not to have the
>> Tcl/Tk dependency at all.
> 
> If you don't do something, it is often interesting to state why: if you 
> don't install something to prevent a dependency you don't want to have, it 
> is different from saying that you do not want to have a GUI, _even if_ the 
> dependency is there already.

I read his message as "these are useless for me, so why introduce a useless dependency?" The "effect" is to have no Tcl dependence, but the original reason is to have no GUI.

So, "If you don't do something, it is often interesting to state why". Why no TCL (in git)? Because no X11 (on the machine).

Paolo
Eygene Ryabinkin· Mar 21, 2007, 14:42 UTC · re: Paolo Bonzini · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Paolo, good day.
Wed, Mar 21, 2007 at 03:38:53PM +0100, Paolo Bonzini wrote:
Show 12 quoted lines
> >> I am happening to develop on some machines on which I have no
> >> X-Windows or any GUI providers at all, so I prefer not to have the
> >> Tcl/Tk dependency at all.
> > 
> > If you don't do something, it is often interesting to state why: if you 
> > don't install something to prevent a dependency you don't want to have, it 
> > is different from saying that you do not want to have a GUI, _even if_ the 
> > dependency is there already.
> 
> I read his message as "these are useless for me, so why introduce a
> useless dependency?"  The "effect" is to have no Tcl dependence,
> but the original reason is to have no GUI.

Yep, you're right. That was I meant originally. How do you feel about NO_GUI or NO_TCL names?

-- 
Eygene
Paolo Bonzini· Mar 21, 2007, 14:49 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Show 6 quoted lines
>> I read his message as "these are useless for me, so why introduce a
>> useless dependency?"  The "effect" is to have no Tcl dependence,
>> but the original reason is to have no GUI.
> 
> Yep, you're right. That was I meant originally. How do you feel
> about NO_GUI or NO_TCL names?
I agree with you. ;-)
Paolo
Alex Riesen· Mar 21, 2007, 14:58 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

On 3/21/07, Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
Show 16 quoted lines
> Wed, Mar 21, 2007 at 03:38:53PM +0100, Paolo Bonzini wrote:
> > >> I am happening to develop on some machines on which I have no
> > >> X-Windows or any GUI providers at all, so I prefer not to have the
> > >> Tcl/Tk dependency at all.
> > >
> > > If you don't do something, it is often interesting to state why: if you
> > > don't install something to prevent a dependency you don't want to have, it
> > > is different from saying that you do not want to have a GUI, _even if_ the
> > > dependency is there already.
> >
> > I read his message as "these are useless for me, so why introduce a
> > useless dependency?"  The "effect" is to have no Tcl dependence,
> > but the original reason is to have no GUI.
>
> Yep, you're right. That was I meant originally. How do you feel
> about NO_GUI or NO_TCL names?

That'd be NO_TCL_TK. TCL has nothing to do with graphics. And you have one more supporter for NO_GUI (my server has no usable graphics, will never have and runs cron jobs with git in them).

Jakub Narebski· Mar 24, 2007, 23:16 UTC · re: Alex Riesen · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

On Wed, Mar 21, 2007, Alex Riesen wrote:
> On 3/21/07, Eygene Ryabinkin <rea-git@codelabs.ru> wrote:
Show 6 quoted lines
>> Yep, you're right. That was I meant originally. How do you feel
>> about NO_GUI or NO_TCL names?
> 
> That'd be NO_TCL_TK. TCL has nothing to do with graphics.
> And you have one more supporter for NO_GUI (my server has no
> usable graphics, will never have and runs cron jobs with git in them).

Actually the value for NO_TCL_TK (or NO_TCLTK) can be detected automatically by ./configure in similar way that we used to detect the existence of Python in the old days when there were core code which written in Python. Perhaps we should provide way to override autodetection _and_ provide path to 'wish' executable with the --with-wish=PATH or --with-tcltk=PATH to ./configure script (similar to how Python dependency was handled, and how paths to shell and Perl are handled).

The NO_GUI is another issue, if to be configured by ./configure script, then only as an user option, not something to be autodetected: either --without-gui (treat gui as package, although usually it is about package to be used, not package provided) or --disable-gui (treat gui as feature of project).

The fact that all GUI that comes with git repository is in Tcl/Tk slightly clouds this issue. As do the fact that those affect only installation stage, and not build (well, with the exception of WISH_PATH / TCLTK_PATH).

-- 
Jakub Narebski
Poland
Eygene Ryabinkin· Mar 21, 2007, 14:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Johannes,
Wed, Mar 21, 2007 at 03:25:32PM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> > I am happening to develop on some machines on which I have no
> > X-Windows or any GUI providers at all, so I prefer not to have the
> > Tcl/Tk dependency at all.
> 
> My point (and I think it's the same point as Jakub's) is that NO_GUI is a 
> misnomer. It should be NO_TCL, since the only reason you state to skip 
> installation of these parts is that they depend on X11, which is not 
> installed on the machine.

Can't speak for Jakub, but it seems to me that he just suggested that the configure should look if the Tcl/Tk is available and refuse to install the GUI tools if there is no Tcl/Tk. Though only Jakub can tell for sure.

> If you don't do something, it is often interesting to state why: if you 
> don't install something to prevent a dependency you don't want to have, it 
> is different from saying that you do not want to have a GUI, _even if_ the 
> dependency is there already.

But I am saying that I do not want the GUI tools installed because I do not need GUI at all. And if the GUI will be rewritten to the Qt (oh, my, don't do that) I will still have no need of it.

> Conclusion: I am in favour of NO_TCL, but not of NO_GUI.

I am not against such renaming as long as the TCL will be used for the GUI part of git. Should I file a patch for the NO_GUI -> NO_TCL change? What do others think about the knob name?

-- 
Eygene
Johannes Schindelin· Mar 21, 2007, 15:35 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Hi,
I will make my point very clear now:

People like you compile the source not so much to develop _in_ Git, but _with_ Git. So, you are likely to "./configure && make install".

For something like "./configure && make install", it makes tons of sense to check which dependencies are there, and which are not. Then, depending if it is possible to compile (or install) only parts of Git, because some dependencies are not met, ./configure can figure out what flags to set.

Now, if you have no X11 installed (and consequently no Tk), it is very easy for ./configure to find out what to do.

However, if you state that you want to have no gui (and you state it as such), you are not caring if X11 is installed or not! And ./configure cannot figure out what you want, based on what it finds, since it cannot read your mind. At least that is what I expect.

Now, enough talk, if my point is still not clear enough, I'll just let it be.

Ciao, Dscho

Eygene Ryabinkin· Mar 21, 2007, 16:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Johannes,
Wed, Mar 21, 2007 at 04:35:03PM +0100, Johannes Schindelin wrote:
Show 9 quoted lines
> I will make my point very clear now:
> 
> People like you compile the source not so much to develop _in_ Git, but 
> _with_ Git. So, you are likely to "./configure && make install".
> 
> For something like "./configure && make install", it makes tons of sense 
> to check which dependencies are there, and which are not. Then, depending 
> if it is possible to compile (or install) only parts of Git, because some 
> dependencies are not met, ./configure can figure out what flags to set.

Yes, I see your point. But up to date (with 1.5.0.3 tarball) I saw no stock configure and thus used the 'make && make install' sequence with the 'prefix' set to some good location. Reading the INSTALL file I've found that the 'make configure && ./configure --prefix=<blah> && make && make install' will do the trick too. I just was unaware of it since I was using the first sequence. And thus I happened to do 'NO_GUI=yes make && make install'.

Show 7 quoted lines
> Now, if you have no X11 installed (and consequently no Tk), it is very 
> easy for ./configure to find out what to do.
> 
> However, if you state that you want to have no gui (and you state it as 
> such), you are not caring if X11 is installed or not! And ./configure 
> cannot figure out what you want, based on what it finds, since it cannot 
> read your mind. At least that is what I expect.

OK. So configure needs the detection of the Tcl/Tk and the --disable-gui option. And the Makefile can be modified to get the additional NO_TCL_TK option that will help configure to inform the Makefile that no Tcl/Tk is here.

Technically, the checks in Makefile will look as 'ifndef NO_GUI && NO_TCL_TK' instead of 'ifndef NO_GUI'. Later they can diverge as the software will evolve.

Are people happy with such plan?
-- 
Eygene
Junio C Hamano· Mar 21, 2007, 16:17 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 5 quoted lines
> Technically, the checks in Makefile will look as 'ifndef NO_GUI && NO_TCL_TK'
> instead of 'ifndef NO_GUI'. Later they can diverge as the software will
> evolve.
>
> Are people happy with such plan?

Maybe later you might even want to view the graphical history from the server displaying on remote X, who knows?

We have NO_CURL and such because lack of the necessary libraries and headers prevent your build from completing, but in the case of git-gui and gitk, they are just scripts and you would not have any trouble in building. I do not know if adding more conditional to Makefile in order to skip them is worth it.

Eygene Ryabinkin· Mar 26, 2007, 07:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Junio, good day.
Wed, Mar 21, 2007 at 09:17:21AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> 
> > Technically, the checks in Makefile will look as 'ifndef NO_GUI && NO_TCL_TK'
> > instead of 'ifndef NO_GUI'. Later they can diverge as the software will
> > evolve.
> >
> > Are people happy with such plan?
> 
> Maybe later you might even want to view the graphical history
> from the server displaying on remote X, who knows?
Not sure: I don't like X on the servers ;)) But others can...
Show 5 quoted lines
> We have NO_CURL and such because lack of the necessary libraries
> and headers prevent your build from completing, but in the case
> of git-gui and gitk, they are just scripts and you would not
> have any trouble in building.  I do not know if adding more
> conditional to Makefile in order to skip them is worth it.

OK, I reworked the patch following the suggestion of Jakub and Johannes: now configure has the built-in detection of the Tcl/Tk binary and has the --with-tcltk/--without-tcltk options. To implement this I still need the NO_TCLTK knob in the Makefile. Moreover, the configure's switch --with-tcltk=/path/to/binary works as expected: the location of the Tcl/Tk interpreter will be rewritten in the gitk and git-gui. The patch follows on this thread.

And regarding the building troubles and the additional knob in the
Makefile: the trouble is in the packaging process. For example, RPM
or FreeBSD ports are looking at what is really installed, so if
user do not want the Tcl/Tk part, then no package parts that depend
on it should be installed. And the bare 'make install' installs all
things. Sure, you can make 'make install && rm -f <not needed
files>', but it is always a pain for the package builders to get
the idea about the precise file list. So I just wanted to integrate
the desired behaviour in the mainstream Git to make packager's life
a bit easy. May be by the cost of making developer's life a bit
harder: he should watch for the NO_TCLTK in his Makefiles.
-- 
Eygene
Eygene Ryabinkin· Mar 26, 2007, 07:32 UTC · re: Eygene Ryabinkin · lore

[PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

The following make options were added:
- NO_TCLTK: disables building and installation of the git GUI part
that depends on the Tcl/Tk.
- WITH_P4IMPORT: enables the installation of the Perforce import
script.

Configure's options --with-tcltk and --without-tcltk were added and configure script teached to search for the Tcl/Tk interpreter. The GUI part will not be installed if system lacks Tcl/Tk binary.

Internal make option TCLTK was added: it governs the location of the Tcl/Tk interpreter, so user can specify its own binary location either with './configure --with-tcltk=/path/to/binary' or 'TCLTK=/path/to/binary make'.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 Makefile         |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 config.mak.in    |    1 +
 configure.ac     |   19 ++++++++++++++++++
 git-gui/Makefile |    1 +
 4 files changed, 75 insertions(+), 2 deletions(-)
Show changes to 4 files +75 −2

Makefile, config.mak.in, configure.ac, git-gui/Makefile

diff --git a/Makefile b/Makefile
index 51c1fed..3cccf79 100644
--- a/Makefile
+++ b/Makefile
@@ -110,6 +110,13 @@ all::
 # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's
 # MakeMaker (e.g. using ActiveState under Cygwin).
 #
+# Define NO_TCLTK if you do not want Tcl/Tk GUI.
+#
+# The TCLTK variable governs the location of the Tck/Tk interpreter.
+# If not set it defaults to the bare 'wish'.
+#
+# Define WITH_P4IMPORT to build and install Python git-p4import script.
+#
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -159,6 +166,7 @@ AR = ar
 TAR = tar
 INSTALL = install
 RPMBUILD = rpmbuild
+TCLTK ?= wish
 
 # sparse is architecture-neutral, which means that we need to tell it
 # explicitly what architecture to check for. Fix this up for yours..
@@ -196,9 +204,20 @@ SCRIPT_PERL = \
 	git-svnimport.perl git-cvsexportcommit.perl \
 	git-send-email.perl git-svn.perl
 
+SCRIPT_PYTHON = \
+	git-p4import.py
+
+ifdef WITH_P4IMPORT
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
 	  git-status git-instaweb
+else
+SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
+	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  git-status git-instaweb
+endif
+
 
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS = \
@@ -241,6 +260,9 @@ endif
 ifndef PERL_PATH
 	PERL_PATH = /usr/bin/perl
 endif
+ifndef PYTHON_PATH
+	PYTHON_PATH = /usr/local/bin/python
+endif
 
 export PERL_PATH
 
@@ -608,6 +630,10 @@ ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
 
+ifeq ($(TCLTK),)
+NO_TCLTK=YesPlease
+endif
+
 QUIET_SUBDIR0  = $(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
@@ -646,6 +672,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
+PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
@@ -667,7 +694,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)
 
@@ -699,6 +728,15 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
+$(patsubst %.py,%,$(SCRIPT_PYTHON)) : % : %.py
+	rm -f $@ $@+
+	sed -e '1s|#!.*/python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+	    $@.py >$@+
+	chmod +x $@+
+	mv $@+ $@
+
 perl/perl.mak: GIT-CFLAGS
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
@@ -892,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)"' "$$0"|' gitk && rm -f gitk.bak
+	$(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='$(TCLTK)' install
+endif
 	if test 'z$(bindir_SQ)' != 'z$(gitexecdir_SQ)'; \
 	then \
 		ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
@@ -929,11 +973,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
 
@@ -974,7 +1024,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..8e441dd 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 = @TCLTK@
 
 prefix = @prefix@
 exec_prefix = @exec_prefix@
diff --git a/configure.ac b/configure.ac
index 3a8e778..a95dbfb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -75,6 +75,12 @@ 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]),\
+GIT_PARSE_WITH(tcltk))
+#
 
 
 ## Checks for programs.
@@ -84,6 +90,19 @@ 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 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" = "yes" -o "$with_tcltk" = ""; then
+    AC_CHECK_PROGS(TCLTK, [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="$with_tcltk"
+    AC_SUBST(TCLTK)
+  fi
+fi
 
 ## Checks for libraries.
 AC_MSG_NOTICE([CHECKS for libraries])
diff --git a/git-gui/Makefile b/git-gui/Makefile
index b82789e..09c28ed 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -55,6 +55,7 @@ all:: $(ALL_PROGRAMS)
 
 install: all
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
+	sed -i .bak -e'1,3s|^exec .* "$$0"|exec '"$(TCLTK)"' "$$0"|' git-gui && rm git-gui.bak
 	$(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' ;)
 

-- 
1.5.0.3-dirty
Junio C Hamano· Mar 26, 2007, 08:27 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Please do not mix P4IMPORT and TCLTK issues in the same patch. As I am still not convinced about what to do with NO_TCLTK, this forces people who might want to see P4IMPORT to wait longer than necessary.

Eygene Ryabinkin· Mar 27, 2007, 10:26 UTC · re: Junio C Hamano · lore

[PATCH] Add the WITH_P4IMPORT knob to the Makefile.

WITH_P4IMPORT: enables the installation of the Perforce import
script.
Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 Makefile |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Show changes to Makefile +26 −0
diff --git a/Makefile b/Makefile
index 51c1fed..a294ec8 100644
--- a/Makefile
+++ b/Makefile
@@ -110,6 +110,8 @@ all::
 # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's
 # MakeMaker (e.g. using ActiveState under Cygwin).
 #
+# Define WITH_P4IMPORT to build and install Python git-p4import script.
+#
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -196,9 +198,20 @@ SCRIPT_PERL = \
 	git-svnimport.perl git-cvsexportcommit.perl \
 	git-send-email.perl git-svn.perl
 
+SCRIPT_PYTHON = \
+	git-p4import.py
+
+ifdef WITH_P4IMPORT
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  $(patsubst %.py,%,$(SCRIPT_PYTHON)) \
 	  git-status git-instaweb
+else
+SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
+	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
+	  git-status git-instaweb
+endif
+
 
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS = \
@@ -241,6 +254,9 @@ endif
 ifndef PERL_PATH
 	PERL_PATH = /usr/bin/perl
 endif
+ifndef PYTHON_PATH
+	PYTHON_PATH = /usr/local/bin/python
+endif
 
 export PERL_PATH
 
@@ -646,6 +662,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
+PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
@@ -699,6 +716,15 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
+$(patsubst %.py,%,$(SCRIPT_PYTHON)) : % : %.py
+	rm -f $@ $@+
+	sed -e '1s|#!.*/python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+	    $@.py >$@+
+	chmod +x $@+
+	mv $@+ $@
+
 perl/perl.mak: GIT-CFLAGS
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 

-- 
1.5.0.3-dirty
Junio C Hamano· Mar 27, 2007, 10:54 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Add the WITH_P4IMPORT knob to the Makefile.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> WITH_P4IMPORT: enables the installation of the Perforce import
> script.
>
> Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>

Thanks. I wonder if we need to update git.spec.in file if we were to take this patch.

Eygene Ryabinkin· Mar 27, 2007, 11:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add the WITH_P4IMPORT knob to the Makefile.

Tue, Mar 27, 2007 at 03:54:16AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> 
> > WITH_P4IMPORT: enables the installation of the Perforce import
> > script.
> >
> > Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
> 
> Thanks.
You're welcome ;))
> I wonder if we need to update git.spec.in file if we
> were to take this patch.

Created a quick patch, but have no Linux host with autoconf >= 2.59 at hand, so can not test. The patch follows.

-- 
Eygene
Eygene Ryabinkin· Mar 27, 2007, 11:25 UTC · re: Eygene Ryabinkin · lore

[PATCH] Added git-p4 package to the list of git RPMs.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 git.spec.in |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)
Show changes to git.spec.in +21 −4
diff --git a/git.spec.in b/git.spec.in
index 46aee88..8c79a79 100644
--- a/git.spec.in
+++ b/git.spec.in
@@ -50,6 +50,13 @@ Requires:       git-core = %{version}-%{release}, tla
 %description arch
 Git tools for importing Arch repositories.
 
+%package p4
+Summary:        Git tools for importing Perforce repositories
+Group:          Development/Tools
+Requires:       git-core = %{version}-%{release}, python
+%description p4
+Git tools for importing Perforce repositories.
+
 %package email
 Summary:        Git tools for sending email
 Group:          Development/Tools
@@ -86,22 +93,22 @@ Perl interface to Git
 
 %build
 make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" WITH_OWN_SUBPROCESS_PY=YesPlease \
-     prefix=%{_prefix} all %{!?_without_docs: doc}
+     WITH_P4IMPORT=YesPlease 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_OWN_SUBPROCESS_PY=YesPlease WITH_P4IMPORT=YesPlease \
      prefix=%{_prefix} mandir=%{_mandir} INSTALLDIRS=vendor \
      install %{!?_without_docs: install-doc}
 find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} ';'
 find $RPM_BUILD_ROOT -type f -name '*.bs' -empty -exec rm -f {} ';'
 find $RPM_BUILD_ROOT -type f -name perllocal.pod -exec rm -f {} ';'
 
-(find $RPM_BUILD_ROOT%{_bindir} -type f | grep -vE "archimport|svn|cvs|email|gitk|git-gui|git-citool" | sed -e s@^$RPM_BUILD_ROOT@@)               > bin-man-doc-files
+(find $RPM_BUILD_ROOT%{_bindir} -type f | grep -vE "p4import|archimport|svn|cvs|email|gitk|git-gui|git-citool" | sed -e s@^$RPM_BUILD_ROOT@@)               > bin-man-doc-files
 (find $RPM_BUILD_ROOT%{perl_vendorlib} -type f | sed -e s@^$RPM_BUILD_ROOT@@) >> perl-files
 %if %{!?_without_docs:1}0
-(find $RPM_BUILD_ROOT%{_mandir} $RPM_BUILD_ROOT/Documentation -type f | grep -vE "archimport|svn|git-cvs|email|gitk|git-gui|git-citool" | sed -e s@^$RPM_BUILD_ROOT@@ -e 's/$/*/' ) >> bin-man-doc-files
+(find $RPM_BUILD_ROOT%{_mandir} $RPM_BUILD_ROOT/Documentation -type f | grep -vE "p4import|archimport|svn|git-cvs|email|gitk|git-gui|git-citool" | sed -e s@^$RPM_BUILD_ROOT@@ -e 's/$/*/' ) >> bin-man-doc-files
 %else
 rm -rf $RPM_BUILD_ROOT%{_mandir}
 %endif
@@ -133,6 +140,13 @@ rm -rf $RPM_BUILD_ROOT
 %{!?_without_docs: %{_mandir}/man1/git-archimport.1*}
 %{!?_without_docs: %doc Documentation/git-archimport.html }
 
+%files p4
+%defattr(-,root,root)
+%doc Documentation/git-p4import.txt
+%{_bindir}/git-p4import
+%{!?_without_docs: %{_mandir}/man1/git-p4import.1*}
+%{!?_without_docs: %doc Documentation/git-p4import.html }
+
 %files email
 %defattr(-,root,root)
 %doc Documentation/*email*.txt
@@ -167,6 +181,9 @@ rm -rf $RPM_BUILD_ROOT
 %{!?_without_docs: %doc Documentation/*.html }
 
 %changelog
+* Tue Mar 27 2007 Eygene Ryabinkin <rea-git@codelabs.ru>
+- Added the git-p4 package: Perforce import stuff.
+
 * Mon Feb 13 2007 Nicolas Pitre <nico@cam.org>
 - Update core package description (Git isn't as stupid as it used to be)
 

-- 
1.5.0.3-dirty
Brian Gernhardt· Mar 27, 2007, 16:03 UTC · re: Eygene Ryabinkin · lore

[PATCH] Remove unused WITH_OWN_SUBPROCESS_PY from RPM spec

We don't have a copy of subprocess.py anymore, so we removed that option from the Makefile. Let's not leave that cruft around the RPM spec file either. ---

This applies on top of "[PATCH] Added git-p4 package to the list of git RPMs.", which is what made me notice that it was there. Untested, but simply removing a completely unused option from the make command line shouldn't cause any problems, right? Right?

 git.spec.in |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Show changes to git.spec.in +4 −5
diff --git a/git.spec.in b/git.spec.in
index e469f21..4bf7a8f 100644
--- a/git.spec.in
+++ b/git.spec.in
@@ -92,15 +92,14 @@ Perl interface to Git
 %setup -q
 
 %build
-make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" WITH_OWN_SUBPROCESS_PY=YesPlease \
-     WITH_P4IMPORT=YesPlease prefix=%{_prefix} all %{!?_without_docs: doc}
+make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" WITH_P4IMPORT=YesPlease \
+     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 \
-     prefix=%{_prefix} mandir=%{_mandir} INSTALLDIRS=vendor \
-     install %{!?_without_docs: install-doc}
+     WITH_P4IMPORT=YesPlease prefix=%{_prefix} mandir=%{_mandir} \
+     INSTALLDIRS=vendor install %{!?_without_docs: install-doc}
 find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} ';'
 find $RPM_BUILD_ROOT -type f -name '*.bs' -empty -exec rm -f {} ';'
 find $RPM_BUILD_ROOT -type f -name perllocal.pod -exec rm -f {} ';'

-- 
1.5.1.rc2
Junio C Hamano· Apr 4, 2007, 18:30 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added git-p4 package to the list of git RPMs.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 16 quoted lines
> diff --git a/git.spec.in b/git.spec.in
> index 46aee88..8c79a79 100644
> --- a/git.spec.in
> +++ b/git.spec.in
> @@ -50,6 +50,13 @@ Requires:       git-core = %{version}-%{release}, tla
>  %description arch
>  Git tools for importing Arch repositories.
>  
> +%package p4
> +Summary:        Git tools for importing Perforce repositories
> +Group:          Development/Tools
> +Requires:       git-core = %{version}-%{release}, python
> +%description p4
> +Git tools for importing Perforce repositories.
> +
>...
Thanks. I'll also add git-p4 here.
Show changes to git.spec.in +1 −1
diff --git a/git.spec.in b/git.spec.in
index 1d3934b..f0746ed 100644
--- a/git.spec.in
+++ b/git.spec.in
@@ -12,7 +12,7 @@ URL: 		http://kernel.org/pub/software/scm/git/
 Source: 	http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz
 BuildRequires:	zlib-devel >= 1.2, openssl-devel, curl-devel, expat-devel  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}
 BuildRoot:	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
-Requires:	git-core, git-svn, git-cvs, git-arch, git-email, gitk, git-gui, perl-Git
+Requires:	git-core, git-svn, git-cvs, git-arch, git-email, gitk, git-gui, git-p4, perl-Git
 
 %description
 Git is a fast, scalable, distributed revision control system with an
Jakub Narebski· Mar 26, 2007, 08:30 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Thanks a lot for the patch, but...
Eygene Ryabinkin wrote:
> Internal make option TCLTK was added: it governs the location of
> the Tcl/Tk interpreter, so user can specify its own binary location
> either with './configure --with-tcltk=/path/to/binary' or
> 'TCLTK=/path/to/binary make'.
...shouldn't it be TCLTK_PATH?
-- 
Jakub Narebski
Poland
Junio C Hamano· Mar 26, 2007, 08:36 UTC · re: Jakub Narebski · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Jakub Narebski <jnareb@gmail.com> writes:
Show 10 quoted lines
> Thanks a lot for the patch, but...
>
> Eygene Ryabinkin wrote:
>
>> Internal make option TCLTK was added: it governs the location of
>> the Tcl/Tk interpreter, so user can specify its own binary location
>> either with './configure --with-tcltk=/path/to/binary' or
>> 'TCLTK=/path/to/binary make'.
>
> ...shouldn't it be TCLTK_PATH?

Thanks for sanity checking. That means that the absense of tcltk would make it impossible to munge the scripts to point at the wish binary, so makes the NO_TCLTK stuff easier to swallow.

Eygene Ryabinkin· Mar 26, 2007, 10:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Jakub, Junio,
Mon, Mar 26, 2007 at 01:36:49AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Thanks a lot for the patch, but...
> >
> > Eygene Ryabinkin wrote:
> >
> >> Internal make option TCLTK was added: it governs the location of
> >> the Tcl/Tk interpreter, so user can specify its own binary location
> >> either with './configure --with-tcltk=/path/to/binary' or
> >> 'TCLTK=/path/to/binary make'.
> >
> > ...shouldn't it be TCLTK_PATH?
Yes, probably is should be TCLTK_PATH.
> Thanks for sanity checking.  That means that the absense of
> tcltk would make it impossible to munge the scripts to point at
> the wish binary, so makes the NO_TCLTK stuff easier to swallow.

Sorry, did not get the point. The TCLTK is initialized to the 'wish' by 'TCLTK ?= wish', so TCLTK will always be here and initialized to the wish by-default.

-- 
Eygene
Junio C Hamano· Mar 27, 2007, 04:12 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 9 quoted lines
> Mon, Mar 26, 2007 at 01:36:49AM -0700, Junio C Hamano wrote:
> ...
>> Thanks for sanity checking.  That means that the absense of
>> tcltk would make it impossible to munge the scripts to point at
>> the wish binary, so makes the NO_TCLTK stuff easier to swallow.
>
> Sorry, did not get the point. The TCLTK is initialized to the 'wish'
> by 'TCLTK ?= wish', so TCLTK will always be here and initialized
> to the wish by-default.

Earlier I said I did not see a reason for not building wish applications on a build system that lack them. I am stating that you could argue that your rewriting the path to wish is a good reason (I would say it is half-good, as you can still tell the build procedure where wish will be on the deployed system without having it on your build system) for not building wish applications in a build that lacks wish installation.

Eygene Ryabinkin· Mar 27, 2007, 06:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Added options NO_TCLTK, WITH_P4IMPORT and --with-tcltk/--without-tcltk.

Junio, good day.
Mon, Mar 26, 2007 at 09:12:17PM -0700, Junio C Hamano wrote:
Show 19 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> 
> > Mon, Mar 26, 2007 at 01:36:49AM -0700, Junio C Hamano wrote:
> > ...
> >> Thanks for sanity checking.  That means that the absense of
> >> tcltk would make it impossible to munge the scripts to point at
> >> the wish binary, so makes the NO_TCLTK stuff easier to swallow.
> >
> > Sorry, did not get the point. The TCLTK is initialized to the 'wish'
> > by 'TCLTK ?= wish', so TCLTK will always be here and initialized
> > to the wish by-default.
> 
> Earlier I said I did not see a reason for not building wish
> applications on a build system that lack them.  I am stating
> that you could argue that your rewriting the path to wish is a
> good reason (I would say it is half-good, as you can still tell
> the build procedure where wish will be on the deployed system
> without having it on your build system) for not building wish
> applications in a build that lacks wish installation.

OK, so, probably, I should modify the behaviour of the --with-tcltk and configure to look for the Tcl/Tk interpreter _only_ if --with-tcltk[=PATH] was given and to leave the things unmodified in the case of absence of that option. But still, --without-tcltk will disable Tcl/Tk dependant parts. Will people be happy with such behaviour?

-- 
Eygene
Eygene Ryabinkin· Mar 27, 2007, 10:24 UTC · re: Eygene Ryabinkin · lore

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

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 <rea-git@codelabs.ru>
---
 Makefile         |   30 ++++++++++++++++++++++++++++--
 config.mak.in    |    1 +
 configure.ac     |   26 ++++++++++++++++++++++++++
 git-gui/Makefile |    3 +++
 4 files changed, 58 insertions(+), 2 deletions(-)
Show changes to 4 files +58 −2

Makefile, config.mak.in, configure.ac, git-gui/Makefile

diff --git a/Makefile b/Makefile
index a294ec8..06b6c6b 100644
--- a/Makefile
+++ b/Makefile
@@ -112,6 +112,11 @@ 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'.
+#
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -161,6 +166,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 +630,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  =
 
@@ -684,7 +694,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 +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
 	$(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)' install
+endif
 	if test 'z$(bindir_SQ)' != 'z$(gitexecdir_SQ)'; \
 	then \
 		ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
@@ -955,11 +973,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 +1024,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..2316b24 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 =
@@ -55,6 +57,7 @@ all:: $(ALL_PROGRAMS)
 
 install: all
 	$(INSTALL) -d -m755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
+	sed -i .bak -e'1,3s|^exec .* "$$0"|exec '"$(TCLTK_PATH)"' "$$0"|' git-gui && rm git-gui.bak
 	$(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' ;)
 

-- 
1.5.0.3-dirty
Junio C Hamano· Mar 27, 2007, 10:53 UTC · re: Eygene Ryabinkin · lore

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

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 12 quoted lines
> 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 <rea-git@codelabs.ru>
Thanks.
Show 20 quoted lines
> diff --git a/Makefile b/Makefile
> index a294ec8..06b6c6b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -112,6 +112,11 @@ 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'.
> +#
>...
> +TCLTK_PATH ?= wish
> ...  
> +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.

Show 10 quoted lines
> @@ -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.

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.

Eygene Ryabinkin· Mar 27, 2007, 11:07 UTC · re: Junio C Hamano · lore

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

Junio, good day.
Tue, Mar 27, 2007 at 03:53:33AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> > +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.
Show 21 quoted lines
> 
> > @@ -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?

Show 19 quoted lines
> 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
Junio C Hamano· Mar 28, 2007, 01:52 UTC · re: Eygene Ryabinkin · lore

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

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 7 quoted lines
>> 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.
Eygene Ryabinkin· Mar 28, 2007, 09:12 UTC · re: Junio C Hamano · lore

[PATCH] Add --with-tcltk and --without-tcltk to configure.

--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 <rea-git@codelabs.ru>
---
 Makefile         |   31 +++++++++++++++++++++++++++++--
 config.mak.in    |    1 +
 configure.ac     |   26 ++++++++++++++++++++++++++
 git-gui/Makefile |    3 +++
 4 files changed, 59 insertions(+), 2 deletions(-)
Show changes to 4 files +59 −2

Makefile, config.mak.in, configure.ac, git-gui/Makefile

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
Eygene Ryabinkin· Mar 28, 2007, 09:13 UTC · re: Eygene Ryabinkin · lore

[PATCH] Added Tcl/Tk interpreter path rewriting for the GUI tools.

--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 <rea-git@codelabs.ru>
---
 Makefile         |   31 ++++++++++++++++++++++++++++---
 git-gui/Makefile |   17 +++++++++++++++--
 2 files changed, 43 insertions(+), 5 deletions(-)
Show changes to 2 files +43 −5

Makefile, git-gui/Makefile

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
Junio C Hamano· Mar 28, 2007, 19:48 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 13 quoted lines
> --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 <rea-git@codelabs.ru>
> ---
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.

Show 6 quoted lines
> +# 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?

Show 9 quoted lines
> @@ -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.

Show 18 quoted lines
> @@ -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.

And from the part 2:
Show 10 quoted lines
> @@ -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.

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.

Eygene Ryabinkin· Mar 29, 2007, 07:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Junio, good day.
Wed, Mar 28, 2007 at 12:48:45PM -0700, Junio C Hamano wrote:
Show 31 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> 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 <rea-git@codelabs.ru>
> > ---
> 
> 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?

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.
Show 14 quoted lines
> 
> > @@ -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.
Show 23 quoted lines
> 
> > @@ -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.
Show 16 quoted lines
> And from the part 2:
> 
> > @@ -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". ;))

Show 5 quoted lines
> 
> 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
Junio C Hamano· Mar 29, 2007, 08:00 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 12 quoted lines
> Wed, Mar 28, 2007 at 12:48:45PM -0700, Junio C Hamano wrote:
>> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>> ...
>> > +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 $@+ $@
>> ...
> 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". ;))
Actually, look at the wish script you are running sed on.
	exec wish "$0" -- "$@"

If you substitute "wish" with "/i use stupid/$PATH/to/wish", I think Tcl splits the path at SP and does not protect $var reference, so the careful quoting in the Makefile is still not good enough ;-).

But come to think of it, it lets shell handle $PATH to find wish anyway, so *unless* we have specific version dependency to wish that wish binary normally found on user's $PATH is inadequate, we probably should not even need to be doing any of this path munging. You might end up discovering the path to wish binary in your autoconf script, we do not have to use it. ./configure can just see if there is wish, and set NO_TCLTK appropriately without any of the path business.

What do you think?
> 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.

That is semi-intended, in that you are not even supposed to be building with "1.5.1-rc1.GIT". The version file in the tarball that git.spec file lives in should use git-describe, built from the source before the tarball was made, to get the version number, and wouldn't be "$anything.GIT", which is the last-ditch fallback string, which is set by GIT-VERSION-GEN for people who build in a wrong way.

Eygene Ryabinkin· Mar 29, 2007, 08:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Junio, Thu, Mar 29, 2007 at 01:00:35AM -0700, Junio C Hamano wrote:

Show 8 quoted lines
> Actually, look at the wish script you are running sed on.
> 
> 	exec wish "$0" -- "$@"
> 
> If you substitute "wish" with "/i use stupid/$PATH/to/wish", I
> think Tcl splits the path at SP and does not protect $var
> reference, so the careful quoting in the Makefile is still not
> good enough ;-).

It is not Tcl/Tk, who interprets that string: it is for shell. So, if the line will look like exec "/insane path/to/wish" "$0" -- "$@", then we will just get the "/insane path/to/wish" executed with the script name on the first place and other arguments following the '--'.

Or you meant something different? I am little confused with the '$PATH' in your example. Was it intended?

Show 10 quoted lines
> But come to think of it, it lets shell handle $PATH to find wish
> anyway, so *unless* we have specific version dependency to wish
> that wish binary normally found on user's $PATH is inadequate,
> we probably should not even need to be doing any of this path
> munging.  You might end up discovering the path to wish binary
> in your autoconf script, we do not have to use it.  ./configure
> can just see if there is wish, and set NO_TCLTK appropriately
> without any of the path business.
> 
> What do you think?

There are problems at least with FreeBSD: it just installs the wish8.4, wish8.3, wish8.2, etc. It does not provide the bare 'wish' as the link to one of those: it is hard to tell what 'wish' we will like to use. Sure, I can search for 'wish8.3', 'wish8.4' in the configure script. But when new wish will be out the Git configure should be fixed for it. Seems like passing the path of the Tcl/Tk interpreter still have some meaning in this situation.

Show 12 quoted lines
> 
> > 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.
> 
> That is semi-intended, in that you are not even supposed to be
> building with "1.5.1-rc1.GIT".  The version file in the tarball
> that git.spec file lives in should use git-describe, built from
> the source before the tarball was made, to get the version
> number, and wouldn't be "$anything.GIT", which is the last-ditch
> fallback string, which is set by GIT-VERSION-GEN for people who
> build in a wrong way.

Just built the tarball and tried the produced specfile: it wanted to build 'git-1.5.1.rc1.26.g7a88-dirty'. Yes, my repository was dirty, I admit it. Maybe you're right and there is no good reason for the '-' symbols in the version string.

-- 
Eygene
Junio C Hamano· Mar 29, 2007, 08:35 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> Or you meant something different? I am little confused with
> the '$PATH' in your example. Was it intended?

Yes, the dollar-sign-in-pathname is pretty much a part of my pathological example.

Show 14 quoted lines
>> But come to think of it, it lets shell handle $PATH to find wish
>> anyway, so *unless* we have specific version dependency to wish
>> that wish binary normally found on user's $PATH is inadequate,
>> we probably should not even need to be doing any of this path
>> munging.  You might end up discovering the path to wish binary
>> in your autoconf script, we do not have to use it.  ./configure
>> can just see if there is wish, and set NO_TCLTK appropriately
>> without any of the path business.
>> 
>> What do you think?
>
> There are problems at least with FreeBSD: it just installs the
> wish8.4, wish8.3, wish8.2, etc. It does not provide the bare 'wish'
> as the link to one of those.

Then sed -e 's/wish/$(WISH_NAME)/', still letting the shell to handle the path part, could be a simpler option. I dunno.

Eygene Ryabinkin· Mar 29, 2007, 08:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Thu, Mar 29, 2007 at 01:35:55AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> 
> > Or you meant something different? I am little confused with
> > the '$PATH' in your example. Was it intended?
> 
> Yes, the dollar-sign-in-pathname is pretty much a part of my
> pathological example.

Got it. So you mean that the '$' sign should be escaped as well? ;)) Or we can get another _SQ substitution to the script and the string will look like exec 'whatever you'\''d written here' "$0" -- "$@"

Show 17 quoted lines
> >> But come to think of it, it lets shell handle $PATH to find wish
> >> anyway, so *unless* we have specific version dependency to wish
> >> that wish binary normally found on user's $PATH is inadequate,
> >> we probably should not even need to be doing any of this path
> >> munging.  You might end up discovering the path to wish binary
> >> in your autoconf script, we do not have to use it.  ./configure
> >> can just see if there is wish, and set NO_TCLTK appropriately
> >> without any of the path business.
> >> 
> >> What do you think?
> >
> > There are problems at least with FreeBSD: it just installs the
> > wish8.4, wish8.3, wish8.2, etc. It does not provide the bare 'wish'
> > as the link to one of those.
> 
> Then sed -e 's/wish/$(WISH_NAME)/', still letting the shell to
> handle the path part, could be a simpler option.  I dunno.

Ah, you mean that './configure --with-tcltk=wish8.4' should also do the trick? It seems to be easy to achieve by just skipping the 'test -x' part in the configure.ac. So the semantics of '--with-tcltk=PATH' will be: "If you're telling me about the path to the interpreter, it is you who should take care of it. I do not mind if you will give me something unexecutable, unexistent and so on.". Comments?

-- 
Eygene
Tom Prince· Mar 29, 2007, 09:12 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

On Thu, Mar 29, 2007 at 12:58:35PM +0400, Eygene Ryabinkin wrote:
Show 7 quoted lines
> Ah, you mean that './configure --with-tcltk=wish8.4' should also
> do the trick? It seems to be easy to achieve by just skipping
> the 'test -x' part in the configure.ac. So the semantics of
> '--with-tcltk=PATH' will be:
> "If you're telling me about the path to the interpreter, it is
> you who should take care of it. I do not mind if you will give
> me something unexecutable, unexistent and so on.". Comments?

Definitely, when cross compiling, or generating packages, you often don't even have the program installed in the right place, so erroring out is the wrong thing to do in this case.

  Tom
Eygene Ryabinkin· Mar 29, 2007, 10:06 UTC · re: Tom Prince · lore

Re: [PATCH] Add --with-tcltk and --without-tcltk to configure.

Tom, good day.
Thu, Mar 29, 2007 at 01:12:39PM +0400, Tom Prince wrote:
> Definitely, when cross compiling, or generating packages, you often
> don't even have the program installed in the right place, so erroring
> out is the wrong thing to do in this case.
OK, the patch follows.
-- 
Eygene
Eygene Ryabinkin· Mar 29, 2007, 10:06 UTC · re: Eygene Ryabinkin · lore

[PATCH] Eliminate checks of user-specified Tcl/Tk interpreter.

Do not make the checks on the Tcl/Tk interpreter passed by '--with-tcltk=/path/to/wish' configure option: user is free to pass anything.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 configure.ac |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
Show changes to configure.ac +0 −2
diff --git a/configure.ac b/configure.ac
index 43a6769..50d2b85 100644
--- a/configure.ac
+++ b/configure.ac
@@ -102,8 +102,6 @@ if test -z "$NO_TCLTK"; then
   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"

-- 
1.5.0.3-dirty
Eygene Ryabinkin· Mar 29, 2007, 10:07 UTC · re: Eygene Ryabinkin · lore

[PATCH] Added correct Python path to the RPM specfile.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 git.spec.in |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Show changes to git.spec.in +7 −2
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
Junio C Hamano· Mar 26, 2007, 08:25 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Added make options NO_GUI and WITH_P4IMPORT.

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>> Maybe later you might even want to view the graphical history
>> from the server displaying on remote X, who knows?
>
> Not sure: I don't like X on the servers ;)) But others can...

Did you know that you can have only minimum X clients on your server machine, and display on remote X terminals? But that is besides the point.

Show 9 quoted lines
>> We have NO_CURL and such because lack of the necessary libraries
>> and headers prevent your build from completing, but in the case
>> of git-gui and gitk, they are just scripts and you would not
>> have any trouble in building.  I do not know if adding more
>> conditional to Makefile in order to skip them is worth it.
>
> OK, I reworked the patch following the suggestion of Jakub and Johannes:
> now configure has the built-in detection of the Tcl/Tk binary and has
> the --with-tcltk/--without-tcltk options.

That is exactly what I am quite against. I often find other people's packages silly when they disable tk support only because the build procedure does not find tcl/tk installed on the system it is built on, even when the tk component of the package is pure wish script and does not have any C native stuff (which requires libtcl development component on the build system, which in turn justifies such disabling).

Show 5 quoted lines
> And regarding the building troubles and the additional knob in the
> Makefile: the trouble is in the packaging process. For example, RPM
> or FreeBSD ports are looking at what is really installed, so if
> user do not want the Tcl/Tk part, then no package parts that depend
> on it should be installed.

I think the simple RPM spec file we ship with git.git takes care of that nicely by splitting gitk into a separate package (As it was Chris Wright's work, I cannot take credit for that part at all). I would imagine both modern distro's packaging system and people who actually maintain packages for distros are capable enough to handle this situation just fine. I still do not think the "packaging difficulty" is not a strong enough reason. 

But I could be persuaded otherwise...

← back to recent threads