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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 14, 2010, 17:22 UTC
Message-ID: <201004141922.20213.jnareb@gmail.com>
In-Reply-To: <4BC55558.1060608@mailservices.uwaterloo.ca>

On Wed, 14 Apr 2010, Mark Rada wrote:

> On 10-04-13 6:30 PM, Jakub Narebski wrote:
>> On Tue, 13 April 2010, Charles Bailey wrote:
>>> On 01/04/2010 06:36, Mark Rada wrote:

[...]

Show 57 quoted lines

>>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
>>>>
>>>
>>> I have a question about this last line of the patch. Are GITWEB_JS and 
>>> GITWEB_CSS supposed to be a source path or a URI?
>>>
>>> The documentation for install (and my previous assumption) was that they 
>>> represented the path on the target web server. I'm used to overriding 
>>> them so that gitweb.cgi can live in my /cgi-bin directory, but the 
>>> static files are served from /gitweb which is readable but not executable.
>>>
>>> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from 
>>> the list of dependencies for gitweb.cgi otherwise make failed.
>>>
>>> Have I got the wrong end of the stick?
>> 
>> Thanks a lot for noticing this bug.
>> 
>> 
>> 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.
>> 
>> If I remember correctly the original patch, before adding required
>> support for minified gitweb.js and gitweb.css to git-instaweb script,
>> and before support for CSS minification had
>> 
>>    ifdef JSMIN
>>    gitweb.cgi: gitweb.perl gitweb.min.js
>>    else
>>    gitweb.cgi: gitweb.perl
>>    endif
>> 
>> which should probably be replaced in current situation by
>> 
>>    ifdef JSMIN
>>    gitweb.cgi : gitweb.min.js
>>    endif
>>    ifdef CSSMIN
>>    gitweb.cgi : gitweb.min.css
>>    endif
>> 
>> just adding prerequisites to gitweb.css target in gitweb/Makefile
>> 
>> 
>> I guess that support for adding minifiction support to git-instaweb
>> would need to be more complicated. [...]
> 
> Aw, frig, never thought of using gitweb like that so I made some
> assumptions to make things cleaner looking.
> 
> I think this can be fixed by just using different variable names? Or
> perhaps some nested ifdef's? I'm not sure which will be better.
> 
> I wasn't at the computer today so I'm just getting to it now, I'll try
> to have something when in the next day, going to bed now. Good night.

I think the best solution for prerequisites would be to have multiple target-only (without commands) rules, which according to make documentation would get concatenated. This means the following code in gitweb/Makefile:

    ifdef JSMIN
    gitweb.cgi : gitweb.min.js
    endif
    ifdef CSSMIN
    gitweb.cgi : gitweb.min.css
    endif

in place of

   gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)

For git-instaweb I think that best solution would be to introduce new variables holding _source_ of gitweb JavaScript code and CSS, e.g.

            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \

in place of

            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \

...although GITWEB_CSS might mean something different for Makefile and git-instaweb than for gitweb/Makefile and gitweb itself.

-- 
Jakub Narebski
Poland