GH-49473: [Python] Fix get_include and get_library_dirs to work with both editable and non-editable builds#49476
GH-49473: [Python] Fix get_include and get_library_dirs to work with both editable and non-editable builds#49476raulcd wants to merge 1 commit intoapache:mainfrom
Conversation
… with both editable and non-editable installs
|
@pitrou this solves the issue but I am wondering whether it is ok for those files to be where they are at the moment. Can you validate whether it works for you (editable on conda environment?) |
|
It "solves" the issue of test_cython but it breaks the request to make an editable install. Right now it gives me a "hybrid" install, which is going to be error-prone: >>> pa.__file__
'/home/antoine/arrow/dev/python/pyarrow/__init__.py'
>>> pa.lib.__file__
'/home/antoine/mambaforge/envs/pyarrow/lib/python3.10/site-packages/pyarrow/lib.cpython-310-x86_64-linux-gnu.so'(I actually don't understand how it works at all) |
|
Looking at our build logs, I think what we're seeing is that scikit-build-core's interpretation of "editable install" is very unusual: |
|
So perhaps this PR is the right way to do it, given scikit-build-core's own strategy. Also, scikit-build-core admittedly lists editable install support as "experimental". |
|
Oh, actually it's configurable: @henryiii explains it a bit more here. Let me try with "inplace". |
|
Ok, "inplace" does not seem to work at all as nothing is copied to the editable dir, even Python extension modules such as So we should probably stick with the default editable strategy for now. |
|
Or, according to scikit-build/scikit-build-core#1141 (comment) 👍
Well, I don't know what we should later do, but this PR is obviously simpler and solves the issue for now... |
I don't really understand the above, I am not sure I understand where our CMake code should live so the cpp/so files are not moved to the site-packages folder and remain in source. |
|
Yes, I don't really understand it either. We should probably live with the current solution. |
|
I consulted with 🤖 and perhaps we should switch to |
I am unsure about that, the problem happens for editable builds where scikit-build-core seems to be pushing the files to python site-packages instead of in the source tree. As per the other change on |
Rationale for this change
With
scikit-build-coreour Cython tests fail to find PyArrow C++ headers due to get_include returning a non-existent directory.What changes are included in this PR?
Editable builds location of libraries and headers are located at ``
Previous to this change:
with this change:
Are these changes tested?
Yes, locally for editable installs and on CI for non-editable.
Are there any user-facing changes?
The only change is that headers, generated Cython cpp files and built shared libraries are installed on the virtualenv.