Bug 239096

Summary: Review Request: perl-libwhisker2 - Perl module geared specificly for HTTP testing
Product: [Fedora] Fedora Reporter: Sindre Pedersen Bjørdal <sindrepb>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: rayvd
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: j: fedora-review+
petersen: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-05-27 04:41:40 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 239097    

Description Sindre Pedersen Bjørdal 2007-05-04 20:47:20 UTC
Spec URL: http://folk.ntnu.no/sindrb/packages/green_nyc/perl-libwhisker2.spec
SRPM URL: http://folk.ntnu.no/sindrb/packages/green_nyc/perl-libwhisker2-2.4-1.fc7.src.rpm

Description: 
Libwhisker is a Perl module geared specificly for HTTP testing.

Comment 1 Jason Tibbitts 2007-05-07 22:21:04 UTC
I'll take a look.

Comment 2 Jason Tibbitts 2007-05-08 02:00:27 UTC
Looks clean, but there are a couple of problems:
  W: perl-libwhisker2 unversioned-explicit-obsoletes perl-libwhisker
  W: perl-libwhisker2 unversioned-explicit-provides perl-libwhisker
When you obsolete a package in this manner, you need to use versioned provides
and obsoletes.  The simplest thing is to obsolete <= the current perl-libwhisker
version, and provide current version+epsilon.

libwhisker2-2.4-lw1bridge.patch basically adds a GPL license statement onto a
file.  While I agree that it's best if each source file contains the
(abbreviated) license statement, this is generally something that upstream
should be doing instead of package maintainers.  

There's a test suite, and it actually works fine, but because it uses fixed
ports it's not really suitable for running within the buildsys.  Stick this in a
%check section to run it:
  cd t
  perl ./test.pl
Still, it would need to be commented out in the spec or made conditional on a
flag so the buildsys wouldn't break.

There's a typo in the Source0: line; "wiretripnet" should be "wiretrip.net".

I'm not sure why you have those BuildRequires:; the package seems identical
without them.  What you may need is some Requires: instead, because the built
package doesn't end up with any.  Oddly, though, the test suite still passes. 

* source files match upstream:
   5d136f6de318e140a03f76a50392628ed7d00fbee297579ae29b81466f8c8e00  
   libwhisker2-2.4.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
? BuildRequires are proper.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
X rpmlint has complaints about unversioned provides and obsoletes.
? the dependencies seem a bit light:
   perl(LW)
   perl(LW2)
   perl-libwhisker
   perl-libwhisker2 = 2.4-1.fc7
  =
   perl(:MODULE_COMPAT_5.8.8)
   perl(LW2)
   perl(strict)
   perl(vars)

* %check is not present.  There is a test suite, but it's not appropriate for 
  running inside the buildsys. All tests pass when run manually.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Comment 3 Sindre Pedersen Bjørdal 2007-05-08 14:56:33 UTC
Update:

- Fix typo in Source0 url
- Update lw1bridge patch to not include License info
- Add explicit version to Provides and Obsoletes
- Added tests, commented out
- Cleaned up BuildRequires and Requires

The Requires on openssl-perl and perl-Net-SSLeay I know are valid. The MD5
dependency I'm not sure about, but I think the LW2.pm actually does use it. Can
you verify?

Spec URL: http://folk.ntnu.no/sindrb/packages/green_nyc/perl-libwhisker2.spec
SRPM URL:
http://folk.ntnu.no/sindrb/packages/green_nyc/perl-libwhisker2-2.4-2.fc7.src.rpm

Comment 4 Jason Tibbitts 2007-05-09 05:53:34 UTC
OK, builds fine and rpmlint is quiet.

You don't need "BuildRequires: perl" but it doesn't hurt anything to have it.

Note for others who may check for BuildRequires: needed for the perl-devel
split: this module doesn't build like a regular module and has a custom
Makefile.pl which doesn't make use of the regular Perl build infrastructure.

Yes, this package does at least try to use MD5 at some point although it will
use an internal implementation if the module isn't available.  At least
according to my reading of src/mdx.pl.

The only issue at this point that concerns me is the name of the package.  Our
naming guidelines indicate that packages should be named according to how
they're carried in CPAN, but of course this module isn't in CPAN.  According to
CPAN rules it wold be called perl-LW2, but that seems somewhat suboptimal as
well.  Frankly I favor the current naming, but before I approve I'll gather a
couple of other opinions.

Comment 5 Jason Tibbitts 2007-05-09 20:40:32 UTC
OK, I've thought about it some more and I'm happy with the current naming. 
libwhisker2 is the name that upstream users and that people would expect to find
this package under, we can't guess at what CPAN would name the module since it
accepts module names that are not identical to the name of the topmost defined
package, and the expected Perl library provides perl(LW2) are still there for
automatic dependencies.

APPROVED

Comment 6 Sindre Pedersen Bjørdal 2007-05-11 15:16:32 UTC
New Package CVS Request
=======================
Package Name: perl-libwhisker2
Short Description: Perl module geared specificly for HTTP testing
Owners: foolish
Branches: FC-5 FC-6 EL-4 EL-5
InitialCC: jaboutbo

Comment 7 Ray Van Dolson 2007-05-23 01:22:27 UTC
Found this as a result of a review on nikto (#239097).

They LW1 compatability bridge does not appear to be being installed correctly:

# rpm -ql perl-libwhisker2.noarch
/usr/lib/perl5/vendor_perl/5.8.8/LW2.pm
/usr/share/doc/perl-libwhisker2-2.4
/usr/share/doc/perl-libwhisker2-2.4/CHANGES
/usr/share/doc/perl-libwhisker2-2.4/LICENSE
/usr/share/doc/perl-libwhisker2-2.4/README
/usr/share/man/man3/LW2.3pm.gz

(Note: this is from FC6, but I think the same would occur in FC7)

Building from the .spec file I see the following:

Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.98077
+ umask 022
+ cd /net/leoray/leoray1/users/ray5147/rpmbuild/BUILD
+ cd libwhisker2-2.4
+ LANG=C
+ export LANG
+ unset DISPLAY
+ rm -rf /var/tmp/perl-libwhisker2-2.4-2-root-ray5147
+ mkdir -p
/var/tmp/perl-libwhisker2-2.4-2-root-ray5147/usr/lib/perl5/vendor_perl/5.8.8
+ mkdir -p /var/tmp/perl-libwhisker2-2.4-2-root-ray5147/usr/share/man/man3
+ make install DESTDIR=/var/tmp/perl-libwhisker2-2.4-2-root-ray5147
export DESTDIR
perl Makefile.pl install
LW2.pm installed to
/var/tmp/perl-libwhisker2-2.4-2-root-ray5147/usr/lib/perl5/vendor_perl/5.8.8
LW2.3pm installed to /var/tmp/perl-libwhisker2-2.4-2-root-ray5147/usr/share/man/man3
perl Makefile.pl install_compat
Error: bad build command 'install_compat'

The solution is to use this libwhisker2-2.4-lw1bridge.patch file instead of the
one in the SRPM:

http://www.bludgeon.org/~rayvd/rpms/perl-libwhisker2/libwhisker2-2.4-lw1bridge.patch

The only difference being that we use "perl Makefile.pl install_lw1" as per the
documentation instead of "perl Makefile.pl install_compat"

Once I make this change I now see the LW v1 compatability bridge installed
correctly and can use the nikto package.

Comment 8 Sindre Pedersen Bjørdal 2007-05-23 11:46:57 UTC
patch applied and package rebuilt in koji for devel, which is F8. The F7 branch
is  missing from all my packages, I'm looking into this. 

Comment 9 Sindre Pedersen Bjørdal 2007-05-23 15:09:23 UTC
Seems the F7 branch wasn't created during merge, please add it.

New Package CVS Request
=======================
Package Name: halberd
Short Description: Tool to discover HTTP load balancers
Owners: foolish
Branches: FC-6 EL-4 EL-5 F-7



Comment 10 Jens Petersen 2007-05-27 04:41:40 UTC
You mean:

Package Change Request
======================
Package Name: perl-libwhisker2
New Branches: F-7

Again, please do not reopen Package Review's for CVS Requests,
since they are handled by the fedora-cvs flag not bug state.

F-7 branch edded.