Re: [PATCH 4/6] GITWEB - Makefile changes

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 11 Dec 2009, J.H. wrote:

>> IMPORTANT!
>> 
>> A note about this change: I think it would be better to move creating
>> gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make
>> main Makefile call gitweb/Makefile, and not vice versa like in your
>> solution.
>> 
>> If it is possible.
> 
> It's quite possible, and I'm fine with doing that.  If no one has any 
> objections I can re-work those with the understanding that the build 
> process for gitweb shift to the gitweb/ directory instead of the main 
> Makefile.

In my opinion it would be better solution because it would reduce size
of main (master) Makefile, and not be much larger than this solution.
 
git-gui/, Documentation/, perl/ all have their own makefiles, which do
the work, and are called from main (master) Makefile.

>>> diff --git a/Makefile b/Makefile
>>> index 4a1e5bc..8db9d01 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>>>  	chmod +x $@+ && \
>>>  	mv $@+ $@
>>>  
>>> +.PHONY: gitweb
>> 
>> Why it is here, and not with the .PHONY block at line 1924 of
>> Makefile?  It would be nice to have comment supporting this choice in
>> email with this patch (or in commit message).
> 
> There are 6 other instances of .PHONY in the makefile, having the .PHONY 
> localized seemed to make it the most obvious since it was right next to 
> the actual target.

I was thinking here about this large block of .PHONY declarations,
the one which is not inside conditional.
 
>>> +gitweb: gitweb/gitweb.cgi
>>>  ifdef JSMIN
>>>  OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
>>>  gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
>>> @@ -1537,7 +1539,7 @@ endif
>>>  	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
>>>  	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
>>>  	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
>>> -	    $< >$@+ && \
>>> +	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
>> 
>> Why this change?
> 
> Preparation for a later change.  The change could happen all at the same 
> time if it makes more logical sense.

Please at least describe this change in commit message.

But I think it could be moved to other patch, or put in separate patch.
This change has nothing to do with easier gitweb generation.
 
>>>  	chmod +x $@+ && \
>>>  	mv $@+ $@
>>>  
>>> diff --git a/gitweb/Makefile b/gitweb/Makefile
>>> new file mode 100644
>>> index 0000000..8d318b3
>>> --- /dev/null
>>> +++ b/gitweb/Makefile
>>> @@ -0,0 +1,14 @@
>>> +SHELL = /bin/bash
>> 
>> Why is this needed?

Why do you need to define SHELL?
 
>>> +
>>> +FILES = gitweb.cgi
>>> +
>>> +.PHONY: $(FILES)
>> 
>> Why .PHONY?  $(FILES) are created.
> 
>  From this makefile I wanted to explicitly call up to the main makefile 
> no matter what, the main makefile doesn't consider the targets .PHONY 
> and it has all the dependencies that it would expect.

What is the reason of this phony .PHONY?  If gitweb.cgi is newer than
gitweb.perl (and other sources), then without .PHONY it wouldn't be 
regenerated.  With .PHONY it would call master Makefile... which would
notice that gitweb.cgi is newer than gitweb.perl and do not regenerate.
 
So what this .PHONY does is unnecessary call make on master Makefile...



But I guess this issue would be moot if it was the other way around,
i.e. master Makefile calling gitweb/Makefile.

-- 
Jakub Narebski
Poland