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:

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