Use package_data instead of data_files in setup.py

[jdemeyer]

Depends on #21604

CC: @embray @kiwifb @mkoeppe

Component: build

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/20108

[embray]

comment:1

Indeed, looking at the setup.py, the only files that are installed using the data_files option are going into the selected site-packages (typically under SAGE_LOCAL, but in principle the appropriate site-packages for any Python that is used to run `setup.py).

This being the case it should be using package_data instead.

[embray]

comment:2

Mostly tangential, but I noticed that one of the files that is installed is ntlwrap.cpp. Not questioning that there's a good reason, but I am wondering what the background is to that. I'm searching around but if you know a direct reference for that it would be great. It certainly seems odd without context.

[jdemeyer]

comment:3

Replying to @embray:

Mostly tangential, but I noticed that one of the files that is installed is ntlwrap.cpp.

It's a file used by some Cython .pxd files. So yes it should be installed.

Of course, ideally this file shouldn't even exist in the first place: it mostly contains stuff to work around Cython limitations which are no longer relevant. Me and Jean-Pierre Flori already did some work to reduce the usage of ntlwrap.cpp, but we're not there yet.

[jdemeyer]

comment:4

I seem to remember a problem with package_data: it assumes that the directory layout of the sources and package_data is identical.

I believe that you cannot use package_data to install something from /some/random/directory/foo.dat to site-packages/sage/foo.dat. And we need this for Cython-generated files.

[embray]

comment:5

I've had that problem before--it can be worked around.

[embray]

comment:6

I guess what confuses me about ntlwrap.cpp is why the functions defined in it aren't also declared in ntlwrap.h, and reference that from the .pxd files instead. Does it have something to do with them being inline? Anyways as you say it's probably a moot point.

[jdemeyer]

Changed author from Jeroen Demeyer to none

[jdemeyer]

comment:8

Replying to @embray:

I guess what confuses me about ntlwrap.cpp is why the functions defined in it aren't also declared in ntlwrap.h, and reference that from the .pxd files instead.

Don't try to understand it. Everything about ntlwrap.cpp is the way it is only because of historical reasons.

[jdemeyer]

comment:9

Replying to @embray:

I've had that problem before--it can be worked around.

For me, the question remains: is it worth it? If we can use data_files with distutils, why not keep doing that?

[embray]

comment:10

I'm trying to bring sage the Python package up to speed with the current practices for Python packaging. The data_files issue is a red herring from my perspective. Maybe part of the problem is that you were running ./setup.py install which with setuptools builds an egg by default, and is discouraged (instead, run pip install .).

[embray]

comment:11

Just recording (without comment for now) that the only header files installed from build/cythonized (i.e. generated by use of cdef public/cdef api) are:

['sage/ext/interpreters/wrapper_cdf.h
 'sage/ext/interpreters/wrapper_el.h,
 'sage/ext/interpreters/wrapper_rr.h,
 'sage/ext/interrupt/interrupt_api.h',
 'sage/ext/interrupt/interrupt.h',
 'sage/modular/arithgroup/farey_symbol.h']

[jdemeyer]

comment:12

Replying to @embray:

the current practices for Python packaging.

I guess you can deduce from my comments that I don't always agree with those "current practices". It seems that, in many cases, they introduce more problems than they solve. And I really don't like arguments of the form "you should do X because everybody does X".

[embray]

comment:13

That attitude is why nobody wants to package Sage :) Sometimes you have to go along to get along. Plus, when it comes to the Python-specific stuff actually the "current practices" are actually much improved over where things were and are moving in the right direction. The biggest problem is just the historical baggage and the poor understanding that engenders (such as the relationships between various projects).

[jdemeyer]

comment:14

I think a problem is also that there is no clear direction from Python upstream: you have setuptools, pip and wheel which are all independent projects and distutils as part of Python and there doesn't seem to be much coordination between these. For example, the fact that wheel, distutils and setuptools all treat data_files differently is an indication of this.

[embray]

comment:15

When you install with pip it does the equivalent of python setup.py --single-version-externally-managed --record=path/to/log/of/installed/files.txt (pip then puts the install record somewhere in the package's .dist-info/ directory and uses it for uninstall). The above command doesn't work if the package isn't using setuptools, but has some interesting workarounds that inject setuptools anyways. This all happens somewhere around here. So you end up using setuptools anyways. But what pip install does is still very different from what ./setup.py install does by default which is to build an egg install.

There's been some discussion lately about how to change that. So that it just does what pip does. I actually need to pick that up again because my idea had decent support but I never made a pull request (though I do have a patch).

[jdemeyer]

comment:17

Any further suggestions? I am inclined to close this as "wontfix".

[embray]

comment:18

I obviously just haven't looked at this in a while. I would still like to though.

[jdemeyer]

comment:19

Thinking more about setup.py stuff: #21600 shows that the current way how we use data_files doesn't really work (for other reasons).

But again as suggested by #21600: maybe the best solution is to use neither data_files nor package_data but just roll our own data-file handling class.

[jdemeyer]

Changed dependencies from #20002 to #21604

[jdemeyer]

comment:20

Do you think the approach of #21600 (using neither data_files nor package_data) makes sense? If so, we could close this ticket.

[embray]

comment:21

Replying to @jdemeyer:

Do you think the approach of #21600 (using neither data_files nor package_data) makes sense? If so, we could close this ticket.

I'm not strictly against it. I think the implementation details still need some work but I'm open to the idea. I'd still rather get package_data working instead though. I'm still convinced whatever the problem is I can fix it, I just haven't tried very hard yet.

[jdemeyer]

Reviewer: Jeroen Demeyer