Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log-1.1.2-1.fc16.src.rpm Description: Horde Logging package with configurable handlers, filters, and formatting.
I will review this package
After a quick review: [MUST] "Requires: php-common >= 5.2.0" to satisfy package.xml [SHOULD] %check is present and all tests pass [SHOULD] Localized php.ini not necessary (see https://bugzilla.redhat.com/show_bug.cgi?id=785471#c5)
Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log-1.1.2-2.fc16.src.rpm
I believe these issues have been fixed (except for that I can't use %check until Constraint and Log are in the repo so I can build Test) Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log-1.1.2-2.fc16.src.rpm
Sorry! I just noticed you are waiting for this review before importing Horde_Test and being able to run tests for other Horde packages (https://bugzilla.redhat.com/show_bug.cgi?id=785606#c6). Could you update to the latest version (2.0.1) before review?
Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log-2.0.1-1.fc17.src.rpm
Created attachment 664213 [details] php-horde-Horde-Log-review.txt Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-17-x86_64 Command line :/usr/bin/fedora-review -b 785480
(In reply to comment #7) ===== MUST items ===== [!]: Requires correct, justified where necessary. According to phpci report (and "Horde/Log/Formatter/Xml.php" source), requires "php-dom" ===== SHOULD items ===== [!]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. ===== COULD items ===== * Localized php.ini not necessary (also note that you not create it in %prep for use in %install) * In %files, "%{pear_testdir}/Horde_Log" could be changed to "%{pear_testdir}/%{pear_name}" * Require the following virtual packages: php-date, php-pcre, php-reflection, php-spl * When adding tests in %check in the future, add the require virtual packages above along with the following: php-simplexml, php-libxml
(In reply to comment #8) > ===== SHOULD items ===== > > [!]: If the source package does not include license text(s) as a separate > file > from upstream, the packager SHOULD query upstream to include it. Disregard this comment. I was looking for a license file labeled LICENSE, but the license text is included in the COPYING file.
Updated Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Log-2.0.1-2.fc17.src.rpm
Created attachment 665846 [details] phpci.log
Created attachment 665851 [details] phpci.log
Created attachment 665852 [details] php-horde-Horde-Log-review.txt Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-rawhide-x86_64 Command line :/usr/bin/fedora-review -b 785480 --mock-config fedora-rawhide-x86_64
After initial import: * Remove "Requires: php-simplexml php-libxml" as these are only build requires * Please remove "PHPRC=../php.ini" from %install No blockers. ===== APPROVED =====
New Package SCM Request ======================= Package Name: php-horde-Horde-Log Short Description: Horde Logging package with configurable handlers, filters, and formatting Owners: nb Branches: f17 f18 el6 InitialCC: remi
Git done (by process-git-requests).