For completeness, here's a link to an updated webrev after changing the path to php-cgi in the partial config file. Everything else is the same, but I think that others want to review it before going to the next stage.
http://cr.opensolaris.org/~tekgrrl/lighttpd-putback-2 Thanks Amanda Amanda Waite wrote: > Hi Siriam, > > Sriram Natarajan wrote: > >> HI >> Looks good to me except for couple of suggestions >> >> - Did you try compiling with -xO4 ? With respect to Apache, I noticed >> around 5% gain by simply compiling with apache with -xO4. It seems that >> Apache 2.2.9 is going to come with this flag (-xO4). Af course, you >> could integrate as is and then file a bug to track this issue. >> >> > > We are taking the cautious approach initially, our tests haven't shown > any significant benefits with -xO4 over -xO3 at least not on the > workloads I've tested with. As you say, we can consider other > optimisations post integration. > >> - If I am not mistaken, there is no /usr/bin/php-cgi . php-cgi is only >> under /usr/php5/5.2.4/bin/php-cgi. Once this is integrated, I will file >> a bug to ensure that PHP 5.2.6 integration handles this change in >> location as well. >> >> > > You're right, in order to run the test suite on an installed version of > Lighttpd we tend to link php-cgi into /usr/bin as opposed to changed 5 > or 6 config files used by the tests. I'll change it back to 5.2.4 path, > these paths will be invalid soon anyway, but at least for now they'll be > based on the real world. > > Thanks for the review > > Amanda > > >> - Sriram >> >> Amanda Waite wrote: >> >> >>> http://cr.opensolaris.org/~tekgrrl/lighttpd-putback-1 is an updated >>> webrev based on comments received so far. >>> >>> Items addressed: >>> >>> - Indentation of Makefile.sfw >>> - Changed the fcgi-php.conf file to use a variable for the php command >>> string and added a note >>> in the comments at the head of the file. >>> - Created a sunman-stability file and modified the man page to remove >>> duplicate information. >>> - Changed the install script to use _install M to install the man page >>> >>> Items not addressed >>> >>> - Use of gtar -xpf >>> - We didn't actually change this as it seems the most popular way of >>> doing it and because I'm >>> scared of breaking what works >>> >>> - service instance name change. >>> - Needs further discussion and some kind of agreement on convention, >>> is currently consistent with >>> Apache 2.2 >>> >>> - Running _install to install binaries >>> - We believe that post_process and post_process_so do the same thing >>> and it seems that all >>> binaries are run through one of these commands >>> >>> Please review and let us have your feedback. >>> >>> Thanks >>> >>> Amanda >>> >>> >>> Amanda Waite wrote: >>> >>> >>> >>>> I've looked at the options for having lighttpd pick up the PHP paths >>>> dynamically and there really are none. The only thing I can do is to >>>> make the fcgi-php.conf file into a sample file or remove it >>>> altogether. Leaving it as it is doesn't seem to be an option as the >>>> path to the ini file will be invalid soon. >>>> >>>> I can define variables in the config file so I could set the paths to >>>> be variables and then make it clear that they need to be set in order >>>> to make it work correctly. >>>> >>>> Thanks >>>> >>>> Amanda >>>> >>>> >>>> Amanda Waite wrote: >>>> >>>> >>>> >>>>> 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 >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> _______________________________________________ >>>>> >>>>> >>>>> webstack-discuss mailing list >>>>> webstack-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> _______________________________________________ >>> >>> >>> webstack-discuss mailing list >>> webstack-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >>> >>> >>> >> _______________________________________________ >> >> >> webstack-discuss mailing list >> webstack-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >> >> > > _______________________________________________ > > > webstack-discuss mailing list > webstack-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >
