joe-jordan / pyvoro

2D and 3D Voronoi tessellations: a python entry point for the voro++ library
Other
106 stars 26 forks source link

Remove cb from setup.py #6

Closed ansobolev closed 10 years ago

ansobolev commented 10 years ago

The pull request removes cb.py dependency from setup.py, making it available to build pyvoro using built-in features of python setuptools. This way pyvoro can be distributed in a variety of ways, including python eggs. Moreover, now it can be installed using pip (with pip install -e).

joe-jordan commented 10 years ago

This is a very good idea; cb.py makes development easier, but isn't really required for end users (this project has served as a beta test of cb.py long enough, I think,) thank you for implementing it. On the implementation, several things need to be changed before I will accept it.

Finally, not quite a prerequisite to acceptance, but please justify why we're calling the "make" shell command. Are you invoking the original voro++ Makefile? Why are we not just linking all the .o files in one step, rather than creating a .a and then linking against it? Finally, please confirm that your implementation doesn't install voro++.a into a user's system folders to link against, as this is not the point of having a python interface!

ansobolev commented 10 years ago

You are perfectly right about the validation of Cython version (from the other side, why anyone would use Cython prior to 0.15 in their production run, anyway?). I added assertion for having Cython, as well as having right version of Cython in setup.py and tested it against Cython 0.14 and 0.20.2, and it seems to work fine. Also I added a few words in readme about how to install dependencies automatically using pip, hope that'll be enough. Moreover, you can get rid of Cython dependency if you choose to distribute cpp file instead of pyx, because Cython is used only to produce cpp from pyx.

As for calling make, I thought that having to add every single file to the sources line will mess up the sources line of pyx file header, so I chose to make library first, and then link against it. But it seems that we can move sources declaration to setup.py and skip the step of building a library, actually making everything clean and neat.

joe-jordan commented 10 years ago

Remove cython dependency: I suppose we could, but I'm not in the habit (unless I've done so here accidentally) of commiting autogenerated code - the Cython is the source, and anyone who wants to edit it needs to change that, not the generated cpp (which, AFAIK, is basically unreadable anyway).

Re: including all the source files in setup.py - look at the source of voro++.cc :) - maybe setup.py can be simplified again!

ansobolev commented 10 years ago

Remove cython dependency: I meant not giving cpp file out instead of Cython source, but along with Cython source. This is actually the recommended way of distibuting cython modules, see the 1st sentence in http://docs.cython.org/src/reference/compilation.html#distributing-cython-modules. After that you can put the package on PyPI (well, it's worth doing even now, but without dumb depenencies it'll be much better).

Include all files - wow, that's nice! I used provided makefile to get source files, and didn't even open sources themselves - it seems that I ought to :)

joe-jordan commented 10 years ago

OK, thanks very much for your continued work on this. If you would like to eliminate the Cython dependency using a recent, stable version to commit a CPP file, alongside the pyx, and modify setup.py to use that, then by all means do. I'll accept the request this evening (UK time) in any case, when I've got some time to double check everything (and update the version on my computers).

If you would also like to explain/point me at instructions for adding this project to pypi then please do - I wouldn't know where to start to get software into a semi-official package manager like that. Thanks! I will update the project version to 1.3 as part of tidying all this up, and add a git tag (in case that helps with pypi.)

One last thing, you asked who on earth would be using an ancient cython version - the answer is people who install cython using apt-get et al, particularly on old versions of linux (packages are not updated expect for security by default on ubuntu, for instance.) Eliminating the Cython dependency makes this problem go away for people stuck on an old machine. Note that lots of people insterested in Voronoi tessellations are, like me, academics - and they haven't heard of a phrase like "production environment" :)

joe-jordan commented 10 years ago

I have found the PyPI instructions here, so don't worry about that part.

ansobolev commented 10 years ago

Not a problem, I just want to see the package on PyPI - I write a small project that uses pyvoro, and it's way easier to list prerequisites if they are on PyPI. Thanks for the package, by the way :) As for instructions, there is this very nice tutorial, which guides through every step ofthe process.

joe-jordan commented 10 years ago

project now live on PyPI - enjoy!