Bug 785480 (Horde_Log)

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).