Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 785606 (Horde_Test)
Summary: | Review Request: php-horde-Horde-Test - Horde testing base classes | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nick Bebout <nb> | ||||
Component: | Package Review | Assignee: | Remi Collet <fedora> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | fedora, notting, package-review, shawn | ||||
Target Milestone: | --- | Flags: | fedora:
fedora-review+
gwync: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2012-12-12 22:49:32 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | 785424, 785439, 785455, 785480, 785493 | ||||||
Bug Blocks: | 874128, 874172, 874677, 874688, 894563, 908357, 908361, 908371 | ||||||
Attachments: |
|
Description
Nick Bebout
2012-01-29 22:16:36 UTC
I am not an official package reviewer, but here are some comments I have for this package: *** It would help readability to group your Build* and Requires* statements together instead of mixing them with each other and your Provides and Conflicts statements *** Your Provides statement should be Provides: php-pear(pear.horde.org/%{pear_name}) = %{version} instead of Provides: php-pear(%{pear_name}) = %{version} *** You should run phpci on your packages to make sure you have all requires: $ phpci print --recursive --report=extension src/Horde_Test-1.3.0 22 / 22 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++>] 100.00% BASE: /projects/fedora/REVIEWS/785606/src/Horde_Test-1.3.0 ------------------------------------------------------------------------------- PHP COMPAT INFO EXTENSION SUMMARY ------------------------------------------------------------------------------- EXTENSION PECL VERSION COUNT ------------------------------------------------------------------------------- Core 4.0.0 52 PDO 1.0.4dev 5.1.0 1 SPL 0.2 5.0.0 8 dom 20031129 5.0.0 4 json 1.2.1 5.2.0 1 pcre 4.0.0 4 standard 4.0.0 63 ------------------------------------------------------------------------------- A TOTAL OF 7 EXTENSION(S) WERE FOUND REQUIRED PHP 5.2.0 (MIN) ------------------------------------------------------------------------------- Time: 1 second, Memory: 9.25Mb ------------------------------------------------------------------------------- According to phpci, you need to require "php-pdo" and "php-dom" (virtual provide of php-xml) at a minimum. File Horde_Test-1.3.0/lib/Horde/Test/Functional.php uses the dom functions. File Horde_Test-1.3.0/lib/Horde/Test/Factory/Db.php uses the PDO functions. While package.xml lists these as optional, I believe this package would provide a better end-user experience if you added the two requires: Requires: php-pdo Requires: php-dom spl, json, and pcre are all virtual provides from php-common. For completeness (and to prevent any future packaging issues due to PHP package changes) my sponsor and I have agreed to require all virtual provides. It is up to you, but I would suggest adding the following as well: Requires: php-spl Requires: php-json Requires: php-pcre *** Do you plan on building for EPEL 5? - If not, please remove "rm -rf $RPM_BUILD_ROOT" from the %install section (see http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag). - If you do, there are several staements that need to be added. *** Up to you, but package.xml lists the following optional packages if you choose to require: - php-pear(pear.horde.org/Horde_Cli) - php-pear(pear.horde.org/Horde_Log) (if you do choose to require these packages, please update this ticket's "Depends On") I believe I have fixed these issues. Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-2.fc16.src.rpm (In reply to comment #2) Almost ready for official review. Please change Requires: php-pear(pear.horde.org/Horde_Cli) Requires: php-pear(pear.horde.org/Horde_Log) to Requires: php-pear(pear.horde.org/Horde_Cli) < 2.0.0 Requires: php-pear(pear.horde.org/Horde_Log) < 2.0.0 per package.xml requirements I believe I have fixed these issues. Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-3.fc16.src.rpm Created attachment 601950 [details]
php-horde-Horde-Test-review.txt
Generated by fedora-review 0.2.0 (53cc903) last change: 2012-07-09
Could: remove "localized" php.ini (no more useful) Could: requires php(language) >= 5.2.0 per new PHP Guildelines, but this is fedora specific (for now), so php-common seems acceptable as you target both fedora/epel MUST Package installs properly: Need to wait for Horde_Log and Horde_Constrant to be approved before import Lot of used class are not listed in requires As they are not listed by upstream, I think we can ommit them (optional) Ex, in /usr/share/pear/Horde/Test/Factory/Alarm.php if (!class_exists('Horde_Alarm')) { throw new Horde_Test_Exception('The "Horde_Alarm" class is unavailable!'); } return new Horde_Alarm_Null(); Package with tests which use the Horde_Test_Factory_Alarm will have to BR both Horde_Test and Horde_Alarm. After this package, I think others horde package, providing tests, will have to run them in the %check. No blocker ========= APPROVED ========= New Package SCM Request ======================= Package Name: php-horde-Horde-Test Short Description: Horde-specific PHPUnit base classes Owners: nb Branches: el6 f16 f17 InitialCC: Git done (by process-git-requests). |