Declare all package data in setup.cfg instead of in setup_package.py files by astrofrog · Pull Request #9729 · astropy/astropy

astrofrog · 2019-12-06T16:28:37Z

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_data in setup_package.py files to instead be in the setup.cfg file. The documentation updates are in #9726 and are difficult to disentangle from other documentation updates, so let's keep that separate.

…files

pllim · 2019-12-06T16:35:12Z

setup.cfg

    scikit-image

+[options.package_data]
+* = data/*, data/*/*, data/*/*/*, data/*/*/*/*, data/*/*/*/*/*, data/*/*/*/*/*/*


Who are you so wise in the way of science?!

astrofrog · 2019-12-06T17:18:39Z

astropy/wcs/setup_package.py

-            source = join('cextern', 'wcslib', 'C', header)
-            dest = join('astropy', 'wcs', 'include', 'wcslib', header)
-            if newer_group([source], dest, 'newer'):
-                shutil.copy(source, dest)


I need to figure out how to reproduce this behavior with setup.cfg, this is what's causing the test failure currently

(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)

Hmm, this looks all rather complicated; is the library created in astropy.wcs used outside of astropy?

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!

I don't think we document this?

Ah we do somewhat: http://docs.astropy.org/en/stable/api/astropy.wcs.get_include.html#astropy.wcs.get_include

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.

astrofrog · 2019-12-07T04:02:36Z

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?

saimn · 2019-12-10T03:14:59Z

There is also the MANIFEST.in file, which is currently used, and provides more options to include or exclude data (e.g. graft astropy could probably replace the recursive globbing ?).
https://setuptools.readthedocs.io/en/latest/setuptools.html#including-data-files
https://python-packaging.readthedocs.io/en/latest/non-code-files.html

~~Also of interest : package_data is only used when building binary packages. But I'm not sure if this is (still) true.~~ edit: as confirmed by @astrofrog this is no longer true.

Generally I think people advice against using package-data, e.g. https://blog.ionelmc.ro/2014/06/25/python-packaging-pitfalls/#forgetting-package-data

nden · 2019-12-10T03:57:56Z

I believe drizzlepac uses those header files
https://github.com/spacetelescope/drizzlepac/blob/master/setup.py#L39

nden · 2019-12-10T04:07:50Z

It looks like they are used also in modeling for wrapping the projections.

mhvk · 2019-12-10T14:29:17Z

Sounds like we're now in for ensuring there are links to system libraries in place...

nden · 2019-12-10T15:20:41Z

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.

astrofrog · 2019-12-10T15:21:13Z

I've opened #9752 for whether get_include should return system headers in some cases (I think that's beyond the scope of this PR)

mhvk · 2019-12-10T16:27:20Z

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 -I/usr/include etc.
And certainly the current approach seems right: if we provide a library to link against, we should also provide its header.

astrofrog · 2019-12-18T13:09:48Z

Regarding using MANIFEST.in and include_package_data, I don't have a strong opinion on this, and the setuptools documentation gives both this and the current approach as options. In the interest of moving this forward, since this currently works as-is, can we consider merging this and then making the switch to MANIFEST.in in a separate issue/PR? (my priority is to get rid of unnecessary setup_package.py files). Note that the comment about package_data only working for binary packages is no longer, true, what we do here is strictly equivalent to MANIFEST.in.

So for now, can we go ahead with this PR? If so I'll open an issue regarding switching to MANIFEST.in.

mhvk · 2019-12-18T13:24:15Z

Yes, let's get this in - very nice not to have to worry about setup_package any more!

mhvk

Approving, since somehow that hadn't happened yet.

astrofrog · 2019-12-18T14:18:05Z

I'm going to go ahead and merge this since it was approved - and APE 17 has been merged. I've opened a new issue to discuss whether to consider switching to using MANIFEST.in: #9801

Declare all package data in setup.cfg instead of in setup_package.py …

9c1c4fe

…files

astrofrog added Refactoring no-changelog-entry-needed labels Dec 6, 2019

astrofrog added this to the v4.1 milestone Dec 6, 2019

astropy-bot bot added the wcs label Dec 6, 2019

astrofrog mentioned this pull request Dec 6, 2019

Refactor infrastructure as described in astropy-helpers removal APE #9726

Merged

12 tasks

astrofrog added the zzz 💤 astropy-helpers-removal archived: PRs and issues related to the removal of astropy-helpers label Dec 6, 2019

pllim reviewed Dec 6, 2019

View reviewed changes

astrofrog added the Ready-for-final-review label Dec 6, 2019

astrofrog commented Dec 6, 2019

View reviewed changes

Add back copying of WCSLIB header files

a455a33

astrofrog mentioned this pull request Dec 10, 2019

astropy.wcs.get_include() incorrect when astropy compiled against system libaries #9752

Open

mhvk added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Dec 18, 2019

mhvk approved these changes Dec 18, 2019

View reviewed changes

astrofrog mentioned this pull request Dec 18, 2019

Consider declaring package data in MANIFEST.in #9801

Closed

astrofrog merged commit 59f8ab4 into astropy:master Dec 18, 2019

Uh oh!

Conversation

astrofrog commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 7, 2019

Uh oh!

saimn commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nden commented Dec 10, 2019

Uh oh!

nden commented Dec 10, 2019

Uh oh!

mhvk commented Dec 10, 2019

Uh oh!

nden commented Dec 10, 2019

Uh oh!

astrofrog commented Dec 10, 2019

Uh oh!

mhvk commented Dec 10, 2019

Uh oh!

astrofrog commented Dec 18, 2019

Uh oh!

mhvk commented Dec 18, 2019

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astrofrog commented Dec 6, 2019 •

edited

Loading

saimn commented Dec 10, 2019 •

edited

Loading