Hello,

As a candidate packager for Fedora (I need a sponsor), I can make an unofficial review of your package if you don't mind.

In order to make it more easy using fedora-review, there's a couple of things I would modify in your spec file. I'm not a proven Fedora Python packager, so if one of them wants to make any comment, they are welcome.

First, in order to reflect Fedora packaging rules, you should rename your spec file to match the name of the package according to the specific rules dedicated to python modules (https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/NamingGuidelines#Addon_Packages_.28python_modules.29).
It should be set as python-pygeoip.spec.

Then, to follow this recommendation the packages names should be python-pygeoip and python3-pygeoip in the spec file.

If we look at http://pkgs.fedoraproject.org/cgit/python-setuptools.git/tree/python-setuptools.spec file cited as an example in https://fedoraproject.org/wiki/Packaging:Python#Building_more_than_once we can see that the first test macro is: 

%if 0%{?fedora}

The release version of Fedora is not evaluated.

In each section BuildRequires for the python2 package and the python3 package, you should add:

BuildRequires: python-nose
BuildRequires: python-tox
BuildRequires: curl
BuildRequires: tar

in order to be able to proceed with the test provided upstream (see below about the %check section).

As the upstream developer provides some tests, the packaging guide invite you to use them. Here there's no makefile in order to achieve them. The only solution I would use (proven python packager are welcome about that) is to make them manually by adding a %check section as this one for example (taken and adapted from the makefile available on the github but not on the pipy URL):

%check
rm -rf maxmind-geoip-samples.tar.gz tests/data
mkdir -p tests/data
curl -s https://www.defunct.cc/maxmind-geoip-samples.tar.gz | tar -zx -C tests
# Test with the only available python env in Fedora
sed -i -e 's/\(envlist = \)\(.*$\)/\1py27,py34/g' tox.ini
tox

%if 0%{?with_python3}
pushd %{py3dir}
rm -rf maxmind-geoip-samples.tar.gz tests/data
mkdir -p tests/data
curl -s https://www.defunct.cc/maxmind-geoip-samples.tar.gz | tar -zx -C tests
# Test with the only available python env in Fedora
sed -i -e 's/\(envlist = \)\(.*$\)/\1py27,py34/g' tox.ini
tox
popd
%endif # with_python3

Here, I'm not sure if it is necessary to proceed with the tests on the two packages (python2 and python3 - confirmation needed).
The sed command replaces the value of the envlist variable in the tox.ini file to reflect the real python env available in Fedora (e.g. ptyhon2.7 and python3.4). As the tests dependencies were correctly set in the BuildRequires sections curl, tar and tox (python-tox) are available.

In each %files section, it could be possible to simplify the directives beginning with %{python2_sitelib} (respectively %{python3_sitelib}) by only one:

%{python2_sitelib}/*

respectively

%{python3_sitelib}/*

While testing the packages build with such a modified spec file with rpmlint, it states that "hostname" in the description is not the correct spelling:

spelling-error %description -l en_US hostname -> host name, host-name, hostage

Don't know if it should be corrected (real correct spelling against real usage in IT).

I'll do an unofficial review using fedora-review tool, once you proposed a new spec file containing what I proposed or what proven python packagers will indicate to do.

Feel free to ask for any help if needed.

Cordially,


-- 
NVieville

Hello,

Commenting my previous message.

According to https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 the 2 lines:

BuildRequires: tar

should be removed from the spec file.

Cordially,


-- 
NVieville

Spec URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-pygeoip.spec
SRPM URL: https://williamjmorenor.fedorapeople.org/rpmdev/python-pygeoip-0.3.1-1.fc20.src.rpm

Hello NVieville

Very nice feedback but package must build without downloading anything during build.

Think the %%test is not a blocker but this will need a pacth to include the test files.

Created attachment 951665 [details]
Patch proposition for the spec file

Hello,

You are right. I've missed that.

It could be possible to include the test archive as a source, but 18,139,571 bytes are probably too much (need more doc searching here). I'll probably try to search about that in the next few days, but if on your side you find something interesting about this subject, I really appreciate you post the links you find.

As a proposition, I provided a patch against your spec file including all the "glitches" I noticed previously. Feel free to use it or only pick what you think interesting in it.

I think it should be worth verifying that python-sphinx (respectively python3-sphinx) included in the BuildRequires sections of your new spec file is (are) really useful. I'm probably wrong, but I thought that sphinx was used to build some documentation, and as you Source0 points to https://pypi.python.org and this archive doesn't contain any docs folder, may be you should drop these BuildRequires. If you choose to take in account as a Source0 file, the provided archive on github, and as this one includes a docs directory needing sphinx in its Makefile, then this dependency should be kept. 
But in this case, you should probably have to split this documentation in a separate python-pygeoip-doc.noarch package from your spec file.

Hope this help.

Any comment are welcome.

I'll let you know in the next few days if I find something about the first point of this message.

Cordially,


-- 
NVieville

There are some docs files in the master git repo, I will make a patch to include these docs in the SRPM so we can make some docs with python-sphinx

https://github.com/appliedsec/pygeoip/tree/master/docs

Also the git repo have some test file, this files are already in the tarball

https://github.com/appliedsec/pygeoip/tree/master/tests

So I think It could be better to work with this test files, I really think than make a 17 MB patch in a 28.0 KB tarball not make sense.

Hello William,

If you want to make the git repo the source of this package, some modifications on your spec file are needed.

I would suggest that you modify this line:

%global pypi_name pygeoip

to:

%global git_name pygeoip
%global commit 7501c9327fc80e78be3f44585395e30141d8c749
%global shortcommit %(c=%{commit}; echo ${c:0:7})

Then, in order to take this in account, you should modify this line:

Source0:           https://pypi.python.org/packages/source/p/%{pypi_name}/%{pypi_name}-%{version}.tar.gz

to:

Source0:            https://github.com/appliedsec/pygeoip/archive/%{commit}/%{git_name}-%{commit}.tar.gz

The pypi_name macro have to be renamed git_name in the entire spec file too.

the setup line in the %prep section have to be modify from:

%setup -q -n %{pypi_name}-%{version}

to:

%setup -qn %{git_name}-%{commit}

As you want to include the provided documentation, and as this one is build with sphinx, then you should keep the required packages in the BuildRequires.

But sphinx provides a static bundled jquery.js file. So one have to add:

Provides:           bundled(jquery)

in each part describing the separates docs packages. See here for some explanations:

https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Packages_granted_temporary_exceptions

Rpmlint continues to warn about wrong-file-end-of-line-encoding for this file, but this seems to be okay as it is a one-line optimized jscript file.

In order to make it easier for you to apply these modifications, I will attach a patch against your last spec file containing all the suggestion I made. Feel free to use any part of it you think useful.

Don't hesitate to ask me for any help if needed.

Cordially,


-- 
NVieville

Created attachment 951896 [details]
Second patch proposition for the spec file

Hello William,

Any news about your project and the review of this package?

Cordially,


-- 
NVieville

Hello, I have not updated this package, this is a dependency for another app than I would like to package into RPM, it is ERP Next, but there will make a new release soon (I guees so), so I am waiting the reales of v5 for return to this project.

By the way, did you find a sponsor? Maybe i can help you to find one, in the other hand I have some packaqes in review but I did not find a reviewer yet, maybe you can help me to review those packages please.

Regards.

William

Is this a duplicate of https://bugzilla.redhat.com/show_bug.cgi?id=910235 ?

Yes it is, is the same package :-/

Sorry I didn't notice it sooner :(  I found this ticket when going to branch python-pygeoip for epel7.

Closing as duplicate of 910235.

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