Opened 5 years ago
Closed 5 years ago
#20108 closed enhancement (wontfix)
Use package_data instead of data_files in setup.py
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build | Keywords: | |
Cc: | embray, fbissey, mkoeppe | Merged in: | |
Authors: | Reviewers: | Jeroen Demeyer | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #21604 | Stopgaps: |
Description
Change History (24)
comment:1 Changed 5 years ago by
comment:2 follow-up: ↓ 3 Changed 5 years ago by
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.
comment:3 in reply to: ↑ 2 Changed 5 years ago by
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.
comment:4 Changed 5 years ago by
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.
comment:5 follow-up: ↓ 9 Changed 5 years ago by
I've had that problem before--it can be worked around.
comment:6 follow-up: ↓ 8 Changed 5 years ago by
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.
comment:7 Changed 5 years ago by
comment:8 in reply to: ↑ 6 Changed 5 years ago by
Replying to embray:
I guess what confuses me about
ntlwrap.cpp
is why the functions defined in it aren't also declared inntlwrap.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.
comment:9 in reply to: ↑ 5 Changed 5 years ago by
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?
comment:10 follow-up: ↓ 12 Changed 5 years ago by
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 .
).
comment:11 Changed 5 years ago by
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']
comment:12 in reply to: ↑ 10 Changed 5 years ago by
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".
comment:13 Changed 5 years ago by
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).
comment:14 Changed 5 years ago by
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.
comment:15 Changed 5 years ago by
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).
comment:16 Changed 5 years ago by
- Cc fbissey added
comment:17 Changed 5 years ago by
Any further suggestions? I am inclined to close this as "wontfix".
comment:18 Changed 5 years ago by
I obviously just haven't looked at this in a while. I would still like to though.
comment:19 Changed 5 years ago by
- Cc mkoeppe added
- Dependencies changed from #20002 to #21604
- Milestone changed from sage-7.1 to sage-7.4
comment:20 follow-up: ↓ 21 Changed 5 years ago by
Do you think the approach of #21600 (using neither data_files
nor package_data
) makes sense? If so, we could close this ticket.
comment:21 in reply to: ↑ 20 Changed 5 years ago by
Replying to jdemeyer:
Do you think the approach of #21600 (using neither
data_files
norpackage_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.
comment:22 Changed 5 years ago by
- Milestone changed from sage-7.4 to sage-duplicate/invalid/wontfix
- Reviewers set to Jeroen Demeyer
- Status changed from new to needs_review
comment:23 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:24 Changed 5 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
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 underSAGE_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.