Bug 785447 (Horde_Auth)

I believe this one is ready to review now.

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

I will review this package

I believe all of the normal blockers for the php-horde-Horde-* packages are fixed with this package.

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

[SHOULD] lib/Horde/Auth/Sql.php includes "$now = new Horde_Date(time());".  Please verify Horde_date is not a dependency.

[SHOULD] %check is present and all tests pass



[OPTIONAL] For readability, you may want to group your Build* and Requires*

[OPTIONAL] I'm not sure what the exact Fedora packaging guidelines are for this, but I prefer to be very verbose and would list both the min and max requirements instead of just max (even though you know you don't have < 1.0.0 in Fedora):
Requires: php-pear(pear.horde.org/PACKAGE) >= 1.0.0
Requires: php-pear(pear.horde.org/PACKAGE) < 2.0.0

[OPTIONAL] You may want to consider adding requires for the following optional dependencies (from package.xml):
* php-pear(pear.horde.org/Horde_Db)
* php-pear(pear.horde.org/Horde_History)
* php-pear(pear.horde.org/Horde_Lock)
* php-pear(pear.horde.org/Horde_Imap_Client)
* php-pear(pear.horde.org/Horde_Kolab_Session)
* php-pear(pear.horde.org/Horde_Ldap)
* php-pear(pear.horde.org/Horde_Imsp)
* php-pear(pear.horde.org/Horde_Http)

[OPTIONAL] phpci: You may want to consider adding the following requires:
* php-ctype
* php-date
* php-ftp
* php-hash
* php-ldap
* php-pcre
* php-pdo
* php-spl
--- NOTE: Some of these may be from the optional horde packages mentioned in the previous [OPTIONAL] so you may not need to require them if you do not require those
--- NOTE: package.xml specifies "ctype" and "ftp" extensions as optional... you may want to have upstream add the additional optional extensions if they are not already required by the mentioned optional packages (I'm assuming ldap would be required by Horde_Ldap, pdo would be required by Horde_Db, etc).

[OPTIONAL] package.xml lists PECL "pam" and "sasl" as optional.  These are not packaged in Fedora (at least I couldn't find them).  Looking at the PECL site, I'm not sure if those extensions are actively maintained or not though.  If they are, you may want to package them and eventually add them as dependencies.

[OPTIONAL] You may want to s/pear.horde.org/%{pear_channel}/ and then add "%global pear_channel pear.horde.org"

(In reply to comment #5)
> [OPTIONAL] I'm not sure what the exact Fedora packaging guidelines are for
> this, but I prefer to be very verbose and would list both the min and max
> requirements instead of just max (even though you know you don't have <
> 1.0.0 in Fedora):
> Requires: php-pear(pear.horde.org/PACKAGE) >= 1.0.0
> Requires: php-pear(pear.horde.org/PACKAGE) < 2.0.0

Per https://bugzilla.redhat.com/show_bug.cgi?id=785446#c6, ignore this

Updated

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

Updated

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

quick notes:
- missing %check
- missing %{php_metadir}
- must own %{pear_datadir}/Horde_Auth/migration
- should use %{pear_name} (and perhaps %{channel_name})

I don't think we need to have Horde_Test in the optional dep (even if available) as this pull a lot of (probably unwanted) dependencies.

Updated

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

Shawn, do you plan to review this one ? (the next according to my build order)

(In reply to comment #12)
> Shawn, do you plan to review this one ? (the next according to my build
> order)

I just tried to, but pear.horde.org is unreachable :(  I'll release my review in case you can get to it before I do.

Nick --

Without being able to download the upstream source and do an official review, here are some things I see that need to be addressed:

* BuildRequires:
      php-pear   ===>   php-pear(PEAR) >= 1.7.0
* BuildRequires:  php-common >= 5.3.0
* Requires:       php-pear(PEAR) >= 1.7.0

Also, I'm guessing that the "LGPLv2+" license listed needs to be changed to "LGPLv2" like a bunch of the other Horde packages.

Updated

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

Created attachment 716958 [details]
phpci.log

phpci 2.14.0

Created attachment 716960 [details]
review.txt

Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -b 785447

[!]: Each %files section contains %defattr if rpm < 4.4

[!]: Requires correct, justified where necessary.

	Missing from phpci report
	Requires:       php-ctype
	Requires:       php-date
	Requires:       php-ftp
	Requires:       php-hash
	Requires:       php-ldap
	Requires:       php-pcre
	Requires:       php-pdo
	Requires:       php-spl

There is a lot of optional dependencies from package.xml.
Need to evaluate which ones can be added without breaking the build order.
(at least Horde_Db, Horde_Lock)

Updated

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

Actually

Updated

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

-%defattr(-,root,root,-)

[x]: Each %files section contains %defattr if rpm < 4.4

+Requires:       php-ctype php-date php-ftp php-hash php-ldap
+Requires:       php-pcre php-pdo php-spl

+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_Lock) >= 2.0.0
+Conflicts:      php-pear(%{pear_channel}/Horde_Lock) >= 3.0.0
+Requires:       php-pear(%{pear_channel}/Horde_Http) >= 2.0.0
+Conflicts:      php-pear(%{pear_channel}/Horde_Http) >= 3.0.0
+Requires:       php-pear(%{pear_channel}/Horde_Ldap) >= 2.0.0
+Conflicts:      php-pear(%{pear_channel}/Horde_Ldap) >= 3.0.0

[x]: Requires correct, justified where necessary.


No Blocker, all issues fixed.

=== APPROVED ===

New Package SCM Request
=======================
Package Name: php-horde-Horde-Auth
Short Description: Horde Authentication API
Owners: nb remi
Branches: el6 f18 f19
InitialCC:

Git done (by process-git-requests).

php-horde-Horde-Auth-2.0.4-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/php-horde-Horde-Auth-2.0.4-2.fc18

php-horde-Horde-Auth-2.0.4-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-horde-Horde-Auth-2.0.4-2.el6

php-horde-Horde-Auth-2.0.4-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

php-horde-Horde-Auth-2.0.4-2.fc18 has been pushed to the Fedora 18 stable repository.