search

astropy / pyregion

ds9 region parser for python
https://pyregion.readthedocs.io
MIT License
39 stars 38 forks source link

pxd files missing in the PyPI package #64

Closed ViviCoder closed 7 years ago

ViviCoder commented 8 years ago

In the package currently available on PyPI (v1.1.4), the two .pxd files (c_numpy.pxd and c_python.pxd) in pyregion/ are missing. It is thus not possible to generate _region_filter.c directly from _region_filter.pyx using Cython. Instead, the .c file itself is shipped.

Please ensure that for the next releases, the pxd files are shipped, and not the c file.

astrofrog commented 8 years ago

@ViviCoder - shipping the C code instead of the Cython files in the tarballs is common practice for packages in the Astropy project. We made this decision because otherwise people would need Cython to install the package.

If you are interested in tweaking the Cython files, I recommend you instead work from a clone of the repository.

ViviCoder commented 8 years ago

I am responsible for the packaging of pyregion in Debian and the Debian policy is to work from source. I understand your concern about the users, and to have everybody satisfied, I suggest you ship pxd files AND c files. Would you agree?

astrofrog commented 8 years ago

@olebole - could you comment on this request? We don't do this for astropy core right?

olebole commented 8 years ago

For Astropy, I couldn't find .pxd files. However, .pyx files are shipped with the tar distribution, and they are cythonized to re-generate the C files during the Debian build. You may check this f.e. for the 1.0.5 i386 build (search for "cythoning"). Other affiliated packages, like astroscrappy also distribute .pxd and .pyx files in their tar files (just checked because I am currently working on it :-) ). It should also not cause problems for you to just include these files -- you can leave the generated .c files there as well for convenience. Is there a reason not to ship .pxd/.pyx?

spearsem commented 8 years ago

Sorry for the late bump on the thread. I just wanted to add that it is important to ship the .pxd files because that is the way someone else can cimport from your library, if they are doing their own thing at the Cython level. It's also much more readable than digging through auto-generated C for declarations. Consider a case when using a Cython fused type ... the cdef function will result in many different type-specific variations (overloads) in the generated C source. It doesn't make sense for someone to try to reason about the code from the point of view of the auto-generated parts ... instead better to let them reason about what the developer had explicitly declared.

astrofrog commented 8 years ago

Ok - I'd be fine with including these files! A pull request to do this would be welcome :)

ViviCoder commented 8 years ago

How do you generate your tarballs?

astrofrog commented 8 years ago

We normally do python setup.py sdist - it looks like we should add a line to the MANIFEST.in to include Cython files, as done in the core astropy package: https://github.com/astropy/astropy/blob/master/MANIFEST.in#L9

ViviCoder commented 8 years ago

Done.

cdeil commented 7 years ago

I also didn't know that Cython files are included in the tarballs. I checked with Astropy and photutils, and there it's the case.

As commented here https://github.com/astropy/pyregion/pull/67#issuecomment-236500989 the Cython files are now included and will be part of the next release.

Thanks @ViviCoder !