Bug 226225 - Merge Review: pam_krb5
Merge Review: pam_krb5
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:19 EST by Nobody's working on this, feel free to take it
Modified: 2010-08-09 00:58 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-09 00:58:51 EDT
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 10:19 EDT, Parag AN(पराग)
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:19:46 EST
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 10:18:29 EDT
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 10:19:24 EDT
Created attachment 434148 [details]
pam_krb5-spec-cleanup.spec

please add changelog entry
Comment 3 Parag AN(पराग) 2010-07-24 10:26:15 EDT
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 15:33:30 EDT
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 05:13:03 EDT
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 11:01:14 EDT
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 00:58:51 EDT
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.