Opened 3 years ago

Closed 3 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 3 years ago by embray

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.

comment:2 follow-up: Changed 3 years ago by embray

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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: Changed 3 years ago by embray

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

comment:6 follow-up: Changed 3 years ago by 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. Does it have something to do with them being inline? Anyways as you say it's probably a moot point.

comment:7 Changed 3 years ago by jdemeyer

  • Authors Jeroen Demeyer deleted

comment:8 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

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.

comment:9 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

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: Changed 3 years ago by embray

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 3 years ago by embray

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 3 years ago by jdemeyer

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 3 years ago by embray

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 3 years ago by jdemeyer

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 3 years ago by embray

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 3 years ago by fbissey

  • Cc fbissey added

comment:17 Changed 3 years ago by jdemeyer

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

comment:18 Changed 3 years ago by embray

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

comment:19 Changed 3 years ago by jdemeyer

  • Cc mkoeppe added
  • Dependencies changed from #20002 to #21604
  • Milestone changed from sage-7.1 to sage-7.4

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.

comment:20 follow-up: Changed 3 years ago by jdemeyer

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 3 years ago by embray

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.

comment:22 Changed 3 years ago by jdemeyer

  • 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 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:24 Changed 3 years ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.