-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44855: [Python][Packaging] Use delvewheel to repair Windows wheels #35323
Conversation
|
@github-actions crossbow submit wheel-windows-cp311-amd64 |
Revision: 408852a Submitted crossbow builds: ursacomputing/crossbow @ actions-63c44cbef0
|
@github-actions crossbow submit wheel-windows-cp311-amd64 |
Revision: 17610b7 Submitted crossbow builds: ursacomputing/crossbow @ actions-41e9fa7976
|
@github-actions crossbow submit wheel-windows-cp39-amd64 |
Revision: 3dcc67c Submitted crossbow builds: ursacomputing/crossbow @ actions-732aa4049a
|
@github-actions crossbow submit wheel-windows-cp39-amd64 |
Revision: c9beb9e Submitted crossbow builds: ursacomputing/crossbow @ actions-f42319b210
|
@github-actions crossbow submit wheel-windows-cp39-amd64 |
Revision: 1d9c22f Submitted crossbow builds: ursacomputing/crossbow @ actions-ed01c3db03
|
### Rationale for this change Those labels are unnecessary once they are merged. There was a conversation on Zulip about removing them in the past. ### What changes are included in this PR? Once we merge a PR we remove labels that starts with the PR workflow prefix `awaiting`. ### Are these changes tested? I have tested the code against an old testing PR I have here: #35323 The label was removed successfully. ### Are there any user-facing changes? No * Closes: #36243 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
A different approach seems required as per #44855 |
@raulcd Can you recreate this PR? This should be the preferred approach in the short term according to #44855 (comment) |
for /f %%i in ('dir dist\pyarrow-*.whl /B') do set WHEEL_NAME=dist\%%i || exit /B 1 | ||
echo "Wheel name: %WHEEL_NAME%" | ||
delvewheel repair %WHEEL_NAME% --add-path C:\arrow\python\build\bdist.win-amd64\wheel\pyarrow -w repaired_wheels || exit /B 1 | ||
delvewheel show %WHEEL_NAME% || exit /B 1 |
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 would probably be more informative to call delvewheel show
before delvewheel repair
?
Reopened, will rebase and fix conflicts and try to get back to it |
pip install delvewheel || exit /B 1 | ||
for /f %%i in ('dir dist\pyarrow-*.whl /B') do set WHEEL_NAME=dist\%%i || exit /B 1 | ||
echo "Wheel name: %WHEEL_NAME%" | ||
delvewheel repair %WHEEL_NAME% --add-path C:\arrow\python\build\bdist.win-amd64\wheel\pyarrow -w repaired_wheels || exit /B 1 |
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.
Is the --add-path
actually necessary?
@github-actions crossbow submit wheel-windows-cp39-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
Revision: ea22e58 Submitted crossbow builds: ursacomputing/crossbow @ actions-f74951e72c
|
@github-actions crossbow submit wheel-windows* |
Revision: ea22e58 Submitted crossbow builds: ursacomputing/crossbow @ actions-40f1ed5d9d
|
Ok, I got this to work using the delvewheel changes in adang1345/delvewheel#59 |
I did a quick test of the wheels crossbow has built here and this seems to fix the issue. From reading the related issues, the only package I identified that ships On Windows 11, (x86_64), I did the following to reproduce the issue:
On the same system, I did the following to fix the issue:
I'll note the system I tested on does have a system-wide copy of |
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.
This is great! Thanks @pitrou
I tested on a Docker container without a system-wide |
@github-actions crossbow submit wheel-windows* |
Revision: 1405d04 Submitted crossbow builds: ursacomputing/crossbow @ actions-181f327725
|
@kou Would you like to take a look at this PR? |
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.
+1
We rename msvcp140.dll
and use it so that we avoid msvcp140.dll
conflict, right?
I confirmed the followings:
pyarrow-19.0.0.dev263-cp39-cp39-win_amd64.whl
includespyarrow/msvcp140-32679400f4881c06b606906b1620c877.dll
notmsvcp140.dll
pyarrow/*.dll
usemsvcp140-32679400f4881c06b606906b1620c877.dll
notmsvcp140.dll
$ unzip pyarrow-19.0.0.dev263-cp39-cp39-win_amd64.whl
$ find pyarrow -name 'msvc*.dll'
pyarrow/msvcp140-32679400f4881c06b606906b1620c877.dll
$ for x in pyarrow/*.dll; do echo $x; LANG=C objdump -p $x | grep -i msvcp; done
pyarrow/arrow.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_acero.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_dataset.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_flight.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_python.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_python_flight.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_python_parquet_encryption.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/arrow_substrait.dll
pyarrow/msvcp140-32679400f4881c06b606906b1620c877.dll
pyarrow/msvcp140-32679400f4881c06b606906b1620c877.dll: file format pei-x86-64
Name 0000000000066e46 MSVCP140.dll
(format RSDS signature 44a344afaacb486aad1c45e382407415 age 1 pdb D:\a\_work\1\s\binaries\amd64ret\bin\amd64\\msvcp140.amd64.pdb)
pyarrow/parquet.dll
DLL Name: msvcp140-32679400f4881c06b606906b1620c877.dll
Yes. |
Thanks for the reviews, let's merge now! |
Rationale for this change
We need to ship the C++ standard library with our Windows wheels, as it is not guaranteed that a recent enough version is present on the system. However, some other Python libraries may require an even more recent version than the one we ship. This may incur crashes when PyArrow is imported before such other Python library, as the older version of the C++ standard library would be used by both.
What changes are included in this PR?
Use a fixed-up version of delvewheel that allows us to name-mangle an individual DLL, and name-mangle
msvcp140.dll
to ensure that other Python libraries do not reuse the version we ship.Are these changes tested?
By regular wheel build tests.