Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 15, 2010, 00:25 UTC
Message-ID: <201004150225.42101.jnareb@gmail.com>
In-Reply-To: <7viq7tmvsb.fsf@alter.siamese.dyndns.org>

On Thu, 15 Apr 2010, Junio C Hamano wrote:

Show 23 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
>> On Tue, 13 April 2010, Charles Bailey wrote:
>>> On 01/04/2010 06:36, Mark Rada wrote:
>>>> @@ -84,13 +92,14 @@ endif
>>>>
>>>>   all:: gitweb.cgi
>>>>
>>>> +FILES = gitweb.cgi
>>>>   ifdef JSMIN
>>>> +FILES += gitweb.min.js
>>>> +endif
>>>> +ifdef CSSMIN
>>>> +FILES += gitweb.min.css
>>>> +endif
>>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
>>
>> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with
>> gitweb JavaScript code and default gitweb stylesheet,... but during work
>> on minification of JavaScript code and CSS file it somehow got confused
>> to mean source path.
> 
> I am not touching instaweb part, but this would fix the build/clean side
> of the things, no?

Close, see below.

Show 9 quoted lines

> 
> -->8 --
> gitweb: simplify gitweb.min.* generation and clean-up rules
> 
> GITWEB_CSS and GITWEB_JS are meant to be "what URI should the installed
> cgi script use to refer to the stylesheet and JavaScript", never "this is
> the name of the file we are building".
> 
> Lose incorrect assignment to them.

Actually the assignment was intended to provide correct *default* values for GITWEB_CSS and GITWEB_JS, so that if e.g. JSMIN is defined gitweb, in absence of build time configuration, would link gitweb.min.js instead of gitweb.js.

Show 7 quoted lines

> 
> While we are at it, lose FILES that is used only for "clean" target in a
> misguided way.  "make clean" should try to remove all the potential build
> artifacts regardless of a minor configuration change.  Instead of trying
> to remove only the build product "make clean" would have created if it
> were run without "clean", explicitly list the three potential build
> products for removal.

Good.

Show 23 quoted lines

> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  gitweb/Makefile |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index ffee4bd..1787633 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -80,16 +80,7 @@ endif
>  
>  all:: gitweb.cgi
>  
> -FILES = gitweb.cgi
> -ifdef JSMIN
> -FILES += gitweb.min.js
> -GITWEB_JS = gitweb.min.js
> -endif
> -ifdef CSSMIN
> -FILES += gitweb.min.css
> -GITWEB_CSS = gitweb.min.css
> -endif

I wonder about removing assigmnet to GITWEB_JS and GITWEB_CSS. Without it "make gitweb" (in top dir) would create gitweb/gitweb.cgi and gitweb/gitweb.min.js etc.... but generated gitweb/gitweb.cgi would refer to gitweb.js, not gitweb.min.js. Unless of course one provides values for GITWEB_JS during build time.

Show 19 quoted lines

> -gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
> +gitweb.cgi: gitweb.perl
>  
>  gitweb.cgi:
>  	$(QUIET_GEN)$(RM) $@ $@+ && \
> @@ -118,16 +109,18 @@ gitweb.cgi:
>  	mv $@+ $@
>  
>  ifdef JSMIN
> +all:: gitweb.min.js
>  gitweb.min.js: gitweb.js
>  	$(QUIET_GEN)$(JSMIN) <$<>$@
>  endif # JSMIN
>  
>  ifdef CSSMIN
> +all:: gitweb.min.css
>  gitweb.min.css: gitweb.css
>  	$(QUIET_GEN)$(CSSMIN) <$>$@
>  endif

That makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css. It might be right... and I think the rightness or wrongness might be tied with values of GITWEB>JS and GITWEB_CSS.

Show 7 quoted lines

>  
>  clean:
> -	$(RM) $(FILES)
> +	$(RM) gitweb.cgi gitweb.min.css gitweb.min.js
>  
>  .PHONY: all clean .FORCE-GIT-VERSION-FILE
>

-- 
Jakub Narebski
Poland