Thanks for reviewing this. See inline comments:
Sriram Natarajan wrote:
> HI
>
> - You might want to take a look at the indentation within
> lighthttpd/Makefile.sfw
>
> - Why not simply do it as gtar xf and avoid manually changing the
> permission later
>
> + $(GTAR) xpf - --no-same-owner
> + touch $(LIGHTTPD)/configure
> + find $(LIGHTTPD) -type d -exec /usr/bin/chmod 755 "{}" \;
> + find $(LIGHTTPD) -type f -exec /usr/bin/chmod ugo+r "{}" \;
The way we are doing it here maybe over cautious but it does seem to
avoid any possible permissions problems. I'd be happy to do it the
simpler way, but am thinking that albeit historical, this was done for
other integrations for a reason. I wonder if anyone has any knowledge of
why this was done this way, if not I'll just change it to the simpler form.
>
>
> - Instead of using hard coded PHP path , you might want to use it in
> a dynamic way. This is because PHP 5.2.6 integration is coming pretty
> soon and is going to have a different file layout.
I could just use /usr/bin/php-cgi for the executable. Will 5.2.6 use
the same /usr/bin/php-cgi symlink? Would that be stable enough? For the
php.ini file things are a little more difficult and I need to look into
this some more.
>
> - With MySQL, you will now start the server as svcadm enable
> mysql:version_50. Looks like, Apache and Lighttpd is going to start
> their servers like svcadm enable http:apache22 and http:lighthttpd14.
> Should we not all do this in a consistent way ? I guess, this is a
> separate discussion.
I agree, we should have this discussion. version_50 is somewhat
different from lighttpd14, it feels that 'version' and '50' need to be
separated with an underscore, but I don't get the same feeling for
lighttpd14 or apache22. I wonder if mysql:version_50 might have worked
as mysql:50
>
> - within the man page, there is a reference to memcached. Probably a
> miss in cut and paste .
Great spot, I shall fix both of these references
>
> - While installing man pages, should we not use the convention of
> _install M ?
I've done this with the main lighttpd.1 page, which is a page that I
created, to make this work I created the sunman-stability sed script
required by _install M. The Lighttpd man pages are patched to add some
more detail but they are installed by the standard make, not by the
script. I can changed this, but looking at PHP, MySQL and Apache, none
of their community provided man pages have the sunman-stability data
appended.
>
> - If the binaries and libraries are installed using _install , then
> we can be sure that the symbols get stripped out. I see some
> components doing this and some not. I am not sure, what is the
> convention within SFW ?
All 3 of the executables are stripped by the $SRC/tools/post_process
command and all of the libs are stripped by $SRC/tools/post_process_so.
I don't think I'm missing any and I was fairly confident that this was
the right way to do it if not using _install. Did you see any
specifically that I missed?
Thanks
Amanda
>
> Hope this helps
> Sriram
>
> Amanda Waite wrote:
>> Hi there,
>>
>> Before we do the putback on Lighttpd we'd like to get a few more
>> eyeballs to go over the code and give us a thorough review. Not a
>> great deal has changed since the last code review, but we have
>> removed the horrible LD_LIBRARY_PATH statement from Makefile.sfw and
>> no longer ask the configure script to use mysql_config.
>>
>> The code review is at
>> http://cr.opensolaris.org/~tekgrrl/lighttpd-putback/
>>
>> We would really appreciate your feedback.
>>
>> Thanks
>>
>> Amanda - ISV-Engineering
>> _______________________________________________
>>
>>
>> webstack-discuss mailing list
>> webstack-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss
>>
