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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html