-
Notifications
You must be signed in to change notification settings - Fork 81
DOC: editable: clarify file guarantees with non-Python files #786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| and directory organization in your project, it might also expose files | ||
| that would not be normally available. | ||
| that would not be normally available. Furthermore, some files written in | ||
| other languages to Python may not be available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh? This is actually false. Where did you get the impression that only Python files are exposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you get the impression that only Python files are exposed?
I didn't get the impression that only Python files are exposed — that isn't what this says!
I did get the impression that some files in other languages may not be available: see numpy/numpy#29490.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All installed files are available, if you look them up with the proper Python API.
I really don't understand what you are trying to communicate with these edits. Most likely I don't understand what you mean with "Python file". After all, meson-python is mostly used to build packages that contain Python modules written in other languages that Python, therefore, if these sometimes were to go missing, it would be a major issue.
I guess that you are trying to access installed files by their (expected) filesystem location rather than through the Python API. A few lines above the text you are amending we have this sentence:
This will install a stub in the Python site packages directory that loads the package content from the sources and build directory.
I thought that this should be indication enough that the installed files do not exist in the filesystem at the location where they would be in an normal isntallation. But I have the impression that this is not picked up by most readers. What we really need is a "Accessing data files" in the editable install documentation that spells this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a "proper python API"?
Meson-python certainly installs an editable loader that makes import xxxx work for any xxxx that the non-editable install would provide.
It's less clear how "data files" should be accessed. For a non-editable install the answer is simple, you use importlib.resources to acquire file contents, and if you need a file tree suitable for e.g. the C compiler's -I -L arguments you try to map that to a directory.
The question is not what alternative API should be used for data files in an editable install. I'm not aware that there's an alternative answer to give.
The question is how an editable loader is expected to arrange for importlib.resources to give a correct answer. People expect specific directory structures, among other things, which may not make any sense in an editable install e.g. files that at build time are in different directories and at install time are in a single directory.
Data files are hard. Getting the import system to correctly import *.py and *.so files is much easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In the linked numpy issue, the specific topic at hand was numpy's C headers.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But also ZIP imports have much bigger issues (certainly for numpy): they don't support
import foo._c_ext.Is this to mean that ZIP imports do not support extension modules? I think they do.
https://docs.python.org/3/library/zipimport.html
This module adds the ability to import Python modules (*.py, *.pyc) and packages from ZIP-format archives. It is usually not needed to use the zipimport module explicitly; it is automatically used by the built-in import mechanism for sys.path items that are paths to ZIP archives.
Any files may be present in the ZIP archive, but importers are only invoked for .py and .pyc files. ZIP import of dynamic modules (.pyd, .so) is disallowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a restriction of the C API, really. Python can decide to support exec('string from zipfile buffer'). But extension modules use the underlying platform support for shared libraries, which needs a real file, not a memory buffer.
See also https://sourceware.org/bugzilla/show_bug.cgi?id=11767 (python is repeatedly mentioned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the linked numpy issue, the specific topic at hand was numpy's C headers.
Thinking about it, there may be a way out. If numpy were to return a list of include directories instead of a single include directory, it could list all the directories where the headers are physically located in the filesystem. This requires to keeping a list of all installed header files somewhere in a the code that returns the include directories, and assumes that the numpy headers do not use relative path includes. Then something like
def get_include_paths():
return ':'.join(set(str(importlib.resources(numpy).joinpath(header).parent) for header in HEADERS)))on the numpy side, and something like:
npinc = include_directories(run_command([py, '-c', 'include numpy; print(numpy.get_include_paths())'], capture: true).split(':'))should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, except that : is a poor choice of separator if Windows needs to be supported 🙁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could in principle work, but it's awfully hacky. And we're soon going to be able to drop the include_directories(run_command([py, '-c', 'include numpy; ... completely in favor of a simple np_dep = dependencies('numpy').
I'd much rather document "don't do that, this isn't what editable installs are for".
|
I'm personally a bit lost at synthesising the information I have read from you both, @rgommers and @dnicolodi, so I will mark this as draft until there is more which makes this click for me. |
|
I'm preparing a PR that adds to the editable installs documentation a new section about data files that expands a bit what is proposed here. |
|
Superseded by #803 |
closes numpy/numpy#29490 @rgommers