Bug 243665 - Review Request: perl-Geo-IP - Efficient GeoIP bindings for Perl
Review Request: perl-Geo-IP - Efficient GeoIP bindings for Perl
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-11 07:57 EDT by Michael Fleming
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-13 02:49:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michael Fleming 2007-06-11 07:57:12 EDT
Spec URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP.spec
SRPM URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP-1.27-3.fc7.mf.src.rpm
Description:
This package contains Perl bindings for the GeoIP IP/hostname to country/location/organization database. This package requires Maxmind's GeoIP libraries but is often faster than other, similar modules.
Comment 1 Jason Tibbitts 2007-06-15 14:24:05 EDT
Heh, you beat me to this.  I just used cpanspec to generate mine.

Some comments:

License is GPL or Artistic, not GPL, at least according to my reading of the
README file.

I referenced the module in the ususl CPAN locations:

URL:            http://search.cpan.org/dist/Geo-IP/
Source0:       
http://www.cpan.org/authors/id/T/TJ/TJMATHER/Geo-IP-%{version}.tar.gz

I guess it doesn't really matter either way, but CPAN is usually the canonical
location for Perl modules.

Please remove the "mf" from Release:.

When adding Perl module dependencies, please depend on the perl() symbol and not
the package name: perl(ExtUtils::MakeMaker).  There is no guarantee that the
module will stay in a separate package.

The package has a test suite; any reason for not running it?

Any reason not to package the example directory as documentation?
Comment 2 Michael Fleming 2007-06-17 07:03:52 EDT
I've added the examples directory and the perl(Extutils::MakeMaker) BR, as well
as changed the URLs.

The test suite actually fails (and kills the package build) on one subtest
(which is pretty stupid really - fails to locate yahoo.com in "US" as expected,
even when the GeoIP data does so correctly via the GeoIP packages geoiplookup
tool does the right thing)

However the package itself would operate normally regardless - I've had it
locally for many months (for Awstats usage) without a hiccup. However I can see
a benefit in using the tests and a patch to fix the above bogosity is something
I can easily hack in.

I'll put up a new package once that's done.
Comment 3 Michael Fleming 2007-07-05 08:29:44 EDT
Sorry if this is dragging - I've been ill this week.

I'll patch out the offending bogus test suite failure and release another
version tomorrow or Saturday my time which should clear the remaining issues.
Comment 4 Michael Fleming 2007-07-10 06:21:06 EDT
As it happens there's a new release and it at least builds and tests without
compile errors now. Anyone want to give this a spin?

Spec URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP.spec
SRPM URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP-1.28-1.fc7.src.rpm
Comment 5 Jason Tibbitts 2007-07-10 12:54:53 EDT
Unfortunately the test suite fails for me because it tries to look up yahoo.com,
but mock does not run with a configured DNS server.  I inserted
   perl -pi -e 's/yahoo.com.*//' t/2_namelookup.t
before the "make test" line and it runs to completion.  I guess the test count
should be off but it doesn't seem to bother anything.  You could also just
delete t/2_namelookup.t.

Please make consistent use of macros; don't use both "%{__perl}" and "perl",
just pick one.  But if you want to pick the macro-ized version, you need to use
the macroized versions of rm, make, chmod, etc. as well.

You don't need the manual dependency on GeoIP; rpm finds the libGeoIP dependency
on its own.

Review:
* source files match upstream:
   bede8433e200a433cc0defd198136d5d6dd0580542a92f516a707f4829db0c52  
   Geo-IP-1.28.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X specfile does not use macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
? final provides and requires; see extraneous GeoIP dependency.
   IP.so()(64bit)
   perl(Geo::IP) = 1.28
   perl(Geo::IP::Record)
   perl(Geo::Mirror) = 1.0
   perl-Geo-IP = 1.28-1.fc8
  =
?  GeoIP
   libGeoIP.so.1()(64bit)
   perl(:MODULE_COMPAT_5.8.8)
   perl(Geo::IP::Record)
   perl(base)
   perl(constant)
   perl(strict)
   perl(vars)

X %check is present, but test fail in mock due to missing DNS services:
   t/2_namelookup....
   # Test 11 got: <UNDEF> (t/2_namelookup.t at line 18 fail #11)
   #    Expected: "US"
   #  t/2_namelookup.t line 18 is:   ok($country, $exp_country);
   FAILED test 11
        Failed 1/11 tests, 90.91% okay
   t/3_mirror........ok
   Failed Test      Stat Wstat Total Fail  Failed  List of Failed
   -----------------------------------------------------------------------------
   t/2_namelookup.t               11    1   9.09%  11
   Failed 1/4 test scripts, 75.00% okay. 1/33 subtests failed, 96.97% okay.

* no shared libraries are added to the regular linker search paths.
* 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.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
Comment 6 Jason Tibbitts 2007-07-19 20:40:45 EDT
Ping?
Comment 7 Michael Fleming 2007-07-20 07:13:08 EDT
Pong.

Patched out that weird namelookup failure 

Spec URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP.spec
SRPM URL: http://www.thatfleminggent.com/fedorasubs/perl-Geo-IP-1.28-2.fc7.src.rpm

Go wild :-)
Comment 8 Jason Tibbitts 2007-07-27 02:58:53 EDT
I'm still getting the same failure.  I don't see anything in the spec which
would mess with the failing test

The package still uses macros inconsistently.  It seems like you just want to
change the single instance of "%{__perl}" to just "perl", unless you want to
change all of the perl, make, rm, and chmod calls to %{__whatever}.
Comment 9 Jason Tibbitts 2007-08-25 13:07:05 EDT
So, anything happening here?  The last package you put up doesn't have anything
which would alter the failing test at all; maybe it's the wrong package?
Comment 10 Michael Fleming 2007-08-25 20:33:54 EDT
Yeah, sorry - been otherwise busy. 

I've found and fixed a braino in my patch so it applies correctly (now that the
spec actually uses it) . I've also updated the License tag, fixed macros and
patched out a bad interpreter in one of the examples.

Locally it builds fine in F6 and F7 (i386 and x86_64) - YMMV, but I hope and
expect not ;-)

http://mfleming.fedorapeople.org/perl-Geo-IP-1.28-3.fc7.src.rpm
http://mfleming.fedorapeople.org/perl-Geo-IP.spec
Comment 11 Jason Tibbitts 2007-09-02 13:15:59 EDT
OK, now I finally have some time.

This all looks good to me now, except for one thing.  When you apply patch1, the
backup file is left in there and installed:

E: perl-Geo-IP wrong-script-interpreter
/usr/share/doc/perl-Geo-IP-1.28/example/netspeed.pl.netspeed "/usr/local/bin/perl"

It should be trivial to nuke this or to remove the "-b .netspeed" bit. 
Otherwise  everything looks good, and to avoid another round trip I'll just
approve this now and you can fix the issue when you check in.

APPROVED
Comment 12 Michael Fleming 2007-09-12 20:06:22 EDT
New Package CVS Request
=======================
Package Name: perl-Geo-IP
Short Description: Efficient Perl bindings for the GeoIP location database
Owners: mfleming
Branches: FC-6 F-7 EL-4 EL-5
Cvsextras Commits: yes
Comment 13 Kevin Fenzi 2007-09-12 23:46:55 EDT
cvs done.
Comment 14 Michael Fleming 2007-09-13 02:49:07 EDT
Imported and building (FC6/devel/EPEL-4 already successful, EPEL 5 and F7 to go..)

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