Code review comment for lp:~grant-delaney/ius/php53

Revision history for this message
BJ Dierkes (derks) wrote :

This looks really good... and from what I can tell, FPM is only built into the php-fpm binary which would be required as to not mess with core php. A couple issues and thoughts:

 - Wrap a '_with_fpm' condition in there (similar to _with_zts, _with_psa, etc). So everything added for 'fpm' should be within a '%if 0%{?_with_fpm}' condition. I think I would prefer to put this in there optionally (meaning users can rebuild from the srpm if they want fpm)... until I am comfortable adding it by default.

 - "Requires: %{name}-%{version}" ... should be "Requires: %{name} = %{version}-%{release}" under %package fpm

 - "Provides: php-fpm" should be "Provides: %{real_name}-fpm = %{version}-%{release}

 - Under '%install' sometimes you reference %{_sysconfdir} and others raw '/etc'... should use the macro consistently.

 - %defattr missing from %files fpm

 - Any README, LICENSE, COPYING, from the fpm source need to be included under a %doc under %files fpm

review: Needs Fixing

« Back to merge proposal