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

[Eygene Ryabinkin <rea-git@codelabs.ru>]

Junio, good day.

Tue, Mar 27, 2007 at 03:53:33AM -0700, Junio C Hamano wrote:
> > +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.

> 
> > @@ -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