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

[Mark Rada <marada@uwaterloo.ca>]

On 10-04-14 1:22 PM, Jakub Narebski wrote:
> 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:
> 
> [...]
>>>>> +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)
> 

Hmm, I like this because it is clear (if you know that dependancies
can be joined like that), I was thinking of trying to make a smaller fix
using pathsubst or subst, but that seems to not be as simple as I wanted it
to be.

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

Did you get those lines mixed up? I might be not understanding something
here.

I was actually planning something along the lines of 

             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_NAME)' \
             -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS_NAME)|' \

where I introduce the GITWEB_CSS_NAME variable, to be consistent with the
token in instaweb. This way we don't touch GITWEB_JS in the top level
makefile.

Also, I should update dependancies for instaweb, since those were
forgotten last time around. Just creating a short list of what the fix will
need for when I get home tonight.

-- 
Mark Rada