Bug 785479 (Horde_Constraint)

I will review this package

After a quick review:

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

[MUST] "Requires: php-pear(PEAR) >= 1.7.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)

I can't use the tests yet, Constraint and Log have to be in the repo before I can build Test

Other issues have been fixed.

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Constraint.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Constraint-1.0.1-2.fc17.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?

Updated

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

(In reply to comment #5)

You have a note about commenting out Horde_Test but did not comment it out:

# Horde_Test is not yet available and is optional dependency
# commenting it out for now
Requires:       php-pear(pear.horde.org/Horde_Test) >= 2.1.0

Updated.  I didn't bump the Release: since it was such a minor change.

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

Created attachment 664207 [details]
php-horde-Horde-Constraint-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 785479

(In reply to comment #8)


===== MUST items =====

[!]: Package requires other packages for directories it uses.
[!]: Package must own all directories that it creates.

Since Horde_Test not required yet, "%dir %{pear_phpdir}/Horde" must be used in %files.


===== 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
* In %files, "%{pear_testdir}/Horde_Constraint" could be changed to "%{pear_testdir}/%{pear_name}"

(In reply to comment #9)

> ===== COULD items =====
> 
> * Localized php.ini not necessary
> * In %files, "%{pear_testdir}/Horde_Constraint" could be changed to
> "%{pear_testdir}/%{pear_name}"

* Require (and build require) virtual package "php-pcre"

(In reply to comment #9)
> ===== 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-Constraint.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Constraint-2.0.1-2.fc17.src.rpm

Created attachment 665844 [details]
phpci.log

Created attachment 665845 [details]
php-horde-Horde-Constraint-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 785479 --mock-config fedora-rawhide-x86_64

After initial import, please also remove "PHPRC=../php.ini" from %install.

No blockers.


===== APPROVED =====

New Package SCM Request
=======================
Package Name: php-horde-Horde-Constraint
Short Description: A programmatic way of building constraints that evaluate to true or false
Owners: nb
Branches: f17 f18 el6 
InitialCC: remi

Git done (by process-git-requests).