Bug 806040 - Review Request: pyproj - a python module that performs cartographic transformations and geodetic computations
Review Request: pyproj - a python module that performs cartographic transform...
Status: CLOSED DUPLICATE of bug 855528
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
:
Depends On:
Blocks: 806037
  Show dependency treegraph
 
Reported: 2012-03-22 13:59 EDT by jdekloe
Modified: 2013-10-19 10:42 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-09-08 12:24:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description jdekloe 2012-03-22 13:59:51 EDT
Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.0-1.fc17.src.rpm
Description: 
A python module that performs cartographic transformations and geodetic computations.
This is a dependency for the pygrib python module, see https://bugzilla.redhat.com/show_bug.cgi?id=806037

The Proj class can convert from geographic (longitude,latitude) to native map
projection (x,y) coordinates and vice versa, or from one map projection
coordinate system directly to another.

The Geod class can perform forward and inverse geodetic, or Great Circle,
computations. The forward computation involves determining latitude, longitude
and back azimuth of a terminus point given the latitude and longitude of an
initial point, plus azimuth and distance. The inverse computation involves
determining the forward and back azimuths and distance given the latitudes
and longitudes of an initial and terminus point.

Input coordinates can be given as python arrays, lists/tuples, scalars or
numpy/Numeric/numarray arrays. Optimized for objects that support the Python
buffer protocol (regular python and numpy array objects).
Comment 1 Volker Fröhlich 2012-04-26 17:23:35 EDT
There are two basic problems here:

- pyproj bundles a copy of Proj and uses that
- C code should be generated with our version of Cython

I recently started a draft for this package as well. I'll publish it.

I can't be a sponsor though.

Are you aware of http://fedoraproject.org/wiki/GIS ?
Comment 2 Volker Fröhlich 2012-04-27 06:15:09 EDT
http://www.geofrogger.net/review/pyproj-1.9.0-1.fc16.src.rpm

Please notice this is a draft package and has several things left to do, also mentioned in the spec file. But it should address the major issues I pointed out.
Comment 3 Volker Fröhlich 2012-04-29 11:36:49 EDT
Also consider to change the summary. I think it is not very accurate, because it disguises, it actually uses Proj.
Comment 4 jdekloe 2012-04-29 12:49:22 EDT
Thanks for pointing out that there is a packaging problem here. Obviously I had not yet noticed that.

I am too new to this packaging to be aware of why cython should be used for building, in stead of default python. Any pointers to documentation/explanation would be appreciated (the word cython is never even mentioned on http://fedoraproject.org/wiki/Packaging:Python).

I have no problem with you submitting your spec file for inclusion in Fedora, if it builds the package. My only motive to add it was that it is a requirement of the pygrib package (and this is one I really am eager to see become part of Fedora).

Anyway, if you wish I will contact the original author of this pyproj module, and ask him if he can making packaging a bit easier by more clearly specifying which files he copied from external projects. At least I could not find clear explanations in the pyproj documentation on this issue.

Best regards.
Comment 5 Volker Fröhlich 2012-04-29 13:31:22 EDT
Cython generates C-code from Cython code. The .pyx is the Cython code file, used as the source to generate the .c file. In my opinion, it is a good idea to use Fedora's version of Cython to create the C-code. It is probably not required though.

Anyways, Cython is not exactly used for building and it does not replace Python by any means. It just generates C-code, which is then compiled with a C-compiler.

http://cython.org

I own around 20 packages by now. I'm not keen to own as many packages as possible. Please carry on! :)

I don't think there is anything worth reporting to the developer. His work is dependent on Proj and we have to keep track of which version works with which, on our own, anyway. I don't see a problem with him bundling it. Same goes for the C file, where he reliefs his users from having Cython installed. I'd be fine with how it is and how we can work around it.
Comment 6 jdekloe 2012-05-31 16:57:20 EDT
Thanks again for your comments and explanations.

I corresponded to the upstream author and he was kind enough to provide an alternative setup file that allows to build the module using a preinstalled proj version.

He also explained some of his reasons to include the proj sources. In fact these are not plain copies, but have been patched in various places. Most of these patches have been submitted to (and accepted by) upstream, but not all. He writes:

"pyproj includes a patched copy of the Proj source code in the 'src'
directory.  I could provide you with a modified version of setup.py that
builds against the installed libproj, but it would be missing some
features (an inverse for the Hammer projection for one)."

and:

"The hammer patch is probably the only one that hasn't been included -
but I did submit a patch to them over a year ago."

Anyway, using the modified setup.py (for the time being it is called setup-proj.py) and a modified spec file, I could build the module succesfully using mock for the rawhide target. Rawhide was needed because as you know pyproj depends on proj-v4.8.0 and this is not (yet) available in Fedora 16 or 17.

The new spec and srpm files are available here:
Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.0-2.fc16.r298.src.rpm

>Are you aware of http://fedoraproject.org/wiki/GIS ?
no, this is new for me. Thanks for this pointer.
Comment 7 jdekloe 2012-06-02 17:41:27 EDT
Ofcourse the srpm for rawhide should have been this one:
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.0-2.fc18.r298.src.rpm
Comment 8 Volker Fröhlich 2012-06-03 04:59:44 EDT
Great!

The disttag should be the last part of release:

2.r298%{?dist}

To make it a tiny bit more comfortable for you, you could define a macro and use that:

%global svn r298

2.%{svn}%{?dist}
Source: ...%{svn}...

Don't use the macro in the changelog though, but explictly write down the proper release there. That is 1.9.0-2.r298 in this case.

If you are interested, you can still build Pyproj 1.8.9 on Fedora 16 and 17.

You can use this approach to avoid providing the private libs:

%{?filter_setup:
%filter_provides_in %{python_sitearch}.*\.so$
%filter_setup
}

Taken from http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering

The SRPMs are the same, no matter if they have a fc16 or fc18 disttag.

In case you don't know how to build the Python 3 module yet, it is described on http://fedoraproject.org/wiki/Packaging:Python.

If you're looking for inspiration, you can also take a look at:

http://pkgs.fedoraproject.org/gitweb/?p=pyshp.git;a=blob_plain;f=pyshp.spec;hb=9e99ead3811736584eadf547c1496c5e3ec9a0b4
Comment 9 jdekloe 2012-06-07 15:07:39 EDT
Thanks again for your help.
Updated spec and srpm files are available here:
Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.0-2.fc16.r298.src.rpm

Before trying a python3 version I will first invest some time trying to do a standalone build for python3, using 2to3. The package itself does not provide python3 build in its setup or a script to allow conversion to python3, so still some work needed there.
Comment 10 jdekloe 2012-06-07 15:10:43 EDT
sorry, the previous comment has a small typo.
The 2nd link should again have been:
http://jdekloe.nl/Fedora/pyproj-1.9.0-2.fc18.r298.src.rpm
Comment 11 Volker Fröhlich 2012-06-11 02:29:31 EDT
Your SRPM and spec file don't match.

Please bump the release number on every version you publish. This makes life easier for reviewers.

Also, please use the name and version macro in Source0.

Why did you put in numpy as a BR?
Comment 12 jdekloe 2012-06-15 18:02:23 EDT
here is my next attempt:

Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.0-3r298.fc16.src.rpm

Sorry for not bumping the release number. It was not entirely clear to me that this was required for each modified spec file (since it is not yet in the fedora repository).

Source0 has been changed the way you suggested.

numpy was added as BR because the setup-proj.py script imports it. It calls the 
numpy.get_include()] function to retrieve an include dir needed during the build.

Finally, as you suggested in comment 8, I tried to include the code needed to do a python3 build as well. This was after all a trivial thing, and no 2to3 calls where needed.

On my machine this seems to work well using mock -r fedora-rawhide-x86_64 
as well as using koji.

The next thing I would like to look at is doing some manual tests (i.e. running python, importing the module, doing some calculations) for the created rpm's. Do you now how to easily do this for these rawhide rpm's on my fedora 16 installation?
Comment 13 Volker Fröhlich 2012-06-19 13:20:13 EDT
Please add a period between "3" and "%{svn}. By doing so, it also matches what you have in the changelog.

I noticed that setup.py states version 1.9.2. I assume this does not cause any harm.

Concerning numpy: While it is advertised in the description, it actually seems to be commented out in the actual code. I wonder why is that.

The permissions are 775 because CCACHE_UMASK is set to 002 in mock for some reason. Please correct the permissions with a chmod in the install section.

Python 3 looks fine.

I suggest to use "%{_usr}/" instead of plain "/usr/" for PROJ_DIR. You forgot to export the variable, therefore it doesn't do anything right now.

Ad testing: I could think of the following way: Install the finished package into the existing Mock chroot and then shell into it. I think that should do it.

Please see http://fedoraproject.org/wiki/Projects/Mock for details.
Comment 14 jdekloe 2012-06-21 06:05:20 EDT
New spec and srpm files are here:

Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.2-4.r298.fc16.src.rpm

The dot has been added as suggested, and I changed the version to 1.9.2 as mentioned in the setup file.

numpy is listed as a build requirement, needed by and used in setup-proj.py. It's not used in setup.py. It's also not clear to me why it is needed during the build, since it is not used in the pyx file. I'll ask the upstream developer about this.

As you say numpy is mentioned and commented in the __init__.py file. The module may return numpy.ndarray's as result (if it gets them as input), but this is optional and standard python lists, tuples or scalars as input work equally well. Therefore I guess during runtime numpy is optional and not really required.

The chmod to correct the permission, the "%{_usr}/" macro and its export have been added.

As for testing. After some experimenting the mock tool indeed is suitable for this. It does need manual addition of the proj and proj-nad packages before the testing actually works.
With this method I could verify that both the python and the python3 package can be imported and used as intended.

Thanks to this testing I found that I needed to add proj-nad as requirement. That one contains datafiles needed during use of the module. If not present the module will not load.
Comment 15 jdekloe 2012-06-21 06:09:50 EDT
A small addition.
During testing I also noticed that "Requires:  proj-nad" works fine for the python package. It refuses to install the pyproj rpm in case proj-nad is absent.
However, the python3 rpm installs without complaining, even is proj-nad is not installed. Is an additional python3 require statement needed to handle this?
Comment 16 Jason Tibbitts 2012-07-03 20:25:12 EDT
I wish I could answer your question above but unfortunately I've no idea at all.  However, a couple of comments:

Your package does not meant the guidelines for snapshot versions.  Please see http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages
I don't know if your snapshot is pre- or post-release, but regardless, it must include at least the snapshot date and "svn".  If you wish to include the subversion revieson number, you may.  The usual scheme for a post-release snapshot would be 1.9.2-4.20120621svn298.

When I see TODO and such over a spec I have to wonder if it's ready for review.  If you have questions about what you should do, feel free to ask them on an appropriate mailing list.  Then make your changes and present the spec that you'd like reviewed.
Comment 17 jdekloe 2012-07-12 17:19:39 EDT
Thanks for your comments.
I have added the download date to the release version as requested.

As for the TODO comments:

#TODO: Consider Sphinx for documentation

this was an idea added by Volker Fröhlich in his original rpm spec file. I don't know Sphinx from own experience, but from what I read the output seems to be nicer formatted. However, documentation generated by epydoc is already present in the source tree, so for me this means the module is in a usable state, since it includes api documentation. Therefore I removed this TODO comment and would suggest to pass the request to use sphinx to the upstream author for consideration.

#TODO: pyproj.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/pyproj/_proj.so 0775L

this problem has been fixed now, so I removed this TODO as well.

Finally, the #TODO in the check section is more a reminder that the source code does not include a proper test script. It is not a TODO to be included in the spec file, so I turned it into a normal comment. Also adding a proper test script should be suggested to the upstream author. If you agree I am willing to do that. In the meantime I hope this is no reason to keep this module from inclusion in Fedora.

New spec and srpm files are here:

Spec URL: http://jdekloe.nl/Fedora/pyproj.spec
SRPM URL: http://jdekloe.nl/Fedora/pyproj-1.9.2-5.20120712svn300.fc16.src.rpm
Comment 18 jdekloe 2012-09-08 12:24:51 EDT
moved to my other bugzilla account

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

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