Bug 678727 - Review Request: pam_afs_session - AFS PAG and AFS tokens on login (sponsor requested)
Summary: Review Request: pam_afs_session - AFS PAG and AFS tokens on login (sponsor r...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL: http://www.eyrie.org/~eagle/software/...
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-02-19 04:45 UTC by Ken Dreyer
Modified: 2011-05-04 00:58 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-04 00:58:39 UTC
Type: ---
Embargoed:
steve.traylen: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Ken Dreyer 2011-02-19 04:45:51 UTC
Spec URL: http://adiemus.org/~kdreyer/rpms/pam-afs-session.spec
SRPM URL: http://adiemus.org/~kdreyer/rpms/pam-afs-session-2.1-1.src.rpm

Description: pam-afs-session is a PAM module intended for use with a Kerberos v5 PAM module to obtain an AFS PAG (Process Authentication Group) and AFS tokens on login. It puts every new session in a PAG regardless of whether it was authenticated with Kerberos and runs a configurable external program to obtain tokens.

I would like to get this package into Fedora and EPEL 5.

I'm a long-time Fedora user, and I'm looking to be a developer. I maintain a few RPMs at work, but this is my first package in Fedora, so I'm seeking a sponsor.


$ rpmlint SPECS/pam-afs-session.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint SRPMS/pam-afs-session-2.1-1.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint RPMS/i686/pam-afs-session-2.1-1.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint RPMS/i686/pam-afs-session-debuginfo-2.1-1.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Koji scratch builds:
EPEL5: https://koji.fedoraproject.org/koji/taskinfo?taskID=2850664
F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2850668
F14: http://koji.fedoraproject.org/koji/taskinfo?taskID=2850671
F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=2850674

Comment 1 Steve Traylen 2011-02-22 19:43:57 UTC
Hi Ken,

Some initial comments:

1)
Name: pam-afs-session

There are some guidelines on the naming of pam modules:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28httpd.2C_pam.2C_and_SDL.29

though in this case I appreciate it supersedes the normal "use
name of tar ball" which you followed.

2)
A very recent addition to the "rules" is to add %{?_isa} to the 
end of your -devel Buildrequires. e.g.

BuildRequires: pam-devel%{?_isa}

Other than that at first site it's looking pretty good.

Generally have a look at the "Getting Sponsored" pages. Provide some
informal reviews of other pending packages from here:

http://fedoraproject.org/PackageReviewStatus/NEW.html

I'd avoid the "Need sponsor" ones. 

Report back here with some links to some reviews you have made.

Steve.

Comment 2 Ken Dreyer 2011-02-23 06:16:04 UTC
Steve,

Thank you for your suggestions.

(In reply to comment #1)
> though in this case I appreciate it supersedes the normal "use
> name of tar ball" which you followed.

I'm fine with changing it to pam_afs_session. You are right, I was trying to follow the tarball name convention. Do I need to change the name to use underscores?

> 2)
> A very recent addition to the "rules" is to add %{?_isa} to the 
> end of your -devel Buildrequires. e.g.

I incorporated your %{?_isa} suggestion in 2.1-2

Updated Spec (2.1-2): http://adiemus.org/~kdreyer/rpms/pam-afs-session.spec
F14 build (2.1-2): http://koji.fedoraproject.org/koji/taskinfo?taskID=2859436

> Report back here with some links to some reviews you have made.

A few days ago I reviewed #678199 . I can do more reviews next week if necessary.

Comment 3 Steve Traylen 2011-02-23 18:25:55 UTC
Some more reviews would be good. Preferably one you can find problems with,  submitting another
package is also builds a stronger case.

Either pam_afs_session or pam_afs-session , probably the latter.

Please provide direct links to the .src.rpm and .spec at each revision.

Comment 4 Ken Dreyer 2011-03-02 01:18:18 UTC
I've submitted a second package for review at https://bugzilla.redhat.com/681393 .

Comment 5 Ken Dreyer 2011-03-02 17:10:06 UTC
I've also informally reviewed https://bugzilla.redhat.com/664390 .

Comment 6 Steve Traylen 2011-03-02 17:57:06 UTC
Okay I'll look over all of this , just to warn you for me this is a first time so excuse any clunkiness in the process.

Comment 7 Ken Dreyer 2011-03-02 18:24:44 UTC
Steve, thank you.

I hope I'm not interrupting you mid-review, but I've taken your naming suggestion and renamed it to pam_afs_session. This matches the module and the manpage.

http://adiemus.org/~kdreyer/rpms/pam_afs_session.spec
http://adiemus.org/~kdreyer/rpms/pam_afs_session-2.1-3.src.rpm

Koji scratch build:
EL5: http://koji.fedoraproject.org/koji/taskinfo?taskID=2879001
F14: http://koji.fedoraproject.org/koji/taskinfo?taskID=2879014
F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=2879011

Comment 8 Ken Dreyer 2011-03-05 19:44:02 UTC
removing FE-NEEDSPONSOR from this review, per https://fedorahosted.org/fesco/ticket/569

Comment 9 Steve Traylen 2011-03-05 19:52:12 UTC
 spectool pam_afs_session.spec 
Source0: http://archives.eyrie.org/software/afs/pam-afs-session-2.1.tar.gz

this URL looks to have vanished, you need to update to 2.2 I guess.

It's a shame if upstream deletes old versions?

Also you include examples/debian and examples/solaris in your docs 
and these are irrelavent in this context so should be left out.

Steve

Comment 11 Steve Traylen 2011-03-06 16:55:13 UTC
This is really pedantic - sorry.

If you are going to modify the source with
find %{_builddir}/%{buildsubdir}/examples -mindepth 1 -maxdepth 1 -not -name "redhat" -exec rm -rf {} ';'

then that should be in %prep since this  is where you prepare the source.

Otherwise just change 
%doc LICENSE NEWS README TODO examples
to
%doc LICENSE NEWS README TODO examples/redhat

Comment 12 Ken Dreyer 2011-03-07 03:13:40 UTC
(In reply to comment #11)
> This is really pedantic - sorry.

I appreciate your help and the thoroughness of your review.

In the interest of staying closer to upstream's conventions, I want to keep the "examples/redhat/" directory structure. I've moved the find command to the %prep section per your suggestion. Here is 2.2-2:

http://adiemus.org/~kdreyer/rpms/pam_afs_session.spec
http://adiemus.org/~kdreyer/rpms/pam_afs_session-2.2-2.src.rpm

Koji scratch builds:
F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2889613
EL5: http://koji.fedoraproject.org/koji/taskinfo?taskID=2889616

Comment 13 Steve Traylen 2011-03-13 08:17:09 UTC
- Package meets naming and packaging guidelines
It does, pam module ones in particular.
- Spec file matches base package name.
Yes
- Spec has consistant macro usage.
Yes.
- Meets Packaging Guidelines.
Yes
- License
MIT
- License field in spec matches
The LICENSE file is very verbose in specifying all 
the various copy rights but yes it seems to MIT.
- License file included in package
Yes
- Spec in American English
Yes
- Spec is legible.
Yes
- Sources match upstream md5sum:
$ md5sum pam-afs-session-2.2.tar.gz ../SOURCES/pam-afs-session-2.2.tar.g
5621adc56319582b71b61546face4409  pam-afs-session-2.2.tar.gz
5621adc56319582b71b61546face4409  ../SOURCES/pam-afs-session-2.2.tar.gz

- Package needs ExcludeArch
It does not
- BuildRequires correct
Yes.
- Spec handles locales/find_lang
Not  present.
- Package is relocatable and has a reason to be.
Not relocatable.
- Package has %defattr and permissions on files is good.
They are.
- Package has a correct %clean section.
Yes
- Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Package is code or permissible content.
Yes.
- Doc subpackage needed/used.
Not needed.
- Packages %doc files don't affect runtime.
They don't
- Headers/static libs in -devel subpackage.
Not needed.
- Spec has needed ldconfig in post and postun
Not needed.
- .pc files in -devel subpackage/requires pkgconfig
Not needed
- .so files in -devel subpackage.
Not needed.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.
They are.
- Package is a GUI app and has a .desktop file
No gui
- Package compiles and builds on at least one arch.
See koji.
- Package has no duplicate files in %files.
No
- Package doesn't own any directories other packages own.
/lib/security is pulled in via pam which is required - good.
- Package owns all the directories it creates.
- No rpmlint output.

pam_afs_session.x86_64: W: spelling-error %description -l en_US afs -> ads, as, oafs
pam_afs_session-debuginfo.x86_64: W: spelling-error Summary(en_US) afs -> ads, as, oafs
pam_afs_session-debuginfo.x86_64: W: spelling-error %description -l en_US afs -> ads, as, oafs
pam_afs_session.src: W: spelling-error %description -l en_US afs -> ads, as, oafs

- final provides and requires are sane:

$ rpm -qp --provides pam_afs_session-2.2-2.x86_64.rpm 
pam_afs_session.so()(64bit)  
pam_afs_session = 2.2-2
pam_afs_session(x86-64) = 2.2-2

$ rpm -qp --requires pam_afs_session-2.2-2.x86_64.rpm  | grep -v rpm | grep -v libc
libk5crypto.so.3()(64bit)  
libkrb5.so.3()(64bit)  
libkrb5.so.3(krb5_3_MIT)(64bit)  
libpam.so.0()(64bit)  
libpam.so.0(LIBPAM_1.0)(64bit)  
libpam.so.0(LIBPAM_EXTENSION_1.0)(64bit)  
libpam.so.0(LIBPAM_MODUTIL_1.0)(64bit)  
pam_krb5  


SHOULD Items:

- Should build in mock.
See koji.
- Should build on all supported archs
See koji
- Should function as described.
Not checked.
- Should have sane scriptlets.
None
- Should have subpackages require base package with fully versioned depend.
Not relavent.
- Should have dist tag
Yes
- Should package latest version
Yes
- check for outstanding bugs on package. 
Issues:


All seems good. APPROVED.

Steve.

Comment 14 Ken Dreyer 2011-03-16 21:03:13 UTC
New Package SCM Request
=======================
Package Name: pam_afs_session
Short Description: AFS PAG and AFS tokens on login
Owners: ktdreyer
Branches: f13 f14 f15 el5 el6

Comment 15 Jason Tibbitts 2011-03-17 14:12:46 UTC
Git done (by process-git-requests).

Comment 16 Steve Traylen 2011-05-03 18:14:19 UTC
Hi Ken,

You can close this now with "closed nextrelease"

Steve.

Comment 17 Ken Dreyer 2011-05-04 00:58:39 UTC
Thanks Steve, done.


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