On 08/09/2012 05:00 PM, Eric Blake wrote:
> On 08/09/2012 08:17 AM, Martin Kletzander wrote:
>> This patch makes search.php autogenerated from search.php.in, thus
>> removing hardcoded menus, footer etc. and the search.php is added to
>> .gitignore.
>>
>> There is new rule added for *.php files (to make it bit less
>> hardcoded) that takes *.php.code.in and injects it inside the
>> generated *.php (xslt was not happy about php code in the source xml).
>> ---
>
> I will flat-out admit I've never coded in php. However, I can at least
> review the makefile portions for correctness, as well as apply and test
> that the patch does what it claims (and indeed, the generated search.php
> file resembles the original checked in version, with the differences
> being improvements in the boilerplate now that it went through the same
> code generation steps as the rest of our pages).
>
> I did _not_ test a 'make dist' from a VPATH build; there's a slight
> possibility that we may need some cleanup patches as a result, but I'm
> okay with that risk.
>
I'll try that, but I won't probably find necessarily all the issues that
might occur.
>> @@ -173,6 +178,19 @@ internals/%.html.tmp: internals/%.html.in subsite.xsl page.xsl sitemap.html.in
>> || { rm $(srcdir)/$@ && exit 1; }; \
>> else echo "missing XHTML1 DTD" ; fi ; fi
>>
>> +%.php.tmp: %.php.in site.xsl page.xsl sitemap.html.in
>> + @if [ -x $(XSLTPROC) ] ; then \
>> + echo "Generating $@"; \
>> + name=`echo $@ | sed -e 's/.tmp//'`; \
>
> This works, although '\.' is technically tighter than '.' for matching
> the literal '.' in the file name. But why bother with sed and `` in the
> first place? You've go make to do it for you:
>
> name=$(@:.tmp=)
>
Yes, I haven't realized that when remaking other rule to suit my needs,
but it's definitely better looking, easier and faster.
All the other rules use this, so I'll see what I can do with that later
(maybe some bigger cleanup wouldn't hurt), but ...
>> + $(XSLTPROC) --stringparam pagename $$name --nonet --html \
>
> or even skip $$name and directly inline $(@:.tmp=) here.
>
... I went with this version in this patch.
>> + $(top_srcdir)/docs/site.xsl $< > $@ \
>> + || { rm $@ && exit 1; }; fi
>> +
>> +%.php: %.php.tmp %.php.code.in
>> + @echo "Scripting $@"; \
>> + sed -e '/<a id="php_placeholder"><\/a>/r '"$(srcdir)/$@.code.in"\
>
> Missing space before \, for consistent style.
>
Fixed.
> ACK with those minor tweaks.
>
Thanks, pushed.
Martin
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list