Bug 226225 - Merge Review: pam_krb5
Summary: Merge Review: pam_krb5
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:19 UTC by Nobody's working on this, feel free to take it
Modified: 2010-08-09 04:58 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-09 04:58:51 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora-review+


Attachments (Terms of Use)
pam_krb5-spec-cleanup.spec (1.71 KB, patch)
2010-07-24 14:19 UTC, Parag AN(पराग)
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 20:19:46 UTC
Fedora Merge Review: pam_krb5

http://cvs.fedora.redhat.com/viewcvs/devel/pam_krb5/
Initial Owner: nalin@redhat.com

Comment 1 Parag AN(पराग) 2010-07-24 14:18:29 UTC
1) rpmlint reported
pam_krb5.src: W: spelling-error %description -l en_US pam -> map, Pam, pan
pam_krb5.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
pam_krb5.src: W: invalid-url URL: https://fedorahosted.org/pam_krb5/ <urlopen error [Errno -2] Name or service not known>
pam_krb5.i686: W: spelling-error %description -l en_US pam -> map, Pam, pan
pam_krb5.i686: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
pam_krb5.i686: W: invalid-url URL: https://fedorahosted.org/pam_krb5/ <urlopen error [Errno -2] Name or service not known>
2 packages and 0 specfiles checked; 0 errors, 6 warnings.
==> This can be ignored

2)timestamps should be preserved.Use INSTALL="install -p" when installing to
preserve timestamps.

3) I will suggest this package to follow current packaging guidelines and
remove buildroot, %clean section and cleaning of build root in %install    

4) Should follow
https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

Comment 2 Parag AN(पराग) 2010-07-24 14:19:24 UTC
Created attachment 434148 [details]
pam_krb5-spec-cleanup.spec

please add changelog entry

Comment 3 Parag AN(पराग) 2010-07-24 14:26:15 UTC
Also,
 As per https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags , please add comment in spec why you need extras flags.

Comment 4 Nalin Dahyabhai 2010-07-26 19:33:30 UTC
Thank you for picking up this review!

(In reply to comment #1)
> 2)timestamps should be preserved.Use INSTALL="install -p" when installing to
> preserve timestamps.

Done.
 
> 3) I will suggest this package to follow current packaging guidelines and
> remove buildroot, %clean section and cleaning of build root in %install    

Okay, done.
 
> 4) Should follow
> https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make    

Done.

(In reply to comment #3)
> Also,
>  As per https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags ,
> please add comment in spec why you need extras flags.    

I think at the time (and this would have been quite a while ago) libtool wasn't doing the right thing and building the contents of the module with -fPIC, but it seems to be doing the right things now.  Removing the CFLAGS tweaks.

The changes are made in upstream git if you'd like to review them:
  http://git.fedorahosted.org/git/?p=pam_krb5.git;a=blob;f=pam_krb5.spec

Comment 5 Parag AN(पराग) 2010-07-27 09:13:03 UTC
Thanks. New spec looks good.

Just minor thing, if adding my name please use just "Parag AN" as when seen plain text spec file in git it showed garbage characters in parenthesis :)

Please commit in devel and build new package and will approve this review.

Comment 6 Nalin Dahyabhai 2010-07-27 15:01:14 UTC
Whoops, sorry about that.  I can't read that script, so I tried to copy and paste from bugzilla.  Should be all checked-in and built now.

Comment 7 Parag AN(पराग) 2010-08-09 04:58:51 UTC
Thanks for the changes.

Package is APPROVED now.


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