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-1.fc16.src.rpm Description: A programmatic way of building constraints that evaluate to true or false.
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).