[Bug 1155400] Review Request: pygeoip - Pure Python GeoIP API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

nicolas.vieville@xxxxxxxxxxxxxxxxxxxx changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nicolas.vieville@univ-valen
                   |                            |ciennes.fr



--- Comment #1 from nicolas.vieville@xxxxxxxxxxxxxxxxxxxx ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]