Declare all package data in setup.cfg instead of in setup_package.py files#9729
Conversation
astrofrog
added
the
zzz 💤 astropy-helpers-removal
| scikit-image | ||
|
|
||
| [options.package_data] | ||
| * = data/*, data/*/*, data/*/*/*, data/*/*/*/*, data/*/*/*/*/*, data/*/*/*/*/*/* |
There was a problem hiding this comment.
Who are you so wise in the way of science?!
| source = join('cextern', 'wcslib', 'C', header) | ||
| dest = join('astropy', 'wcs', 'include', 'wcslib', header) | ||
| if newer_group([source], dest, 'newer'): | ||
| shutil.copy(source, dest) |
There was a problem hiding this comment.
I need to figure out how to reproduce this behavior with setup.cfg, this is what's causing the test failure currently
There was a problem hiding this comment.
(and as a side note, the reason I didn't pick this up sooner is that the test that exercises this is set to skip if certain env variables aren't set, and this was the case with tox since env vars have to be whitelisted)
There was a problem hiding this comment.
Hmm, this looks all rather complicated; is the library created in astropy.wcs used outside of astropy?
There was a problem hiding this comment.
The idea was to make it so that other packages could create C extensions linked against the bundled WCSLIB in astropy but not sure if anyone uses it!
There was a problem hiding this comment.
I don't think we document this?
There was a problem hiding this comment.
There was a problem hiding this comment.
I see, so it is a bit like numpy.get_include(). Though indeed we should then make sure it always points to the correct include files, either that of the system or of astropy.
|
Ok so here's the weird thing - the way the code was written to copy over the WCSLIB header files was such that if astropy was compiled against a system WCSLIB library, the header files were not copied, which I guess would affect the astropy debian package. So presumably that means that third party packages that want to build extensions against the bundled version of WCSLIB would need to check if the header files are present in astropy and figure out how to link against the system library if not. I'm curious whether we know if anyone is using this. Is this something makes sense to continue supporting? @nden do you know of any cases? |
|
There is also the MANIFEST.in file, which is currently used, and provides more options to include or exclude data (e.g.
Generally I think people advice against using package-data, e.g. https://blog.ionelmc.ro/2014/06/25/python-packaging-pitfalls/#forgetting-package-data |
|
I believe drizzlepac uses those header files |
|
It looks like they are used also in modeling for wrapping the projections. |
|
Sounds like we're now in for ensuring there are links to system libraries in place... |
|
Just to clarify, I think the original idea was to provide a way for external software to link to wcslib so that system libraries are not necessary to install. I think this is the preferred way some users work. The ability to link to system libraries was added later. I believe it was requested by OS maintainers but I may be wrong. |
|
I've opened #9752 for whether |
|
Yes, let's solve one thing at a time! As is, linking with system libraries might work for most people anyway, since one usually does include a standard |
|
Regarding using MANIFEST.in and So for now, can we go ahead with this PR? If so I'll open an issue regarding switching to MANIFEST.in. |
|
Yes, let's get this in - very nice not to have to worry about |
mhvk
added
the
zzz 💤 merge-when-ci-passes
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Approving, since somehow that hadn't happened yet.
6 participants
This is a self-contained subset of changes related to the APE A roadmap for package infrastructure without astropy-helpers. This PR as-is is ready for review, and does not actually require removing astropy-helpers to work, though I think we should maybe wait until Tuesday to merge since that's when we'll be discussing the APE in Socorro.
This moves the declaration of package data from
get_package_datainsetup_package.pyfiles to instead be in thesetup.cfgfile. The documentation updates are in #9726 and are difficult to disentangle from other documentation updates, so let's keep that separate.