[Bug 225659] Merge Review: cracklib

2009-01-14 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225659


Jason Tibbitts  changed:

   What|Removed |Added

 CC||ti...@math.uh.edu




--- Comment #5 from Jason Tibbitts   2009-01-14 21:18:31 EDT 
---
Hmm, nothing from Jef.  I took a quick look, though, and while the spec looks
very nice and clean, I do have a couple of comments.

Were this a new package that hadn't existed for approximately forever, I would
complain about these files:
  /usr/include/crack.h
  /usr/include/packer.h
  /usr/sbin/mkdict
  /usr/sbin/packer
as being terribly generic.  However, if the first package in wins then this
beats everything by default, and there's obviously no point in changing it now.

rpmlint does have a few complaints which can be essentially ignored:

  cracklib-devel.x86_64: W: no-documentation
  cracklib-dicts.x86_64: W: no-documentation
  cracklib-python.x86_64: W: no-documentation
True but not problematic.

  cracklib-dicts.x86_64: E: only-non-binary-in-usr-lib
This is true, but I don't really see a problem in what you're doing.  Does
anything absolutely require those symlinks to be there?  If it didn't, you
could conceivably make this subpackage noarch (once the buildsys support for
that is finished).

  cracklib-dicts.x86_64: W: dangling-relative-symlink /usr/sbin/packer 
   cracklib-packer
  cracklib-dicts.x86_64: W: dangling-relative-symlink /usr/sbin/mkdict 
   cracklib-format
These are fine; the dependencies ensure that the symlinks aren't dangling.

  cracklib.x86_64: W: shared-lib-calls-exit /usr/lib64/libcrack.so.2.8.0 
   e...@glibc_2.2.5
This is kind of weird, and usually indicates a bug or thinko in the package,
but isn't a packaging issue.

It also complains about a couple of things which could do with fixing:

  cracklib-dicts.x86_64: W: summary-ended-with-dot The standard CrackLib 
   dictionaries.
Terribly minor, but perhaps worth fixing if you're in the spec.

  cracklib.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/cracklib-unpacker 
   ['/usr/lib64']
  cracklib.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/cracklib-packer 
   ['/usr/lib64']
  cracklib.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/cracklib-check 
   ['/usr/lib64']
  cracklib-python.x86_64: E: binary-or-shlib-defines-rpath 
   /usr/lib64/python2.6/site-packages/_cracklibmodule.so ['/usr/lib64']
I don't know why these didn't turn up earlier.  Maybe libtool2 actually makes
things worse?  In any case, the recommended hack of tweaking libtool didn't
help, so I guess a call to chrpath is needed.

If the latter were fixed, I'd have no qualms about approving this.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2009-02-19 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225659





--- Comment #6 from Nalin Dahyabhai   2009-02-19 13:42:34 EDT 
---
(In reply to comment #5)
>   cracklib-dicts.x86_64: E: only-non-binary-in-usr-lib
> This is true, but I don't really see a problem in what you're doing.  Does
> anything absolutely require those symlinks to be there?  If it didn't, you
> could conceivably make this subpackage noarch (once the buildsys support for
> that is finished).

The FascistCheck() function in the library takes an absolute path to the
dictionaries to check, so there's an unknown number of packages out there that
hard-code locations under /usr/lib /usr/lib64 (though, come to think of it,
there could be some mistakenly referencing /usr/lib on 64-bit systems... ugh).

> It also complains about a couple of things which could do with fixing:
> 
>   cracklib-dicts.x86_64: W: summary-ended-with-dot The standard CrackLib 
>dictionaries.
> Terribly minor, but perhaps worth fixing if you're in the spec.

Agreed, fixed.

>   cracklib.x86_64: E: binary-or-shlib-defines-rpath 
> /usr/sbin/cracklib-unpacker 
>['/usr/lib64']
>   cracklib.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/cracklib-packer 
>['/usr/lib64']
>   cracklib.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/cracklib-check 
>['/usr/lib64']
>   cracklib-python.x86_64: E: binary-or-shlib-defines-rpath 
>/usr/lib64/python2.6/site-packages/_cracklibmodule.so ['/usr/lib64']
> I don't know why these didn't turn up earlier.  Maybe libtool2 actually makes
> things worse?  In any case, the recommended hack of tweaking libtool didn't
> help, so I guess a call to chrpath is needed.

Weird indeed.  A local rebuild seems to keep cracklib-packer from being
afflicted again, so I'm going to check in the summary change and throw it at
the build system.
...
Hmm, looks good from here.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2009-02-19 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225659


Jason Tibbitts  changed:

   What|Removed |Added

 Status|ASSIGNED|CLOSED
 Resolution||RAWHIDE
 AssignedTo|na...@redhat.com|ti...@math.uh.edu
   Flag|fedora-review-  |fedora-review+




--- Comment #7 from Jason Tibbitts   2009-02-19 14:04:32 EDT 
---
I know there was a problem with libtool and rpath which has been fixed very
recently (in 2.2.6-10) and indeed when I build this package now the rpath issue
is gone.

So I'd say that everything looks good to me, and I'll twiddle the flags and
close this out.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2008-12-04 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225659


Jason Tibbitts <[EMAIL PROTECTED]> changed:

   What|Removed |Added

   Flag|needinfo?   |




--- Comment #4 from Jason Tibbitts <[EMAIL PROTECTED]>  2008-12-04 14:57:01 EDT 
---
Wow, this one hasn't been touched for a while.  Jef, did you want to continue
this review?

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2007-02-11 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225659


[EMAIL PROTECTED] changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 AssignedTo|[EMAIL PROTECTED]|[EMAIL PROTECTED]
   Flag||fedora-review?




-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2007-02-11 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225659





--- Additional Comments From [EMAIL PROTECTED]  2007-02-11 06:19 EST ---
review checklist for cracklib 

Sumary NOT APPROVED. See the notes below for full details. Attached to this
report is a diff of the specfile which includes all of my suggested changes. 
The package owner should review the diff if there are items which can not be
incorporated, bring it up as a comment to this report for discussion. 

NOTE:
Not all the blockers have been address in the specfile diff. There are items
which the package owner must address which are not obvious specfile fixes.


+ GOOD   - BAD
+ rpmlint... see the notes at the end. I've rolled in changes into the spec from
the rpmlint log info
+ packagename is fine
+ specfile name is fine
+ license check 
Artistic , matches source license for SOURCE0, and LICENSE file included 
in %doc spec is english-ish
- md5sum check of sources
9a8c9eb26b48787c84024ac779f64bb2  cracklib-2.8.9.tar.gz from SOURCE0 URL
9a8c9eb26b48787c84024ac779f64bb2 
/home/jspaleta/rpmbuild/SOURCES/cracklib-2.8.9.tar.gz

md5sum check:
ASSurnames.gz: OK
cartoon.gz: OK
common-passwords.txt.gz: OK
Congress.gz: OK
cracklib-2.8.9.tar.gz: OK
cracklib-words.gz: FAILED
Domains.gz: OK
Dosref.gz: OK
etc-hosts.gz: OK
famous.gz: OK
fast-names.gz: OK
female-names.gz: OK
Ftpsites.gz: OK
Given-Names.gz: OK
Jargon.gz: OK
LCarrol.gz: OK
male-names.gz: OK
Movies.gz: OK
myths-legends.gz: OK
names.french.gz: OK
names.hp.gz: OK
other-names.gz: OK
Paradise.Lost.gz: OK
Python.gz: OK
sf.gz: OK
shakespeare.gz: OK
surnames.finnish.gz: OK
Trek.gz: OK

d18e670e5df560a8745e1b4dede8f84f  cracklib-words.gz from SOURCE1 URL
575a44add4db95b43c7abb46b307950f  
/home/jspaleta/rpmbuild/SOURCES/cracklib-words.gz

+ mock build  as done by matt
http://linux.dell.com/files/fedora/FixBuildRequires/mock-results-core/i386/cracklib-2.8.9-8.src.rpm/result/
- buildrequires
removed gzip as exception provided by buildsys...fixed in specfile diff
+ shared libs exist and ldconfig is run in the correct script actions
+ not designed to be relocatable
+ no duplicates in the files section
+ file permissions look okay to me
- static libs are currently included in the -devel package.. 
this is a no no unless you have a good reason

+ docs section looks fine
- devel subpackage, includes .so and headerfiles... and a static .a which it
should not, unless it really really needs to.
+ no gui apps
+ no obvious duplicate file/directory ownership

BAD
*cracklib-words.gz included sources does NOT match upstream according to the
checksum. This is a little sticky, since the upstream file is not versioned nor
in a versioned directory. if upstream continues to update this file
its difficult to make a version comparison to some known state.

*pass-file.gz doesn't appear to have an upstream URL even though its listed as a
source, so there is no upstream to md5sum to check against.  If there is no
upstream for this file, this should be at least noted as a comment in the
specfile. I don't think this its super critical considering this is a dictionary
file and not the functional codebase itself... but it definitely should be noted
in the specfile that its an inhouse creation and why it was created.  This one I
can't fix in my specfile diff because I don't know why that file is there.

*gzip as a buildrequires, removed in specfile diff as a buildsys provided 
exception

* is there a compelling reason to include the static libcrack.a in the -devel
package? The guidelines frown very heavily on including static libraries, and
there needs to be a compelling reason to do so. Removed from the package in the
specfile diff

*cracklib-dicts needs cracklib because of the symlinks in /usr/sbin/
...fixed in specfile diff.

rpmlint log from dell
...reviewer comments inline

rpmlint on cracklib-2.8.9-8.i386.rpm
W: cracklib summary-ended-with-dot A password-checking library.
... fixed in specfile diff
E: cracklib tag-not-utf8 %changelog
... fixed in specfile diff
W: cracklib one-line-command-in-%trigger /sbin/ldconfig
... not sure about this one

rpmlint on cracklib-2.8.9-8.src.rpm
W: cracklib summary-ended-with-dot A password-checking library.
E: cracklib tag-not-utf8 %changelog
E: cracklib non-utf8-spec-file cracklib.spec
... fixed in specfile diff
W: cracklib mixed-use-of-spaces-and-tabs (spaces: line 102, tab: line 108)
... fixed in specfile diff

rpmlint on cracklib-devel-2.8.9-8.i386.rpm
W: cracklib-devel summary-ended-with-dot Development files needed for building
applications which use cracklib.
... fixed in diff
E: cracklib-devel tag-not-utf8 %changelog
W: cracklib-devel no-documentation
... bogus not important

rpmlint on cracklib-python-2.8.9-8.i386.rpm
W: cracklib-python summary-ended-with-dot Python bindings for applications which
use cracklib.
... fixed

[Bug 225659] Merge Review: cracklib

2007-02-11 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225659





--- Additional Comments From [EMAIL PROTECTED]  2007-02-11 06:21 EST ---
Created an attachment (id=147852)
 --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=147852&action=view)
specfile diff which includes suggested fixes for merge review issues


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2007-02-11 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225659


[EMAIL PROTECTED] changed:

   What|Removed |Added

 AssignedTo|[EMAIL PROTECTED]  |[EMAIL PROTECTED]
   Flag|fedora-review?  |fedora-review-




-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2007-02-12 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225659


[EMAIL PROTECTED] changed:

   What|Removed |Added

 Status|ASSIGNED|NEEDINFO
   Flag||needinfo?




--- Additional Comments From [EMAIL PROTECTED]  2007-02-12 20:06 EST ---
Thanks for the detailed review!

I don't have a good reason for leaving the static library in there, so it's
gone.  I've updated the cracklib-words.gz file and noted its retrieval time in
the .spec file, and noted that the pass-file.gz comes from bugzilla.  The
trigger script now specifies ldconfig as its interpreter, so it shouldn't need
the shell at that stage.  I think that's got it sorted, building 2.9-9 as a
candidate fix.

Please let me know if you (or anyone else reading this) spots anything I've 
missed.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review


[Bug 225659] Merge Review: cracklib

2007-12-20 Thread bugzilla
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: cracklib


https://bugzilla.redhat.com/show_bug.cgi?id=225659


[EMAIL PROTECTED] changed:

   What|Removed |Added

   Severity|normal  |medium
   Priority|normal  |medium
Product|Fedora Extras   |Fedora
Version|devel   |rawhide

[EMAIL PROTECTED] changed:

   What|Removed |Added

OtherBugsDependingO||426387
  nThis||




-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.

___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review