Bug 1052060 - Review Request: ip2location - IP to location library
Review Request: ip2location - IP to location library
Status: CLOSED DUPLICATE of bug 1081434
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
NotReady
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-13 04:05 EST by Chris Lim
Modified: 2015-07-21 08:47 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1081434 (view as bug list)
Environment:
Last Closed: 2014-03-27 10:16:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
ip2location src rpm with fixes to comments from Ralf Corsepius. (463.04 KB, application/x-redhat-package-manager)
2014-03-18 11:45 EDT, Guruswamy Basavaiah
no flags Details

  None (edit)
Description Chris Lim 2014-01-13 04:05:52 EST
Spec URL: http://www.ip2location.com/rpm/SPEC
SRPM URL: http://www.ip2location.com/rpm/ip2location-c-6.0.2-2.i386.rpm
Description: IP2Location library enables the user to find the country, region, city, coordinates, zip code, time zone, ISP, domain name, connection type, area code, weather, MCC, MNC, mobile brand name, elevation and usage type for any IP address.
Fedora Account System Username: chrislim2888
Comment 2 Chris Lim 2014-01-16 00:51:25 EST
(In reply to Michael Schwendt from comment #1)
> Please point at a valid spec file and src.rpm.
> 
> * https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
> * https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
> * https://fedoraproject.org/wiki/Package_Review_Process
> * https://fedoraproject.org/wiki/Category:Package_Maintainers

Sorry for my mistake, and I have rectified the issues.

Please find the respective files at
Spec URL: http://www.ip2location.com/rpm/ip2location-c_6.0.2.spec
SRPM URL: http://www.ip2location.com/rpm/ip2location-c-6.0.2-1.fc20.src.rpm

Please review again.
Comment 3 Ralf Corsepius 2014-01-16 01:51:43 EST
(In reply to Chris Lim from comment #2)
> Please review again.
I see your URL:'s are pointing to upstream - So are you upstream?

Some remarks on your package:

* Source0:-URL should be an URL pointing to upstream 
 (Source0: www.ip2location.com/downloads/ip2location-c_6.0.2.tar.gz)

* The *.spec file name does not match Fedora's conventions
 (Must be ip2location-c.spec, not ...-<version>. spec)

* The upstream tarball is a mess.
- It contains binaries and temporary files which should not be part of a source-tarball.
  In case you're upstream, I'd recommend you to build tarballs with "make dist", which should automatically take care about this issue.
  In case you're not upstream, remove all of these in %prep.

- Many files carry bogus permissions.
  In case you're upstream, please fix them.
  In case you're not please fix them in %prep.

* libIP2Location.so isn't properly versioned (keyword: SONAME).
  In case you're upstream, please change this.
  In case you're not, please add an SONAME 0.0.0 to the package.

* Your spec is trying to build and ship a static libs.
  Please append --disable-static to %configure to suppress this behavior.

* Please split your package into a *-devel and a run-time package.

* You are trying to ship a *.la. This is not allowed in Fedora.
  Please rm -f this file in %install

* Your spec doesn't honor multilibs correctly (Carries hard-coded refs to /usr/lib). Replace references to /usr/lib with %{_libdir}.

* Supplying a file named /usr/include/imath.h seems a pretty bad idea to me, because the name seems too generic to me.
  I'd recommend upstream to either use a more unique/less likely to conflict name. Alternatively, a Fedora packager could install all of this package's headers into a subdir of /usr/include 
(e.g. /usr/include/IP2Loc; %configure --includedir=%{_includedir}/IP2Loc)
Comment 4 Christopher Meng 2014-02-11 19:35:44 EST
News here?

This is a very nice alternative to geoip.
Comment 5 Chris Lim 2014-02-11 20:57:08 EST
I'm currently working on the comments from Ralf. Shall release an update once completed.
Comment 6 Guruswamy Basavaiah 2014-03-13 02:35:46 EDT
Hello, 
 I am currently working on the comments from Ralf. Could this bug be assigned to me.
Comment 7 Christopher Meng 2014-03-13 03:11:00 EDT
(In reply to guru2018 from comment #6)
> Hello, 
>  I am currently working on the comments from Ralf. Could this bug be
> assigned to me.

Who are you?
Comment 8 Guruswamy Basavaiah 2014-03-13 03:36:30 EDT
Hello,
 I am one of developer involved in the development of ip2location library.
Comment 9 Christopher Meng 2014-03-13 04:38:38 EDT
(In reply to guru2018 from comment #8)
> Hello,
>  I am one of developer involved in the development of ip2location library.

Ok! Hope you can continue the packaging soon. ;)
Comment 10 Guruswamy Basavaiah 2014-03-18 11:42:22 EDT
Hello  Christopher Meng,
 All the comment from you are addressed expected one and the package srpm is attached. 
 
 Comment which is not fixed is "Please split your package into a *-devel and a run-time package." 
 Instead single -devel package is created. ip2location provide only the libraries and headers for the application, hence no run time package is created. 

 Looking forward for any comments you have. 

Guru
Comment 11 Guruswamy Basavaiah 2014-03-18 11:45:15 EDT
Created attachment 876010 [details]
ip2location src rpm with fixes to comments from  Ralf Corsepius.
Comment 12 Michael Schwendt 2014-03-20 04:27:50 EDT
> ip2location provide only the libraries and headers for the application,
> hence no run time package is created. 

This is not acceptable.

The shared library is the runtime part, the headers and libIP2Location.so are the build-time part.


That's not the only packaging mistake, however. There are more, such as hardcoded /usr/lib, use of %attr for ordinary +x (prefer %install and fixing the upstream install code), missing ldconfig scriptlets, missing licensing files, incorrect license tag, license clarification needed. Just to mention a few.


Please keep the "Spec URL:" and "SRPM URL:" lines in this ticket up-to-date, then run "fedora-review -b 1052060" for some helpful reviewing checks.
Comment 13 Christopher Meng 2014-03-20 23:41:13 EDT
(In reply to guru2018 from comment #8)
> Hello,
>  I am one of developer involved in the development of ip2location library.

Where is Chris Lim?

Who will be the final submitter(packager)?
Comment 14 Chris Lim 2014-03-21 02:03:47 EDT
Guru2018 and I are from the same team of ip2location development. guru2018 is taking over the task for the fixing and fine-tuning.
Comment 15 Christopher Meng 2014-03-21 02:17:31 EDT
(In reply to Chris Lim from comment #14)
> Guru2018 and I are from the same team of ip2location development. guru2018
> is taking over the task for the fixing and fine-tuning.

Understand now.

But here are my advices:

1. One being as the submitter should continue the work.

2. If the original submitter has some reasons that block him from continuing the job, let the people coming after create a new bug and mark this as duplicate.

3. Follow:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

For people coming from one team/company, independent acccounts are needed, please don't use your teammate's account to update the package.

Now I'm concerned about the new SRPM. ;)

Thanks.
Comment 16 Guruswamy Basavaiah 2014-03-27 09:42:21 EDT
Hello Michael Schwendt,
 Yes, we should create 2 RPM for this. I am working on it and rest of your comments.  

 New bug is reported, and this is marked as clone. 
 new bug, 
 https://bugzilla.redhat.com/show_bug.cgi?id=1081434

Guru
Comment 17 Christopher Meng 2014-03-27 10:14:59 EDT
*** Bug 1081434 has been marked as a duplicate of this bug. ***
Comment 18 Christopher Meng 2014-03-27 10:16:22 EDT

*** This bug has been marked as a duplicate of bug 1081434 ***

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