Bug 785450 (Horde_Cache)

Updated

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache-1.0.5-1.fc16.src.rpm

I will review this package

I believe all of the normal blockers for the php-horde-Horde-* packages are fixed with this package.

[MUST] "Requires: php-common >= 5.2.0" to satisfy package.xml

[MUST] Change
    Requires:       php-pear(pear.horde.org/Horde_Util) <= 2.0.0
to
    Requires:       php-pear(pear.horde.org/Horde_Util) < 2.0.0
to satisfy package.xml



[OPTIONAL] For readability, you may want to group your Build* and Requires*

[OPTIONAL] I'm not sure what the exact Fedora packaging guidelines are for this, but I prefer to be very verbose and would list both the min and max requirements instead of just max (even though you know you don't have < 1.0.0 in Fedora):
Requires: php-pear(pear.horde.org/PACKAGE) >= 1.0.0
Requires: php-pear(pear.horde.org/PACKAGE) < 2.0.0

[OPTIONAL] You may want to consider adding requires for the following optional dependencies (from package.xml):
* php-pear(pear.horde.org/Horde_Db)
* php-pear(pear.horde.org/Horde_Log)
* php-pear(pear.horde.org/Horde_Memcache) -- be careful with this one as I don't know whether this would cause issues in certain environments like APC does (which is also an optional requirement of this package)

[OPTIONAL] phpci: You may want to consider adding the following requires:
* php-date
* php-hash
* php-spl
* php-pecl(LZF) >= 1.5.2
--- NOTE: package.xml lists the hash extension as required, but it is a virtual package of php-common (as php-hash) so it would automatically be installed.  You may want to consider adding the requirement to be thorough and prevent any future issue of there is PHP package changes.
--- NOTE: You may want to have upstream add the additional optional extensions (LZF is already listed in package.xml as optional)

[OPTIONAL] You may want to s/pear.horde.org/%{pear_channel}/ and then add "%global pear_channel pear.horde.org"

(In reply to comment #4)
> [OPTIONAL] I'm not sure what the exact Fedora packaging guidelines are for
> this, but I prefer to be very verbose and would list both the min and max
> requirements instead of just max (even though you know you don't have <
> 1.0.0 in Fedora):
> Requires: php-pear(pear.horde.org/PACKAGE) >= 1.0.0
> Requires: php-pear(pear.horde.org/PACKAGE) < 2.0.0

Per https://bugzilla.redhat.com/show_bug.cgi?id=785446#c6, ignore this

Updated

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache-2.0.1-1.fc17.src.rpm

Please remove APC from requires.  It is an optional require and end-users should not be forced to install APC if they are using one of the other opcode caches.

Updated

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Cache-2.0.1-2.fc17.src.rpm

Created attachment 666397 [details]
phpci.log

Created attachment 666398 [details]
php-horde-Horde-Cache-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 785450 --mock-config fedora-rawhide-x86_64

===== COULD items =====
* "s/pear.horde.org/%{pear_channel}/" and add "%global pear_channel pear.horde.org"
* Add a note in %description letting end-users know that they are responsible for installing whichever support opcode cache they would like.


No blockers.


===== APPROVED =====

New Package SCM Request
=======================
Package Name: php-horde-Horde-Cache
Short Description: This package provides a simple, functional caching API for Horde
Owners: nb
Branches: f17 f18 el6 
InitialCC: remi

Git done (by process-git-requests).