Updated

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

I will review this package

*** [MUST] Please check the license.  I see LGPLv2 (from source) but I do not see LGPLv2+ (from spec), i.e. "LGPLv2 or later", mentioned anywhere.


*** [SHOULD] 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


*** [MUST] Your BuildRequires should be
BuildRequires:  php-pear(PEAR) >= 1.7.0
instead of
BuildRequires:  php-pear >= 1:1.4.9-1.2
(this would match what you have in bug 785606 and satisfy this package's package.xml dependency)


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


*** [SHOULD] phpci results: For completeness (and to prevent any future packaging issues due to PHP package changes) you may wish to require the virtual package "php-pcre".


*** [SHOULD] package.xml lists dependencies of Horde_Util and Horde_Exception >= 1.0.0 and < 2.0.0.  You already have requires for < 2.0.0.  I'm not sure if it's required, but I like to be verbose so I would suggest adding the >= 1.0.0 requires even though it appears all packages in Fedora satisfy this requirement already.


*** [SHOULD] Please consider adding requires for the optional dependencies listed in package.xml:
Horde_Db >= 1.0.0 < 2.0.0
Horde_Ldap >= 1.0.0 < 2.0.0
(if you do added requires for these optional dependencies, please update this ticket's "Depends On")


*** [SHOULD] Please consider adding a note for optional dependencies "Horde_Test", "Horde_Log" and "php-pdo" (perhaps in %description for end users) and note what they are needed for (it appears all those are requirements of the tests?).


*** [MUST]  Your provides should be

Provides:	php-pear(pear.horde.org/%{pear_name}) = %{version}

instead of

Provides:	php-pear(%{pear_name}) = %{version}


*** [SHOULD; see https://bugzilla.redhat.com/show_bug.cgi?id=817303#c5]
About
   [ -f package2.xml ] || mv package.xml package2.xml
   mv package2.xml %{pear_name}-%{version}/%{name}.xml

This is a old hack (yes it works) to ensure than package v2 is used.

I often use

   # package.xml is version 2.0
   mv package.xml %{pear_name}-%{version}/%{name}.xml

This is not an issue, but makes the spec simpler to read.


*** [MUST] In %files, use %{pear_name} where you can.


*** [SHOULD] It may be beneficial to use "%global pear_channel pear.horde.org" and then use "%{pear_channel}" where you can.

Please update to 2.0.1

Updated

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

Created attachment 696065 [details]
phpci.log

Created attachment 696066 [details]
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 -m review -b 785465

[!]: License field in the package spec file matches the actual license.
	LGPLv2 (not LGPLv2+)

[!]: Package installs properly.
     Note: Installation errors (see attachment)
	Wait for dependency before import (Horde_Db)

[!]: Requires correct, justified where necessary.
	From package.xml
	Requires:       php-pear(PEAR) >= 1.7.0

	And for consistency
	BuildRequires:  php-common >= 5.3.0
	BuildRequires:  php-pear(PEAR) >= 1.7.0

Optional requires to consider
	Horde_Ldap (I think we should add this one)
	pgp-pdo, php-mysql, php-mysqli (will be pulled by Horde_Db)
	php-ldap (will be pulled by Horde_Ldap)

Updated

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

--- php-horde-Horde-Group.spec.0	2013-02-11 13:21:24.000000000 +0100
+++ php-horde-Horde-Group.spec	2013-03-20 21:26:09.000000000 +0100
@@ -4,17 +4,18 @@
 %global pear_channel pear.horde.org
 
 Name:           php-horde-Horde-Group
-Version:        2.0.1
-Release:        2%{?dist}
+Version:        2.0.2
+Release:        1%{?dist}
 Summary:        Horde User Groups System
 
 Group:          Development/Libraries
-License:        LGPLv2+
+License:        LGPLv2
 URL:            http://pear.horde.org
 Source0:        http://%{pear_channel}/get/%{pear_name}-%{version}.tgz
 
 BuildArch:      noarch
-BuildRequires:  php-pear
+BuildRequires:  php-pear(PEAR) >= 1.7.0
+BuildRequires:  php-common >= 5.3.0
 BuildRequires:  php-channel(%{pear_channel})
 # To run unit tests
 BuildRequires:  php-pear(%{pear_channel}/Horde_Test) >= 2.1.0
@@ -23,7 +24,8 @@
 Requires(post): %{__pear}
 Requires(postun): %{__pear}
 Requires:       php-common >= 5.3.0
-Requires:       php-pcre
+Requires:       php-pear(PEAR) >= 1.7.0
+Requires:       php-pcre php-pdo php-ldap
 Requires:       php-channel(%{pear_channel})
 Requires:       php-pear(%{pear_channel}/Horde_Exception) >= 2.0.0
 Conflicts:      php-pear(%{pear_channel}/Horde_Exception) >= 3.0.0
@@ -32,6 +34,8 @@
 # Optionnal
 Requires:       php-pear(%{pear_channel}/Horde_Db) >= 2.0.0
 Conflicts:      php-pear(%{pear_channel}/Horde_Db) >= 3.0.0
+Requires:       php-pear(%{pear_channel}/Horde_Ldap) >= 2.0.0
+Conflicts:      php-pear(%{pear_channel}/Horde_Ldap) >= 3.0.0
 
 Provides:       php-pear(%{pear_channel}/%{pear_name}) = %{version}
 
@@ -94,16 +98,19 @@
 
 
 %changelog
+* Wed Mar 20 2013 Nick Bebout <nb> - 2.0.2-1
+- Update for review
+
 * Tue Feb 5 2013 Nick Bebout <nb> - 2.0.1-2
 - Remove BuildRoot, Change php(language) to php-common
 
-* Mon Nov 19 2012 Remi Collet <RPMS> - 2.0.1-1
+* Mon Nov 19 2012 Remi Collet <remi> - 2.0.1-1
 - Update to 2.0.1 for remi repo
 
-* Fri Nov  2 2012 Remi Collet <RPMS> - 2.0.0-2
+* Fri Nov  2 2012 Remi Collet <remi> - 2.0.0-2
 - fix provides (missing channel)
 
-* Fri Nov  2 2012 Remi Collet <RPMS> - 2.0.0-1
+* Fri Nov  2 2012 Remi Collet <remi> - 2.0.0-1
 - Update to 2.0.0 for remi repo
 
 * Thu Jun 21 2012 Nick Bebout <nb> - 1.0.5-1


All issues fixed
No blocker

=== APPROVED ===

New Package SCM Request
=======================
Package Name: php-horde-Horde-Group
Short Description: Package for managing and accessing the Horde groups system
Owners: nb remi
Branches: el6 f18 f19
InitialCC:

Git done (by process-git-requests).

php-horde-Horde-Db-2.0.2-1.fc18, php-horde-Horde-Group-2.0.2-1.fc18, php-horde-Horde-LoginTasks-2.0.2-4.fc18, php-horde-Horde-Stream-Filter-2.0.1-4.fc18, php-horde-Horde-Token-2.0.3-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/php-horde-Horde-Db-2.0.2-1.fc18,php-horde-Horde-Group-2.0.2-1.fc18,php-horde-Horde-LoginTasks-2.0.2-4.fc18,php-horde-Horde-Stream-Filter-2.0.1-4.fc18,php-horde-Horde-Token-2.0.3-3.fc18

php-horde-Horde-Db-2.0.2-1.fc18, php-horde-Horde-Group-2.0.2-1.fc18, php-horde-Horde-LoginTasks-2.0.2-4.fc18, php-horde-Horde-Stream-Filter-2.0.1-4.fc18, php-horde-Horde-Token-2.0.3-3.fc18 has been pushed to the Fedora 18 testing repository.

php-horde-Horde-Db-2.0.2-1.fc18, php-horde-Horde-Group-2.0.2-1.fc18, php-horde-Horde-LoginTasks-2.0.2-4.fc18, php-horde-Horde-Stream-Filter-2.0.1-4.fc18, php-horde-Horde-Token-2.0.3-3.fc18 has been pushed to the Fedora 18 stable repository.