Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Repairing Windows wheels #459

Closed
myselfhimself opened this issue Nov 6, 2020 · 33 comments
Closed

Repairing Windows wheels #459

myselfhimself opened this issue Nov 6, 2020 · 33 comments

Comments

@myselfhimself
Copy link
Contributor

myselfhimself commented Nov 6, 2020 •
edited

Hello,
I am attempting to build a wheel for a C++ Python binding of G'MIC with .dll dependents of a wheel file on Windows MSYS2 with Github Actions.
This is for python 3.8 for now. Once I have understood how to "repair" the wheel by including the dependent .dll files into the wheel file, I will proceed to use cibuildwheel most probably. My goal is to iterate on Python versions and use my MSYS2 gcc/g++ and bash commands instead of using Visual Studio.
I will give a count of my progress and struggles using cibuildwheel here. I am already using the latter for MacOS Clang builds (though disabled again for now).

Related to #329 and myselfhimself/gmic-py#5 as well as this gmic-py windows-support pull-request
@myselfhimself
Copy link
Contributor Author

One thing which I have not yet understood, is if cibuildwheel already has a default windows repair command in place, or if it does nothing for now, but just has the slot environment variable for injecting such a repair command.

My idea for repairing under msys2 is to run ldd on the compiled C++/Python .dll and copy all matching /mingw64/bin dll files into a directory, which then would be included within the wheel and if needed loaded again at module import time.

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 6, 2020 •
edited

I have read a bit through related https://github.com/joerick/cibuildwheel/blob/master/cibuildwheel/windows.py and https://github.com/joerick/cibuildwheel/blob/master/examples/github-minimal.yml already.
What is unclear for someone not reading the source is that the tool used for grabbing various Python versions for cross-version building is not MSYS2's pacman, not chocolatey, but nuget. This could be advertised in the documentation for the Windows platform support section (unless I am mistaken).

@mayeut
Copy link
Member

mayeut commented Nov 7, 2020

You might want to have a look at https://discuss.python.org/t/delocate-auditwheel-but-for-windows/2589

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 7, 2020 via email

@myselfhimself myselfhimself mentioned this issue Nov 7, 2020
Open
@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 7, 2020 via email

@YannickJadoul
Copy link
Member

One thing which I have not yet understood, is if cibuildwheel already has a default windows repair command in place, or if it does nothing for now, but just has the slot environment variable for injecting such a repair command.

The latter; see https://cibuildwheel.readthedocs.io/en/stable/options/#repair-wheel-command

@vinayak-mehta
Copy link

vinayak-mehta commented Nov 8, 2020 •
edited

One issue is that this script is not yet a separate python package for example, it is in the middle of a pdf2png project.

It's not a separate Python package yet because of limitations I mention in vinayak-mehta/pdftopng#3 (comment) I'm open to any feedback you all have about how I could improve it and possibly create a package out of it!

I'll keep an eye out on this issue as I'm interested in learning about other ways to build Windows wheels! (since @myselfhimself mentioned about using MSYS2 instead VS)

The latter; see https://cibuildwheel.readthedocs.io/en/stable/options/#repair-wheel-command

Yes! I'm using CIBW_REPAIR_WHEEL_COMMAND_WINDOWS to call that script on Windows.

@mayeut @YannickJadoul @joerick and other contributors, thank you for cibuildwheel!

@myselfhimself
Copy link
Contributor Author

One thing which I have not yet understood, is if cibuildwheel already has a default windows repair command in place, or if it does nothing for now, but just has the slot environment variable for injecting such a repair command.

The latter; see https://cibuildwheel.readthedocs.io/en/stable/options/#repair-wheel-command

Thank you @YannickJadoul for this documentation pointer. So obviously cibuildwheel has no default windows repair wheel command at this time, and its default for the CIBW_REPAIR_WHEEL_COMMAND_WINDOWS environment variable is ''.

@YannickJadoul
Copy link
Member

its default for the CIBW_REPAIR_WHEEL_COMMAND_WINDOWS environment variable is ''

Yep. There's no official PyPA tool either, so I don't think we'll add that before there's an officially supported tool. But you have the possibility to use your own.

@myselfhimself
Copy link
Contributor Author

@vinayak-mehta thank you again for posting clear links to related cibuildwheel files in your project and explaining your usage.

As stated on your png2pdf's issue tracker just now, both your wheel repair .bat or .py commands would be useful or maybe an extension less python command, as under MSYS2, .py or .sh/.bash files are more linux-like than .bat, although all of those are executable (MSYS2 deals with .dll and .exe natively also).

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 9, 2020 •
edited

its default for the CIBW_REPAIR_WHEEL_COMMAND_WINDOWS environment variable is ''

Yep. There's no official PyPA tool either, so I don't think we'll add that before there's an officially supported tool. But you have the possibility to use your own.

@vinayak-mehta mentioned here that he sees 2 drawbacks before making his script onto pypa. I think this is really minor. So Vinayak, feel now encouraged twice by @YannickJadoul and me, to release it as a stand alone project, really!! Although this would mean extra maintainer work in the future for you, though it is pure-python, Python 3.x compatible at wide possible and for one OS only.

@myselfhimself myselfhimself changed the title Msys2 gcc/g++ x cibuildwheel Msys2 gcc/g++ x cibuildwheel / Windows wheels repairing Nov 9, 2020
@YannickJadoul
Copy link
Member

YannickJadoul commented Nov 9, 2020 •
edited

Does this mean we can consider your issue solved? (at least the part where this is related to cibuildwheel)

@myselfhimself
Copy link
Contributor Author

https://github.com/vinayak-mehta/pdftopng/blob/355282f57760f0d494b4a36fa5a83ea097cad75b/.github/workflows/build.yml#L16 is a proof that cibuildwheel x wheel repairing x msvc works. If you like to have a clean tracker, then OK, this could be closed. @vinayak-mehta or I could come and reopen the issue or create a new one, when I have msys2 news or he/someone has independent windows wheel repair executable news.

@myselfhimself
Copy link
Contributor Author

I have fixed wheel_repair.py for the .dll case (was working for .pyd-based projects already), within the gmic-py project only for now.

@vinayak-mehta would you agree, when you have time for it, to create an independent repository for this project, where I co-contribute or make PRs? We could slowly go towards a PyPA module from there, and have it referenced and used by cibuildwheel.

@YannickJadoul
Copy link
Member

Just curious: is this mingw/msys-specific? Or would it work for all Windows wheels?
I still thought mingw isn't supported by Python, though, since Python is built with MSVC?

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 10, 2020 via email

@joerick
Copy link
Contributor

joerick commented Nov 10, 2020

This all sounds very promising! The world of Windows DLLs is somewhat foreign to me, so I can't comment on the technical aspects but I'd be happy to include such a Windows repair command in cibuildwheel, perhaps after a trial period where we recommend it in the documentation.

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Nov 11, 2020 via email •
edited

@Czaki
Copy link
Contributor

Czaki commented Nov 11, 2020

I am trying to understand how cibuildwheel cross-platform compatible projects need to be configured generally before wheel build time and wheel repair time (any kind of setup.py code to keep track of third party shared libraries or some rootlevel directory for shared libraries)...

For Linux and MacOS I need to properly set LD_LIBRARY_PATH and DYLD_FALLBACK_LIBRARY_PATH to be possible to find all external libraries than auditwheel or deallocate collect all dependencies to hidden directory inside wheel (with flat structure)

You could see here my code for build imagecodecs package on Linux and MacOS. Version on Windows was built by the package author.
https://github.com/Czaki/imagecodecs_build/blob/master/azure-pipelines.yaml

@YannickJadoul
Copy link
Member

I would like to see this get officially endorsed by PyPA, though, before cibuildwheel starts offering it by default (potentially linking to it and giving an example how this can be used with CIBW_REPAIR_WHEEL_COMMAND_WINDOWS, before that is the case).
It's great if this will work, but I personally don't really feel like getting mixed up in discussions about Windows wheels, before there's consensus on what to do :-)

@myselfhimself
Copy link
Contributor Author

I am dropping the msys2 idea for now... it seems like a python executable compiled with msvc will not import any of the mingw32 python .dll module I compile.

@joerick joerick changed the title Msys2 gcc/g++ x cibuildwheel / Windows wheels repairing Repairing Windows wheels Nov 12, 2020
@YannickJadoul YannickJadoul mentioned this issue Nov 18, 2020
Closed
@diego-plan9 diego-plan9 mentioned this issue Dec 7, 2020
Merged
@joerick
Copy link
Contributor

joerick commented Jan 15, 2021

I happened to come across this today... looks very new but quite interesting :) https://pypi.org/project/delvewheel/ https://github.com/adang1345/delvewheel @adang1345

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Jan 17, 2021 via email

@Czaki
Copy link
Contributor

Czaki commented Feb 2, 2021

I test delvewheel for build python-snappy. It looks like it works nice, but have two limitations:

  • need at least python 3.6, so approach from delocate (install in each environment) could not be used because of python 2.7 and 3.5 .
  • need to be executed from same architecture as fixed wheel. (32 bits python interpreter for 32 bits wheel).

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Feb 2, 2021 via email

@Czaki Czaki mentioned this issue Feb 4, 2021
Closed
@joerick
Copy link
Contributor

joerick commented Mar 12, 2021

@minrk mentioned that pyzmq uses delvewheel on a Python Bytes episode. I wonder if it's time to consider making it our default?

Pyzmq use it like:

      CIBW_REPAIR_WHEEL_COMMAND_WINDOWS: >-
        delvewheel repair
        -v
        --add-path=C:/libzmq-dll
        --no-dll=vcruntime140.dll
        --wheel-dir={dest_dir}
        {wheel}

Which is actually requiring a few extra options that auditwheel or delocate don't need. So maybe that's an argument against a delvewheel default. I admit, I know very little about Windows dynamic linking, but I'd be curious why delvewheel requires options that delocate and auditwheel do not.

@adang1345
Copy link

delvewheel developer here. To answer your question, delvewheel can still run without these extra options; these options are there to allow package maintainers to have greater control over the DLLs that are included/excluded and where to find them. Documentation for all the options are at https://github.com/adang1345/delvewheel/blob/master/README.md.

@joerick
Copy link
Contributor

joerick commented Mar 13, 2021

thanks for chiming in @adang1345! So the goal and the operating principle is very similar to auditwheel then. Would something like delvewheel repair -w {dest_dir} {wheel} work as a default REPAIR_COMMAND do you think?

More broadly, do you think the project is ready for more widespread usage from cibuildwheel users? It does seem to be pretty solid, but I note that the version is still v0.0.11! :)

@myselfhimself myselfhimself mentioned this issue Mar 13, 2021
Open
@adang1345
Copy link

@joerick delvewheel repair -w {dest_dir} {wheel} would work as a default REPAIR_COMMAND.

Regarding the readiness for widespread usage, I'm not sure. delvewheel is a very new project and thus has undergone only limited testing in the wild. If you were to use delvewheel in cibuildwheel, then I think it should start out as an opt-in experimental feature at first.

@henryiii
Copy link
Contributor

henryiii commented Mar 13, 2021 •
edited

What about making it a suggestion in the docs for now? REPAIR_COMMAND_WIN: "delvewheel repair -w {dest_dir} {wheel}" or such? (And including it in the default requirements)

@myselfhimself
Copy link
Contributor Author

myselfhimself commented Mar 13, 2021 via email

@YannickJadoul
Copy link
Member

What about making it a suggestion in the docs for now? REPAIR_COMMAND_WIN: "delvewheel repair -w {dest_dir} {wheel}" or such? (And including it in the default requirements)

I think this makes sense, yes. Adding it by default and expecting users to notice it's experimental by reading the docs might be slightly unrealistic?

@joerick
Copy link
Contributor

joerick commented Apr 30, 2021 •
edited

I've added a note about delvewheel in the documentation. Let's revisit this in a few months and think about adding it as a default.

@arcondello arcondello mentioned this issue Nov 26, 2021
Closed
@flit flit mentioned this issue Feb 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@joerick @myselfhimself @Czaki @vinayak-mehta @henryiii @YannickJadoul @mayeut @adang1345