GH-45530: [Python][Packaging] Add pyarrow.libs dir to get_library_dirs #45766
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit wheel-windows-* |
|
Revision: f70dc96 Submitted crossbow builds: ursacomputing/crossbow @ actions-ba017247bc
|
python/pyarrow/__init__.py
Outdated
| pyarrow_libs_dir = _os.path.join( | ||
| python_base_install, "Lib", "site-packages", "pyarrow.libs" | ||
| ) |
There was a problem hiding this comment.
I guess the Python stdlib includes a site module that can give you the location of the current executable's site-packages dir which may be a better approach than this.
There was a problem hiding this comment.
what if we use the current folder and substitute pyarrow with pyarrow.libs?
_os.path.dirname(_os.path.abspath(__file__)) --> that's how we add it below
It feels slightly hardcoded and I am not sure if all environments (virtualenv, system installed python, conda installed python) would replicate this folder structure.
There was a problem hiding this comment.
Even better, perhaps take a look at the kind of patch generated by delvewheel itself, since it also has to compute the path to the mangled dependencies.
There was a problem hiding this comment.
Thanks!
Delvewheel uses a patch similar to what @raulcd mentions. From our patched wheel,
# start delvewheel patch
def _delvewheel_patch_1_10_0():
import os
if os.path.isdir(libs_dir := os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir, 'pyarrow.libs'))):
os.add_dll_directory(libs_dir)
I'll try this out.
There was a problem hiding this comment.
Changed in 9e2526c. Testing now.
github-actions
bot
added
awaiting committer review
|
It looks like that worked. From the raw log: |
amoeba
changed the title
github-actions
bot
added
awaiting merge
python/pyarrow/__init__.py
Outdated
| pyarrow_libs_dir = _os.path.join( | ||
| python_base_install, "Lib", "site-packages", "pyarrow.libs" | ||
| ) |
There was a problem hiding this comment.
Hmm, no, this is inflexible. Packages are indeed often installed in site-packages, but they could be elsewhere on PYTHONPATH.
I think this should really be using the pyarrow.libs directory that is next to the current package's directory.
There was a problem hiding this comment.
Changed in 9e2526c. Testing now.
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
|
@github-actions crossbow submit wheel-windows-* |
|
Revision: 9e2526c Submitted crossbow builds: ursacomputing/crossbow @ actions-472ec229f4
|
|
I pushed up a few changes to address PR feedback and this PR is now passing on crossbow and is ready for a final review. Edit: Pushed one more minor change after I wrote this. |
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bc0b858. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
To run
test_cythonon Windows, we need a copy of msvcp140.dll in the library search path.What changes are included in this PR?
pa.get_library_dirs()behavior to add thepyarrow.libsfolder to the library search pathAre these changes tested?
Not yet.
Are there any user-facing changes?
Only a bugfix.
Closes #45530