Bug 438804 (php-pear-Auth) - Review Request: php-pear-Auth - provides methods for creating an authentication system using PHP
Summary: Review Request: php-pear-Auth - provides methods for creating an authenticati...
Keywords:
Status: CLOSED DUPLICATE of bug 460660
Alias: php-pear-Auth
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Christopher Stone
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: pear-HTTP-Client pear-Auth-RADIUS
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-03-25 12:45 UTC by David Hollis
Modified: 2009-05-22 16:59 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-29 14:48:41 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description David Hollis 2008-03-25 12:45:33 UTC
Spec URL: http://web.davehollis.com:81/packages/php-pear-Auth.spec
SRPM URL: http://web.davehollis.com:81/packages/php-pear-Auth-1.5.4-1.fc8.src.rpm
Description: 
The PEAR::Auth package provides methods for creating an authentication
system using PHP.

Currently it supports the following storage containers to read/write
the login data:

* All databases supported by the PEAR database layer
* All databases supported by the MDB database layer
* All databases supported by the MDB2 database layer
* Plaintext files
* LDAP servers
* POP3 servers
* IMAP servers
* vpopmail accounts
* RADIUS
* SAMBA password files
* SOAP (Using either PEAR SOAP package or PHP5 SOAP extension)
* PEAR website
* Kerberos V servers
* SAP servers

Comment 1 David Hollis 2008-03-25 12:46:53 UTC
This is my first Fedora RPM and I do not currently have a sponsor.

Comment 2 Christopher Stone 2008-04-06 18:24:58 UTC
I was going to package this myself[1], there is some discussion on splitting
this up into optional requirements[2].

What would need to be packaged in order to satisfy all optional components of
this package?

[1]
http://fedoraproject.org/wiki/ChristopherStone#head-d7744c4c8ee8b851590aa038b984494254d0fda4
[2]
http://fedoraproject.org/wiki/PackagingDrafts/PHP#head-d17f63e05f4104fca50f0862125e4cf8490591bf


Comment 3 David Hollis 2008-04-07 12:26:23 UTC
I think the sub-packages would just include the specific container files and
appropriate Requires: lines...  For example, a php-pear-Auth-LDAP might have
these specific bits:

Requires: php-ldap

%files
/usr/share/pear/Auth/Container/LDAP.php

(appropriate macros used of course)

I'm not certain if it's terribly necessary to split up however, though it would
likely be the only way to be able to ensure that a particular component would
have the right bits installed without the main package requiring everything
under the sun...


Comment 4 Christopher Stone 2008-04-07 16:51:34 UTC
Dave, can you access the #fedora-devel channel on the freenode IRC network? I
prefer this form of communication for those I sponsor.

http://fedoraproject.org/wiki/Communicate#IRC

Comment 5 Christopher Stone 2008-05-05 17:21:59 UTC
ping?

Comment 6 David Hollis 2008-05-08 17:10:03 UTC
Does splitting this package up add any benefit?  Some of the containers may not
be shippable otherwise since they require external plugins that may or not may
not be able to ship with Fedora (SAP, vpopmail, future additions).  Is the goal
for splitting out containers to ensure that the appropriate PHP extensions are
required so that the end user can expect it to Just Work (like php-ldap for the
LDAP container)?

Comment 7 Christopher Stone 2008-05-10 21:28:07 UTC
I wanted to go ahead and add requirements for all functionality of Auth so that
it just works.  For example, does Auth even work if there is not a DB package
installed?  Some requirements like Log we should add too because it adds a lot
of extra functionality without bringing in too many extra packages.  If we added
Auth_Radius however, this might bring in too many extra packages and not benefit
most users.  So my thought was to split out the radius component into a separate
package, which would require Auth_Radius which requires radius.  I'm not sure if
this is the best approach however, it might just be easier to assume someone
using radius and Auth will also have Auth_Radius installed.

Again, please let me know if you can communicate via IRC as this will make
discussion *much* easier (see comment #4).  Thanks.

Comment 8 David Hollis 2008-05-12 11:37:36 UTC
I can be on IRC, though not at all times and I can't pay close attention to it.
 I'm slacker775 on freenode.

I suppose for Auth plugins that have all necessary support within Fedora for
their add-ons, splitting them out may be helpful, and simple enough.  For ones
that would require external add-ons like SAP, vpopmail, the easiest thing would
be to just include them in the main package and let the end-user add the
necessary dependencies as required.  If we were to not package them, then our
package becomes useless, or they have to rig up a hacked Auth package
(php-pear-Auth-SAP) that only includes the SAP file(s) which starts to become
stupid.

Comment 9 Jason Tibbitts 2008-05-30 22:56:10 UTC
Anything happening here?  Chris, were you planning to sponsor David? 
Unfortunately some other tickets  weren't properly blocking the NEEDSPONSOR
ticket so I reviewed one of them and now it's in limbo.

Comment 10 Christopher Stone 2008-05-31 17:40:50 UTC
I was planning on sponsoring David, but I'm not getting any results.  I've asked
David to redo the spec file with additional requirements, but I've not seen
anything yet.

I suppose we can give it one more week, then just close out these bugs.

Dave??

Comment 11 David Hollis 2008-05-31 18:20:35 UTC
My only real concern with breaking out into subpackages is the additional
dependencies that will be created that aren't currently in Fedora, some of which
may never be.  For example, the Kerberos, SAP, vpopmail and MDB (not MDB2)
containers.  The required plugins for these may FOSS, maybe not.  In any case, I
don't have the time nor personal need to package those pieces adequately for
inclusion and if that holds up the package from being included for others, that
doesn't seem like a great trade-off.  The pieces that can be sub-packaged so as
to satisfy their requirements can be handled easily, though those sub-pkgs will
only have 1-2 files each but those containers that can't be resolved are the
real issue for me.  I can simply include them in the main package and it's left
as an exercise for the user to fill in the missing pieces as required, or they
can be neutered out of the package entirely and require end-users to make their
own sub-packages or to pull in the missing files to satisfy their needs.  Those
two options seem rather silly to me.

Whats the preference? 

Comment 12 Christopher Stone 2008-05-31 23:09:50 UTC
Dave I think you have not been following the discussion very closely.  Nobody is
asking you to package Kerberos, SAP, etc.  Ofcouse this would be *impossible*. 
What we *do* need to do is add requirements for those packages which *are* in
Fedora.  Also, please note that we do not want separate packages for each
requirement, please re-read comment #7 more carefully.

Comment 13 David Hollis 2008-06-02 14:17:48 UTC
Updated package and spec available at:
http://web.davehollis.com:81/packages/php-pear-Auth.spec
http://web.davehollis.com:81/packages/php-pear-Auth-1.6.1-1.fc9.noarch.rpm

It now Requires: all optional elements that enhance functionality though this
does mean that it pulls in a bunch of other packages to install and I can
imagine some folks getting a little peeved at that.  If it is a large enough
issue, maybe a simple way to keep it from requiring 100 packages and to prevent
100 sub-packages, have php-pear-Auth include the most typical functionality -
LDAP, MDB2 and maybe another one or two and then creating a sub-package that
includes the somewhat more obscure containers and has more of the dependencies.

Comment 14 Christopher Stone 2008-06-02 15:24:25 UTC
Do you know which Requires brings in the most packages?  Also, does it make
sense to require both DB and MDB2?  I will take a closer look at this over the
next day or two.  Thanks.

Comment 15 David Hollis 2008-06-02 17:19:21 UTC
It looks like Crypt-CHAP may be responsible for a decent number of the
requirements.  That could get split out into the subpackage.  As for DB vs MDB2,
that would really be more of an issue as to what the developer is currently
using for their DB.  With DB being marked deprecated, that could be thrown into
the subpackage as well with no problem.

Comment 16 Christopher Stone 2008-06-02 20:08:37 UTC
Okay, it is probably best to require both DB and MDB2 since these packages don't
bring in too many extras, and we want this to work out of the box for as many
people as possible.

Please update the spec to make a -samba subpackage.  Also, change the Requires
lines to look like:
Requires: php-pear(File_Passwd) >= 1.1.0
Instead of:
Requires: php-pear-File-Passwd >= 1.1.0

Is dos2unix brought in automatically?  Please modify the spec file so that you
accomplish the same results using only the tools listed here:
https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions
Modifications of this type are better suited in the %prep section of the spec
file.  Please try to accomplish this there, or if not possible explain why.

Shorten the %description section.  The first paragraph is ample, and you don't
want to keep updating it every time a new feature is added.



Comment 17 David Hollis 2008-06-06 11:38:33 UTC
Updated package and spec now available:

http://web.davehollis.com:81/packages/php-pear-Auth.spec
http://web.davehollis.com:81/packages/php-pear-Auth-1.6.1-1.fc9.noarch.rpm

A little bit of refactoring was needed to get the documentation and examples
included properly.  I'm not certain if that was a 1.5 -> 1.6 thing or if it
actually wasn't quite right to begin with.  Using the info from the wiki, I'm
able to do the dos2unix thing on the doc file with a bit of sed magic and
rpmlint has no issues.

Comment 18 Christopher Stone 2008-06-06 14:37:26 UTC
This is looking better.  It should be a -2 release however, and you should
document all your changes in the changelog section of the spec file.

Please also do not forget to add a -samba subpackage which requires the
SMBpasswd module (you can remove crypt-chap from the requires since this is
redundant).

Comment 19 Christopher Stone 2008-06-06 14:58:28 UTC
Dave, please also investigate making a -radius subpackage as well which Requires
Auth_Radius.

Comment 20 Christopher Stone 2008-06-07 15:51:23 UTC
David, please subscribe to the fedora php mailing list[1] if you havn't already
done so and introduce yourself.  We need to have a quick discussion on the
mailing list regarding how to handle Provides for pear packages which have
subpackages like Auth.

I have also whipped up a quick HTTP_Client package which we can add to the
Requires for Auth once it's approved.

[1] https://www.redhat.com/mailman/listinfo/fedora-php-devel-list

Comment 21 Christopher Stone 2008-06-14 16:14:12 UTC
ping?

Comment 22 David Hollis 2008-06-17 17:29:01 UTC
Ok, I've split-off -samba and -addons and set the appropriate requires.  

http://web.davehollis.com:81/packages/php-pear-Auth.spec
http://web.davehollis.com:81/packages/php-pear-Auth-1.6.1-2.fc9.src.rpm

I did have to deal with the CRLF in the README.AUTH in the %install portion
rather than %prep because modifying the file changed the MD5 sum and prevented
the PEAR install from doing it's thing.  I am using sed now instead of dos2unix
so there aren't any undesirable dependencies.

Comment 23 Christopher Stone 2008-06-18 13:17:00 UTC
Dave, please make the following changes:

1.  Instead of making an -addons package, simply rm or %exclude those files or
include them in the main package.
2.  Make a -radius subpackage which Requires: Auth_Radius and include RADIUS.php
in the %files.
3.  Remove the %exclude line, it is not needed.
4.  Move the sed line back into %prep and supply a patch for the MD5SUM, see
http://xulchris.fedorapeople.org/packages/php-pear-HTTP-Client.spec as an example
5. I will add php-pear-HTTP-Client today, please add this to the Requires
6. Perform steps I outlined in comment #20

Comment 24 David Hollis 2008-06-18 18:55:39 UTC
1) Removed -addons package
2) Added -radius packages
3) Moved line-feed stuff back to %prep
4) Added HTTP_Client req

Did need to use the %exclude in the main package to keep the sub-package files
from being included there.

The -radius package has a req on php-pear(Auth_Radius) though that package does
not currently exist in F9 that I see.

http://web.davehollis.com:81/packages/php-pear-Auth.spec
http://web.davehollis.com:81/packages/php-pear-Auth-1.6.1-3.fc9.src.rpm

Comment 25 Christopher Stone 2008-06-22 01:12:35 UTC
Okay more fixes needed:

1.  The sub-packages must require their parent package, like so:
Requires:       %{name} = %{version}-%{release}
2.  Please remove all tabs from the spec file
3.  Please do what I mentioned in comment #20 (we need to discuss virtual
provides for pear subpackages)
4.  Please update the %files section like so:

%files
%defattr(-,root,root,-)
%doc %{pear_name}-%{version}/docdir/%{pear_name}/*
%{pear_xmldir}/%{pear_name}.xml
%{pear_phpdir}/%{pear_name}*
%{pear_testdir}/%{pear_name}
%exclude %{pear_phpdir}/%{pear_name}/Container/SMBPasswd.php
%exclude %{pear_phpdir}/%{pear_name}/Container/RADIUS.php

%files samba
%defattr(-,root,root,-)
%{pear_phpdir}/%{pear_name}/Container/SMBPasswd.php

%files radius
%defattr(-,root,root,-)
%{pear_phpdir}/%{pear_name}/Container/RADIUS.php

Comment 27 David Hollis 2008-06-28 12:28:32 UTC
Shoot, just noticed that the requires for the subpackages was incorrect
(php-pear(Auth_Radius) not php-pear(Auth_RADIUS)).  Here's a corrected round:
http://web.davehollis.com:81/packages/php-pear-Auth.spec
http://web.davehollis.com:81/packages/php-pear-Auth-1.6.1-5.fc9.src.rpm

Comment 28 Jason Tibbitts 2008-08-09 17:33:31 UTC
Anything happening here?

Comment 29 David Hollis 2008-08-14 11:15:36 UTC
Waiting on sponsorship or for someone to take this package and run with it.

Comment 30 Rakesh Pandit 2008-08-23 16:16:53 UTC
Are you interested in continuing with review? If yes, then you need to follow suggestions here:

https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

But, in case you don't want to continue with review and it is okay-- I am interested in taking it from here onwards.

Comment 31 David Hollis 2008-08-29 12:09:08 UTC
Feel free to take ownership on this.  It's more important to me that it be packaged and available.  If it means less work on my end, I can't complain!

Comment 32 Rakesh Pandit 2008-08-29 14:48:41 UTC
I had no intention to hijack ;-)

In case you are sponsored later feel free to take it back if interested.

@Christopher
It has been long pending. So, I will start a new ticket so that it is open to other folks for review. In case you are interested in REVIEW may you switch to new ticket. #460660

--
Thanks

*** This bug has been marked as a duplicate of bug 460660 ***


Note You need to log in before you can comment on or make changes to this bug.