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 785480 (Horde_Log) - Review Request: php-horde-Horde-Log - Horde Logging library
Summary: Review Request: php-horde-Horde-Log - Horde Logging library
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: Horde_Log
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Shawn Iwinski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: horde-channel Horde_Constraint
Blocks: Horde_Controller Horde_Test Horde_SessionHandler Horde_SyncMl
TreeView+ depends on / blocked
 
Reported: 2012-01-29 03:33 UTC by Nick Bebout
Modified: 2013-03-26 13:04 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-12 03:01:01 UTC
Type: ---
Embargoed:
shawn: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)
php-horde-Horde-Log-review.txt (16.06 KB, text/plain)
2012-12-16 04:26 UTC, Shawn Iwinski 		no flags Details
phpci.log (10.38 KB, text/x-log)
2012-12-19 02:57 UTC, Shawn Iwinski 		no flags Details
phpci.log (16.35 KB, text/x-log)
2012-12-19 03:02 UTC, Shawn Iwinski 		no flags Details
php-horde-Horde-Log-review.txt (6.12 KB, text/plain)
2012-12-19 03:03 UTC, Shawn Iwinski 		no flags Details
Show Obsolete (2) View All

Description Nick Bebout 2012-01-29 03:33:05 UTC 
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-1.fc16.src.rpm
Description: Horde Logging package with configurable handlers, filters, and formatting.
Comment 1 Shawn Iwinski 2012-07-04 23:36:01 UTC 
I will review this package
Comment 2 Shawn Iwinski 2012-07-22 04:30:45 UTC 
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)
Comment 3 Nick Bebout 2012-08-02 20:36:21 UTC 
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
Comment 4 Nick Bebout 2012-09-18 23:26:05 UTC 
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
Comment 5 Shawn Iwinski 2012-12-10 03:26:33 UTC 
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?
Comment 6 Nick Bebout 2012-12-13 02:54:21 UTC 
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
Comment 7 Shawn Iwinski 2012-12-16 04:26:34 UTC 
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
Comment 8 Shawn Iwinski 2012-12-16 04:36:56 UTC 
(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
Comment 9 Shawn Iwinski 2012-12-17 19:45:56 UTC 
(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.
Comment 10 Nick Bebout 2012-12-19 00:16:20 UTC 
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
Comment 11 Shawn Iwinski 2012-12-19 02:57:36 UTC 
Created attachment 665846 [details]
phpci.log
Comment 12 Shawn Iwinski 2012-12-19 03:02:35 UTC 
Created attachment 665851 [details]
phpci.log
Comment 13 Shawn Iwinski 2012-12-19 03:03:30 UTC 
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
Comment 14 Shawn Iwinski 2012-12-19 03:08:19 UTC 
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 =====
Comment 15 Nick Bebout 2012-12-19 21:28:36 UTC 
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
Comment 16 Gwyn Ciesla 2012-12-20 11:49:27 UTC 
Git done (by process-git-requests).
Note You need to log in before you can comment on or make changes to this bug.