Bug 185407 - Review Request: pwgen - Automatic password generation
Review Request: pwgen - Automatic password generation
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thorsten Leemhuis (ignored mailbox)
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-03-14 10:06 EST by James Bowes
Modified: 2013-01-10 04:49 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-04-03 12:21:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description James Bowes 2006-03-14 10:06:35 EST
Spec Name or Url: http://flame.cs.dal.ca/~bowes/packages/pwgen.spec
SRPM Name or Url: http://flame.cs.dal.ca/~bowes/packages/pwgen-2.05-1.src.rpm
Description: pwgen generates random, meaningless but pronounceable passwords. These passwords contain either only lowercase letters, or upper and lower case, or upper case, lower case and numeric digits. Upper case letters and numeric digits are placed in a way that eases memorizing the password.

This is my first package, so I am seeking a sponsor.
Comment 1 Patrice Dumas 2006-03-23 06:52:00 EST
I cannot approve, as you are seeking a sponsor, but here are my comments.

The Source0 is wrong, as it should be un URL, like

Source0:        http://dl.sourceforge.net/sourceforge/pwgen/pwgen-%{version}.tar.gz

rpmlint gives the following warnings, that should be easily sorted out:

W: pwgen strange-permission pwgen-2.05.tar.gz 0755
W: pwgen strange-permission pwgen.spec 0755

Those 2 items are blockers. 

The remaining are comments.

Have you verified that the pending patch ahs been merged? (at a quick glance it
seems so)
http://sourceforge.net/tracker/index.php?func=detail&aid=1108220&group_id=28391&atid=393206

I personnally prefer to use globs for man pages extensions such that rpmbuild
picks the man page whatever compression scheme is used (even none). The
corresponding entry in files becomes:

%{_mandir}/man1/pwgen.1* 


Otherwise everything seems ok.
Comment 2 James Bowes 2006-03-24 21:14:19 EST
Locations for updated files:
Spec Url: http://flame.cs.dal.ca/~bowes/packages/pwgen/rev2/pwgen.spec
SRPM Url: http://flame.cs.dal.ca/~bowes/packages/pwgen/rev2/pwgen-2.05-2.src.rpm

(In reply to comment #1)
> I cannot approve, as you are seeking a sponsor, but here are my comments.
> 
> The Source0 is wrong, as it should be un URL, like
> 
> Source0:       
http://dl.sourceforge.net/sourceforge/pwgen/pwgen-%{version}.tar.gz
> 

Fixed.

> rpmlint gives the following warnings, that should be easily sorted out:
> 
> W: pwgen strange-permission pwgen-2.05.tar.gz 0755
> W: pwgen strange-permission pwgen.spec 0755
> 
> Those 2 items are blockers. 
> 

Fixed. I have no idea how that happened in the first place...

> The remaining are comments.
> 
> Have you verified that the pending patch ahs been merged? (at a quick glance it
> seems so)
>
http://sourceforge.net/tracker/index.php?func=detail&aid=1108220&group_id=28391&atid=393206
> 

From what I can tell, it has been.

> I personnally prefer to use globs for man pages extensions such that rpmbuild
> picks the man page whatever compression scheme is used (even none). The
> corresponding entry in files becomes:
> 
> %{_mandir}/man1/pwgen.1* 
> 

Ah, good point. That is now fixed.

> 
> Otherwise everything seems ok.

Thanks for the feedback!
Comment 3 Jose Pedro Oliveira 2006-03-24 21:41:47 EST
James,

A couple more notes:

* the dist tag is missing; 
  the release entry should be something like:

  Release:        2%{?dist}

* [BLOCKER] don't strip the binary;
  rpmbuild will do that for you (when it tries to create the debuginfo RPM)

  just delete the line "strip $RPM_BUILD_ROOT/%{_bindir}/pwgen"

jpo
  
Comment 4 James Bowes 2006-03-25 13:34:17 EST
Locations for updated files:
Spec Url: http://flame.cs.dal.ca/~bowes/packages/pwgen/rev3/pwgen.spec
SRPM Url: http://flame.cs.dal.ca/~bowes/packages/pwgen/rev3/pwgen-2.05-3.src.rpm

(In reply to comment #3)
> James,
> 
> A couple more notes:
> 
> * the dist tag is missing; 
>   the release entry should be something like:
> 
>   Release:        2%{?dist}
> 

Fixed.

> * [BLOCKER] don't strip the binary;
>   rpmbuild will do that for you (when it tries to create the debuginfo RPM)
> 
>   just delete the line "strip $RPM_BUILD_ROOT/%{_bindir}/pwgen"
> 

Fixed.

> jpo
>   

Thanks for the feedback!
Comment 5 Jose Pedro Oliveira 2006-03-27 11:54:41 EST
PUBLISH++

MD5SUMS:
0b6a48c327fb103d634486c4a64052f0  pwgen-2.05-3.src.rpm

b94546a346cb352054ea2d3d09f7f885  pwgen-2.05.tar.gz
4aaddffd833922b16b61c06baecbab1d  pwgen.spec

Good:
* Source tarball MD5 digest verified
* URL and Source URL are valid
* License verified
  (only mentioned in http://sourceforge.net/projects/pwgen/)
* RPM_OPT_FLAGS are present during the building step
* File permissions are valid (rpm -qplv ...)
* Builds without problems in FC-3, FC-4, and FC-5

Note:
This is my vote for approval. The final word belongs to Patrice as he has done
the first review.
Comment 6 Patrice Dumas 2006-03-27 15:53:31 EST
(In reply to comment #2)

> > %{_mandir}/man1/pwgen.1* 
> > 
> 
> Ah, good point. That is now fixed.

This still isn't a blocker, but the way you made the glob, the man page won't be
taken if there is no compression. Indeed in that case, the file name will be
%{_mandir}/man1/pwgen.1
which isn't taken by your glob
%{_mandir}/man1/pwgen.1.*
and that's why I advertise
%{_mandir}/man1/pwgen.1*
Comment 7 Patrice Dumas 2006-03-27 16:01:19 EST
(In reply to comment #5)

> This is my vote for approval. The final word belongs to Patrice as he has done
> the first review.

I interpret that you are sponsoring James in that case... 

APPROVED

There is only a minor issue, I think the changelog is more readable if there is
an empty line between release fields, like

* Sat Mar 25 2006 James Bowes <jbowes@redhat.com> - 2.05-3
- Add dist tag to release.
- Don't strip binary, since rpmbuild will do it.

* Fri Mar 24 2006 James Bowes <jbowes@redhat.com> - 2.05-2
- Use url for Source0 in spec file.
- Use glob for man page extension.
- Increment release number.

And also you can remove the line
- Increment release number.
as it is redundant with having a new changelog entry.

No need to add a changelog entry for those changes, nor if you change the glob
for man pages, and you can do that after importing in the cvs.
Comment 8 James Bowes 2006-04-03 12:21:11 EDT
Thanks for all the help!
Comment 9 Christian Iseli 2006-04-08 17:00:10 EDT
Moved blocker to FE-ACCEPT.
Comment 10 Patrice Dumas 2006-07-21 16:23:44 EDT
This bug should be assigned to jpo, I believe.
Comment 11 James Bowes 2007-12-21 09:01:37 EST
Package Change Request
======================
Package Name: pwgen
New Branches: EL-4 EL-5
Comment 12 Kevin Fenzi 2007-12-21 14:39:43 EST
cvs done.

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