Hide Forgot
Spec URL: http://dl.atrpms.net/all/php-pear-Auth-OpenID.spec SRPM URL: http://dl.atrpms.net/all/php-pear-Auth-OpenID-1.2.1-1.at.src.rpm Description: An implementation of the OpenID single sign-on authentication protocol.
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).