Bug 678727

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.

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.

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.

I've submitted a second package for review at https://bugzilla.redhat.com/681393 .

I've also informally reviewed https://bugzilla.redhat.com/664390 .

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.

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

removing FE-NEEDSPONSOR from this review, per https://fedorahosted.org/fesco/ticket/569

 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

It looks like upstream just moves older versions to an ARCHIVE directory, eg.

http://archives.eyrie.org/software/ARCHIVE/pam-afs-session/pam-afs-session-2.1.tar.gz

I've updated to 2.2, and removed the examples for other platforms.

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

Koji scratch builds:
F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2887900
F14: http://koji.fedoraproject.org/koji/taskinfo?taskID=2887933
F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=2887936
EL5: http://koji.fedoraproject.org/koji/taskinfo?taskID=2887922
EL6: http://koji.fedoraproject.org/koji/taskinfo?taskID=2887907

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

(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

- 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.

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

Git done (by process-git-requests).

Hi Ken,

You can close this now with "closed nextrelease"

Steve.

Thanks Steve, done.