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

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

quoted

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.

quoted

quoted

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.
 

quoted

quoted

+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.
 

quoted

quoted

 	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?
 

quoted

quoted

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