I'm quite disappointed by this package...

BuildRequires should be php-pear >= 1:1.4.9-1.2 (older version doesn't provide
the used macros)

PHPUnit version 1.x is obsolete and replace in devel by PHPUnit 3 (which is not
compatible).

Empty build section should be provided (at least to avoid rpmlint warning, i
know there is a bug if there is no %build, even if it only affect arch RPM).

This package is not in default channel (pear.php.net) but in __uri one.
Channels was discussed by packaging commitee but nothing approved.

Uninstall command is :
pear uninstall --nodeps --ignore-errors --register-only %{channel}/%{pear_name}
With %{channel} defined to __uri

And provides should probably be : php-pear(%{channel}/%{pear_name}) = %{version}

Why are you moving %{pear_docdir}/%{pear_name}/doc to docdir and not all
%{pear_docdir}/%{pear_name}/* ?

And version 1.2.2 is available...

(In reply to comment #1)
> I'm quite disappointed by this package...

I'm very sorry about that, but I followed the packaging guidelines which provide
explicit BuildRequires/Requires for pear packages.

http://fedoraproject.org/wiki/Packaging/PHP?action=recall&rev=2

If you consider the guidelines wrong, then please raise that as a general issue
on the packaging list, ranting on a specific package won't help.

> Channels was discussed by packaging commitee but nothing approved.

If nothing was approved/rejected, this means that the current guidelines still
apply, and you should try to get the FPC to modify them if need be.

> Empty build section should be provided (at least to avoid rpmlint warning, i
> know there is a bug if there is no %build, even if it only affect arch RPM).

Other than rpmlint not liking it there is no "bug".

> PHPUnit version 1.x is obsolete and replace in devel by PHPUnit 3 (which is
> not compatible)

That's bad. So we need a PHPUnit 1.x compat :/

> And version 1.2.2 is available...

Not when I submitted the package ;)

Does 1.2.2 work with PHPUnit 3?

The guidelines need updating, thanks Axel for pointing this out.  We definately
need to get channel stuff in the guidelines, I did not think it important before
because I didn't think we'd have any packages that used other channels.  It
appears I was wrong.

I think any attempt at review on this package should probably wait until the
pear version # and channel guidelines are in approved by the packaging comittee.

In the meantime, Axel, can you please investigate the possibilities of modifying
this to work with PHPUnit3?  It may be that we will need a PHPUnit1-compat rpm
or something, so we should see how difficult it is to either package a compat
rpm or modify this rpm to work with PHPUnit3.

All the other stuff seems like politics to me, I would recommend that you just
use the php-pear template.  It appears you did because of some comments I see in
the spec, but I wouldn't bother removing stuff like %build or changing
%buildroot.  Thats just nonsense stuff and I'd rather focus on the more
important issues.

Thanks.

Axel, now that the pear guidelines have been finalized, I'd be glad to finally
get this review over for you.  Make your spec look like:

http://fedoraproject.org/wiki/SIGs/PHP/PearSpecTemplate

and post some new files.  Any remarks about PHPUnit (1-vs-3)?

Thanks, setting to needinfo on me to bubble up in the frontpage.

This has been needinfo for 5.5 months now; any chance something could happen
here?  There's a push for more openID in Fedora, so it would be nice to get this in.

And it's been another month.  I will close this ticket soon if there is no response.

Brandon, is it possible to have two different versions of a pear package in the
system? The latest version is 2.0.1, but mediawiki-openid still requires 1.2.x.

I'm not sure, but I doubt it.  Most pear packages that I know of install as
/usr/share/pear/<Classname>.php so that they will automatically be picked up
when you require 'Classname.php'.  Can mediawiki be easily patched to use 2.x?

Upstream notes that

"Note: the 2.x version of the library will break this extension. A future
version of the extension will work with the 2.x version correctly." I contacted
upstream directly to see what the status is.

Currently I tend to packaging 2.x in the hopes that the mediawiki openid
extension will catch up soon. But let's also wait for upstream's feedback.

The upstream author asks for helping in testing the current cvs trunk of
mediawiki-openid. It is using version 2.x of this library.

So I'll upgrade to 2.x and once we have the package in rawhide and F-X we can
help  mediawiki to test its openid extension against v 2.x (of course if anyone
wants to test before the bits are packaged he/she should do so!).

http://dl.atrpms.net/all/php-pear-Auth-OpenID.spec
http://dl.atrpms.net/all/php-pear-Auth-OpenID-2.0.1-4.src.rpm

* Sun Feb 24 2008 Axel Thimm <Axel.Thimm@ATrpms.net> - 2.0.1-4
- Update to 2.0.1.
- Change license.
- PEAR install method has regressed, some manual fixes are necessary.
- No testing done (needs too old PHPUnit).

Brandon, did you find any time to look into this review? Thanks!

If Brandon won't find any free time to review this package, then I'll pick it up. 

Looks like Brandon is too busy to review this request so I reassign it to myself.

ОK, some remarks first.

* According to PearSpecTemplate buildroot must be

BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

In any case this is cosmetic issue and may be omitted. I wonder why we still need to explicitly define 
BuildRoot?

* According to PearSpecTemplate Requires(post) and Requires(postun) must be

Requires(post): %{__pear}
Requires(postun): %{__pear}

I personally don't know whether this is an issue at all or just a cosmetic

* to make rpmlint happy we need some CRLF-conversions, so we must add BuildRequires: dos2unix and 
add to %prep

dos2unix doc/media/*.css

in %files section we must add directories it owns and not only %{pear_phpdir}/* but %{pear_phpdir}/data/Auth_OpenID

Ping!

Sorry Peter, I missed comment 16!

-%(%{__id_u} -n): This is not considered the better buildroot, in fact we had
the wording of the guidelines changed to provide better alternatives last year.

%{__tool} vs tool is a matter of style used differently by every packager out
there. Personally I think this is overmacroizing things, as either the rpmbuild
setup is pointing to sane PATHs or you're in deep trouble anyway due to
non-absolute calls to binaries in Makefiles etc.

I'll add the dos2unix fixes, thanks for spotting!

I didn't understand the issue with %{pear_phpdir}/* vs %{pear_phpdir}/data. The
globbing does include all, or not?

*** Bug 439285 has been marked as a duplicate of this bug. ***

> -%(%{__id_u} -n): This is not considered the better buildroot, in fact we had
the wording of the guidelines changed to provide better alternatives last year.

I'm just following the current (at that moment guidelines). It's not a blocker,
I think.

> %{__tool} vs tool is a matter of style used differently by every packager out
there. 

Likewise. I'm just following guidelines (I'm not a keen in php/pear so I'm not
sure what is important and what is not).

> I'll add the dos2unix fixes, thanks for spotting!

OK.

> I didn't understand the issue with %{pear_phpdir}/* vs %{pear_phpdir}/data.
The globbing does include all, or not?

My wish was to avoid multiple ownership of directories. But looks like this kind
of inclusion is a common practice for php-pear-Auth-* packages.

Anywy, I'm waiting for your next release.

Aah! I seem to have missed this review. But it seems to be long pending (since 1
year+). May you mark #454580 duplicate of this request as I am not sponsored yet
and wouldn't be able to change status. And close that request.
     Between is php-openid a pear package?

It would be great if this package makes it ASAP. Thanks.

*** Bug 454580 has been marked as a duplicate of this bug. ***

I am not sponsored yet, so these are just suggestios which may help:

1. using sed in place of dos2unix and removes Buildrequire: dos2unix
2. Everything(except doc) gets installed in data folder %{pear_datadir} => /usr/
share/pear/data/ which I believe is wrong. You should use %{pear__phpdir}
3. While building on my machine I get these warnings:
 (i) WARNING: configuration download directory "/tmp/pear/download" is not 
writeable.  Change download_dir config variable to a writeable dir to avoid 
this warning
     
         This doesn't seem to be an issue, but may be you would like to 
investigate. Or may be its only with me.

 (ii) warning: File listed twice: /usr/share/pear/data/Auth_OpenID/ was coming 
earlier but when I replaced with %{pear_phpdir} same warnings with modified 
file path.
          This also seems to be cosmetic issue. I guess it comes because source 
has not been pear packaged cleanly as %install requires to manually copy files 
in BUILD. May be you can have a look.

My suggestions are based on these changes I made:
http://rakesh.fedorapeople.org/patch/openid_spec.patch

Axel, Did you find some time to update this ?

Thanks!

Spec URL: http://dl.atrpms.net/all/php-pear-Auth-OpenID.spec
SRPM URL: http://dl.atrpms.net/all/php-pear-Auth-OpenID-2.1.1-5.src.rpm

* Wed Jul 30 2008 Axel Thimm <xxx> - 2.1.1-5
- Upgrade to 2.1.1.
- Use php_dir instead of data_dir (Rakesh Pandit <xxx>)
- Fix CRLF (Peter Lemenkov <xxx> & R. Pandit)

This should take care of all comments so far I hope.

REVIEW:

MUST Items:

+ rpmlint is silent
+ The package named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing
Guidelines.
+ The License field in the package spec file matches the actual license.
+ The text of the license(s) included in %doc.
+ The spec file written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package match the upstream source.

[petro@Sulaco SOURCES]$ md5sum php-openid-2.1.1.tar.bz2*
2fd3e4284f106f8a77fd5ba9c83b30f9  php-openid-2.1.1.tar.bz2
2fd3e4284f106f8a77fd5ba9c83b30f9  php-openid-2.1.1.tar.bz2.1
[petro@Sulaco SOURCES]$

+ The package successfully compiles and builds into binary rpms on at least one
supported architecture (tested on ppc).
+ All build dependencies are listed in BuildRequires.
+ A package owns all directories that it creates.
+ A package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ Each package has a %clean section.
+ Each package consistently uses macros.
+ The package contains code, or permissable content.
+ Packages does not own files or directories already owned by other packages. 
+ At the beginning of %install, each package runs rm -rf %{buildroot} 
+ All filenames in rpm packages are valid UTF-8.

However I still have some suggestions.

* docs should be packaged more consistently:

http://peter.fedorapeople.org/php-pear-Auth-OpenID_docs.diff

There are some valuable suggestions taken from Ian's spec:

http://peter.fedorapeople.org/php-openid.spec

* The package should provide php-yadis (seems that it should "Provides:
php-pear(Auth_Yadis) = 1.0.2" or something similar)
* Should it require php-pear-DB instead of php-pgsql and php-mysql (and
commented out php-sqlite)? I'm not sure, though.

(In reply to comment #26)
> REVIEW:

Thanks!

> However I still have some suggestions.
> 
> * docs should be packaged more consistently:
> 
> http://peter.fedorapeople.org/php-pear-Auth-OpenID_docs.diff

OK, but let me keep it commented as when the package starts installing docs
again it would need to be special handled again.

> There are some valuable suggestions taken from Ian's spec:
> 
> http://peter.fedorapeople.org/php-openid.spec
> 
> * The package should provide php-yadis (seems that it should "Provides:
> php-pear(Auth_Yadis) = 1.0.2" or something similar)

There are extra Yadis packages out there (in fact I submitted one last year). I
would wait until someone needs that dependency. OTOH I actually don't have that
strong an opinion on that, if it turns out to be broken we could remove it later
just as good.

> * Should it require php-pear-DB instead of php-pgsql and php-mysql (and
> commented out php-sqlite)? I'm not sure, though.

The dependencies are from upstream and I even had Requires: php-pear-DB >= 1.80
in the previous specfile, but a user rightfully reported that this version in
the upstream dependency is completely broken. That's why I removed this
dependency altogether for now.

I suggest:

a) adopting your doc changed but leaving a comment block for future packages
   that may revide the doc install target
b) making adding a vitual Provides: for Yadis your call - e.g. if you still
   want it, I'll add it.
c) Letting php-pear-DB off as a dependency for now.

OK, finally

APPROVED.

Thanks!

New Package CVS Request
=======================
Package Name: php-pear-Auth-OpenID
Short Description: PHP OpenID
Owners: athimm
Branches: F-8 F-9
InitialCC: 
Cvsextras Commits: no

cvs done.

Package Change Request
======================
Package Name: php-pear-Auth-OpenID
New Branches: el6
Owners: puiterwijk
InitialCC: 

athimm is on the list of people who do not want to maintain EPEL packages.

Git done (by process-git-requests).