Build Failure: GIT-GUI-VARS

Build Failure: GIT-GUI-VARS

[Brian Gernhardt]

"make all" fails with "NO_TCLTK=y" because gitk-wish depends on GIT- 
GUI-VARS which is defined inside the ifndef block.

AFAICT, either gitk-wish should be moved inside a ifndef block or an  
else should be added with a phony GIT-GUI-VARS.  I don't know which  
is preferred.

Actually, I can't tell why make is trying to build it in the first  
place.  :-/  gitk-wish is added to OTHER_PROGRAMS inside a NO_TCLTK  
block. (Debian oldstable, GNU Make 3.80, git repo v1.5.2-rc3)

And, while I'm poking through here, there's a line labeled "Backward  
compatibility -- to be removed after 1.0".  Git IDs itself as  
1.5.2.rc3.  Isn't that after 1.0?  :-)

~~ Brian

Re: Build Failure: GIT-GUI-VARS

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> Actually, I can't tell why make is trying to build it in the first
> place.  :-/  gitk-wish is added to OTHER_PROGRAMS inside a NO_TCLTK
> block.

I do not either, and obviously does not reproduce for me.  Time
for you to try "make -d" perhaps?

Re: Build Failure: GIT-GUI-VARS

[Brian Gernhardt]

On May 10, 2007, at 10:30 PM, Junio C Hamano wrote:

> I do not either, and obviously does not reproduce for me.  Time
> for you to try "make -d" perhaps?

Sorry for the semi-coherent message.  It started out as "we should  
fix this" and I ended up going "why is it broken", all a little too  
late at night.  I'm also not used to debugging Makefiles.

And I figured it out (make -p actually was more useful than -d):   
It's another issue with doing things before including config.mak.  My  
config.mak includes the "NO_TCLTK=y" line, which takes effect at  
Makefile:485.  But gitk-wish is getting included in OTHER_PROGRAMS at  
Makefile:272.  So gitk-wish gets included because NO_TCLTK isn't  
defined at that point, but all the other rules involving gitk don't  
get executed because they're after the NO_TCLTK definition.

I think we need to be including the config.mak much earlier in the  
Makefile as this is not the first time issues like this have come up  
for me.  And they tend to be issues that only come up with specific  
configurations and are therefore harder to track down.  Things I can  
see being an issue for people who put their options in config.mak:

149: ifeq ($(prefix),/usr)
224: ifdef WITH_P4IMPORT
271: ifndef NO_TCLTK   # My issue today
279-285: ifndef {SHELL,PERL,PYTHON}_PATH

~~ Brian

Re: Build Failure: GIT-GUI-VARS

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> On May 10, 2007, at 10:30 PM, Junio C Hamano wrote:
>
>> I do not either, and obviously does not reproduce for me.  Time
>> for you to try "make -d" perhaps?
>
> Sorry for the semi-coherent message.  It started out as "we should fix
> this" and I ended up going "why is it broken", all a little too  late
> at night.  I'm also not used to debugging Makefiles.
>
> And I figured it out (make -p actually was more useful than -d):  It's
> another issue with doing things before including config.mak.

Ahh, config.mak.

I have not been very sympathetic to config.mak from the
beginning, although people seem to want it.  As I try to arrange
variable overrides to be passed from the command line anyway,
I've not used config.mak myself.

> I think we need to be including the config.mak much earlier in the
> Makefile as this is not the first time issues like this have come up
> for me.

However, it needs to be able to override NO_*, NEEDS_* and
friends which means it should come after platform specific
defaults.  But one platform wants to change the default
behaviour depend on the settings of NO_*, possibly set in
config.mak.

It is a mess.  Moving it around would fix one person's problem
while breaking others.  I think the best course of action is

 (1) first for somebody more fluent in speaking Makefile than me
     to volunteer to tackle this problem, and ask help from the
     list;

 (2) have people on various "non-stock" configurations sign-up
     as testers; this does not have to cover the systems listed
     in the ifeq ($(uname_S),XXX) part of the current Makefile;
     what is of interest is to see what kind of variables are
     overridden on the real-world systems via config.mak;

 (3) The volunteer cooks up an improved Makefile, using
     config.mak "non-stock" testers have.

This would be something that "volunteer" would be deciding, but
I have a feeling that we can have two config.mak include in the
makefile, one early and one late.  But I suspect that picking
the right two places would have the same "moving would fix one
while breaking another" issue.

Re: Build Failure: GIT-GUI-VARS

[Jeff King]

On Fri, May 11, 2007 at 10:10:26AM -0700, Junio C Hamano wrote:

> I have not been very sympathetic to config.mak from the
> beginning, although people seem to want it.  As I try to arrange
> variable overrides to be passed from the command line anyway,
> I've not used config.mak myself.
>
> [...]
>
>  (3) The volunteer cooks up an improved Makefile, using
>      config.mak "non-stock" testers have.

I wonder if we would be better served by moving these sorts of
build-time configuration decisions into the actual make dependency
tree.  E.g., something like:

  openssl.lib: mklib-openssl.sh conf-openssl
    sh mklib-openssl.sh <conf-ssl >openssl.lib

  program: main.o openssl.lib:
    cc -o "$@" main.o `cat openssl.lib`

where conf-openssl specifies the user's preference (either actual
library paths, "auto" for autodetection, or "none" not to use it at
all), and mklib-openssl is a script that converts that into the command
line options for the link.

You can of course do the same with creating a .h file to choose an
implementation (you just make a file that #define's the correct thing).

The nice thing about this approach is that:
  1. You move configuration cruft out of the Makefile, making it much
     easier to read. Instead, you have a series of very small and
     obvious shell scripts.
  2. The dependency chain is actually correct. If I edit conf-openssl,
     then that should trigger a re-link for everything which compiles
     against it.

You can also use this for portability fixes:

  program: main.o strcasestr.o
    cc -o "$@" main.o strcasestr.o

  strcasestr.o choose try_strcasestr.c compat/strcasestr.c
    sh choose try_strcasestr.c compat/strcasestr.c

where choose is a script that compiles and runs some test program and
uses the result to choose a source file to become strcasestr.c. Thus you
_always_ link against strcasestr.o, it's just that sometimes there's an
implementation of strcasestr in it (if required by the platform) and
sometimes it's empty (or an alternate implementation, etc).

I have used this technique many times, and would be happy to be involved
in changing the Makefile. However, it's going to be quite a large
change, and I recognize that this style is not familiar to most people,
so obviously that should be taken into account.

-Peff