Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-1.fc16.src.rpm Description: Horde-specific PHPUnit base classes.
I am not an official package reviewer, but here are some comments I have for this package: *** It would help readability to group your Build* and Requires* statements together instead of mixing them with each other and your Provides and Conflicts statements *** Your Provides statement should be Provides: php-pear(pear.horde.org/%{pear_name}) = %{version} instead of Provides: php-pear(%{pear_name}) = %{version} *** You should run phpci on your packages to make sure you have all requires: $ phpci print --recursive --report=extension src/Horde_Test-1.3.0 22 / 22 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++>] 100.00% BASE: /projects/fedora/REVIEWS/785606/src/Horde_Test-1.3.0 ------------------------------------------------------------------------------- PHP COMPAT INFO EXTENSION SUMMARY ------------------------------------------------------------------------------- EXTENSION PECL VERSION COUNT ------------------------------------------------------------------------------- Core 4.0.0 52 PDO 1.0.4dev 5.1.0 1 SPL 0.2 5.0.0 8 dom 20031129 5.0.0 4 json 1.2.1 5.2.0 1 pcre 4.0.0 4 standard 4.0.0 63 ------------------------------------------------------------------------------- A TOTAL OF 7 EXTENSION(S) WERE FOUND REQUIRED PHP 5.2.0 (MIN) ------------------------------------------------------------------------------- Time: 1 second, Memory: 9.25Mb ------------------------------------------------------------------------------- According to phpci, you need to require "php-pdo" and "php-dom" (virtual provide of php-xml) at a minimum. File Horde_Test-1.3.0/lib/Horde/Test/Functional.php uses the dom functions. File Horde_Test-1.3.0/lib/Horde/Test/Factory/Db.php uses the PDO functions. While package.xml lists these as optional, I believe this package would provide a better end-user experience if you added the two requires: Requires: php-pdo Requires: php-dom spl, json, and pcre are all virtual provides from php-common. For completeness (and to prevent any future packaging issues due to PHP package changes) my sponsor and I have agreed to require all virtual provides. It is up to you, but I would suggest adding the following as well: Requires: php-spl Requires: php-json Requires: php-pcre *** Do you plan on building for EPEL 5? - If not, please remove "rm -rf $RPM_BUILD_ROOT" from the %install section (see http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag). - If you do, there are several staements that need to be added. *** Up to you, but package.xml lists the following optional packages if you choose to require: - php-pear(pear.horde.org/Horde_Cli) - php-pear(pear.horde.org/Horde_Log) (if you do choose to require these packages, please update this ticket's "Depends On")
I believe I have fixed these issues. Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-2.fc16.src.rpm
(In reply to comment #2) Almost ready for official review. Please change Requires: php-pear(pear.horde.org/Horde_Cli) Requires: php-pear(pear.horde.org/Horde_Log) to Requires: php-pear(pear.horde.org/Horde_Cli) < 2.0.0 Requires: php-pear(pear.horde.org/Horde_Log) < 2.0.0 per package.xml requirements
I believe I have fixed these issues. Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-3.fc16.src.rpm
Created attachment 601950 [details] php-horde-Horde-Test-review.txt Generated by fedora-review 0.2.0 (53cc903) last change: 2012-07-09
Could: remove "localized" php.ini (no more useful) Could: requires php(language) >= 5.2.0 per new PHP Guildelines, but this is fedora specific (for now), so php-common seems acceptable as you target both fedora/epel MUST Package installs properly: Need to wait for Horde_Log and Horde_Constrant to be approved before import Lot of used class are not listed in requires As they are not listed by upstream, I think we can ommit them (optional) Ex, in /usr/share/pear/Horde/Test/Factory/Alarm.php if (!class_exists('Horde_Alarm')) { throw new Horde_Test_Exception('The "Horde_Alarm" class is unavailable!'); } return new Horde_Alarm_Null(); Package with tests which use the Horde_Test_Factory_Alarm will have to BR both Horde_Test and Horde_Alarm. After this package, I think others horde package, providing tests, will have to run them in the %check.
No blocker ========= APPROVED =========
New Package SCM Request ======================= Package Name: php-horde-Horde-Test Short Description: Horde-specific PHPUnit base classes Owners: nb Branches: el6 f16 f17 InitialCC:
Git done (by process-git-requests).