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.

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

Create wheels on Windows #1015

Conversation

JeanChristopheMorinPerso
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Jul 18, 2021

Fixes #667 .

Summary

This PR makes it possible to build wheels on Windows in GitHub Actions.

I also updated cibuildwheel from 1.10.0 to 1.12.0 (the last version compatible with Python 2.7) and I'm now using the GitHub Action pypa/cibuildwheel@v1.12.0 instead of installing cibuildwheel manually.

Notes to testers

Wheels have been uploaded to test.pypi.org and can be installed using:

python -m pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple opentimelineio==0.14.0dev1

where python is the interpreter you want to install the package into.

We would be extremely happy if we can get some people to test them, specially the Blender community woking on Windows. You can comment in this PR if you find any issues and you can even comment if it works :) Thanks a lot in advance to everyne that will test them!

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2021

Codecov Report

Merging #1015 (8962272) into master (5957ddc) will decrease coverage by 6.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
- Coverage   85.55%   79.30%   -6.26%     
==========================================
  Files         192      113      -79     
  Lines       18317     6102   -12215     
  Branches     2056        0    -2056     
==========================================
- Hits        15672     4839   -10833     
+ Misses       2119     1263     -856     
+ Partials      526        0     -526     
Flag Coverage Δ
unittests 79.30% <ø> (-6.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/opentimelineio/imageSequenceReference.h 44.44% <0.00%> (-24.31%) ⬇️
...eio/opentimelineio-bindings/otio_anyDictionary.cpp 92.85% <0.00%> (-7.15%) ⬇️
src/opentime/stringPrintf.h 60.00% <0.00%> (-6.67%) ⬇️
...elineio/opentimelineio-bindings/otio_anyVector.cpp 93.33% <0.00%> (-6.67%) ⬇️
...lineio/opentime-bindings/opentime_rationalTime.cpp 90.69% <0.00%> (-5.20%) ⬇️
...ntimelineio/opentimelineio-bindings/otio_tests.cpp 72.30% <0.00%> (-3.89%) ⬇️
src/opentimelineio/serializableCollection.cpp 72.97% <0.00%> (-2.79%) ⬇️
src/opentimelineio/composition.cpp 76.38% <0.00%> (-2.62%) ⬇️
...imelineio/opentime-bindings/opentime_timeRange.cpp 93.33% <0.00%> (-1.75%) ⬇️
src/opentime/rationalTime.h 86.04% <0.00%> (-1.19%) ⬇️
... and 220 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5957ddc...8962272. Read the comment docs.

@JeanChristopheMorinPerso JeanChristopheMorinPerso marked this pull request as ready for review July 18, 2021 17:57
Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the submission!

@reinecke
Copy link
Collaborator

reinecke commented Jul 28, 2021

Uploaded .whl files from the "wheels` artifact zip from this build to the OpenTimelineIO test PyPi.

Users can test installing using the command: python -m pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple opentimelineio==0.14.0dev1

Note: We aren't generating a source dist yet (see #988), and I explicitly didn't create one so we'll get an explicit callback if there is a platform mismatch for users.

@JeanChristopheMorinPerso
Copy link
Member Author

JeanChristopheMorinPerso commented Jul 30, 2021

I think I found an issue with Python 2.7 wheels on Windows. They link against VCRUNTIME140_1.dll because of Visual Studio 2019. Python 3.5+ includes that DLL while Python 2.7 don't. I'm trying what's described in https://cibuildwheel.readthedocs.io/en/stable/faq/#importerror-dll-load-failed-the-specific-module-could-not-be-found-error-on-windows and will report back.

@JeanChristopheMorinPerso
Copy link
Member Author

Windows is hard 😞 I am able to get rid of the dependency to VCRUNTIME140_1.dll but I'm still left with VCRUNTIME140.dll (notice the difference between 140_1 and 140) and MSVCP140.dll

I can import OTIO when VCRUNTIME140.dll is installed but as soon as it's not there, 💥 .

From what I read in https://stevedower.id.au/blog/building-for-python-3-5-part-two (Steve Dower a CPython core dev and works at Microsoft) distutils was modified so that it copies the DLL if it's missing in the interpreter and then it's packaged in the wheel. Though we don't use setuptools/distutils built-in compilation stuff. I haven't figured out how to do that manually. So far I've tried simply copying the DLLs manually but that didn't work.

Note that when I build OpenTImelineIO with Pytho 2.7 on Windows and then try to import it, everything works. But when I take it to another VM that doesn't have the build stuff installed, then it doesn't. I do this to ensure that I install OTIO in a clean environment.

I would be more than happy if someone has more experience than me with how DLLs are searched and loaded by Windows.

The fact that I can import OTIO when VCRUNTIME140.dll is installed means we can mix different runtimes since Python 2.7 downloaded from https://python.org is compiled with a much older Visual Studio (2008 to be precised). That Python links against MSVCR90.dll which seems to be part of Windows by default (at least Windows Server 2019). Someone would need to confirm for Windows 10.

So we either have to find how we can ship the DLL within our wheel or statically link against it (like mentioned in Steve's blog post). One odd thing is that when debugging, I found that we MSBuild compiles with the /MD flag instead of the /MT. /MD is for dynamic link while /MT is for static. Let me know if you find this odd too.

@JeanChristopheMorinPerso
Copy link
Member Author

JeanChristopheMorinPerso commented Jul 31, 2021

Ah, I think DLLs can be loaded if they are found in PATH. I'll take an hour or two to think about it and will propose something if it works.

@JeanChristopheMorinPerso
Copy link
Member Author

Good news, I was able to force CMake to tell MSBuild to statically link VCRUNTIME140.dll and MSVCP140.dll and it works great from what I see. I can now create a Python 2.7 Windows wheel and install it on a freshly installed Windows instance.

And the solution isn't too hacky:

diff --git a/src/py-opentimelineio/CMakeLists.txt b/src/py-opentimelineio/CMakeLists.txt
index 01cfeb2..279b450 100644
--- a/src/py-opentimelineio/CMakeLists.txt
+++ b/src/py-opentimelineio/CMakeLists.txt
@@ -1,2 +1,14 @@
+if(MSVC AND Python_VERSION_MAJOR VERSION_LESS 3)
+    # Statically link run-time library (vcruntime and msvcp)
+    # See https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=msvc-160
+    # This allows us to compile OTIO bindings with a newer MSVC version
+    # than the one used by the interpreter where OTIO will be installed.
+    # This is only required for Python < 3 because only these are
+    # compiled with an older compiler (9.0). CPython 3.5+ uses at least
+    # Visual C++ 14.X.
+    # See https://wiki.python.org/moin/WindowsCompilers#Which_Microsoft_Visual_C.2B-.2B-_compiler_to_use_with_a_specific_Python_version_.3F
+    add_compile_options(/MT)
+endif()
+
 add_subdirectory(opentime-bindings)
 add_subdirectory(opentimelineio-bindings)

With this change, the generated _opentime.pyd and _otio.pyd look like this:

C:\Users\jeanchristophemorin1\src\OpenTimelineIO>dumpbin /dependents build\lib.win-amd64-2.7\opentimelineio\_otio.pyd
Microsoft (R) COFF/PE Dumper Version 14.29.30040.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file build\lib.win-amd64-2.7\opentimelineio\_otio.pyd

File Type: DLL

  Image has the following dependencies:

    python27.dll
    KERNEL32.dll

  Summary

        9000 .data
        A000 .pdata
       4F000 .rdata
        2000 .reloc
        1000 .rsrc
       DF000 .text
        1000 _RDATA

while before it was looking like:

C:\Users\jeanchristophemorin1\src\OpenTimelineIO>dumpbin /dependents build_1\lib.win-amd64-2.7\opentimelineio\_otio.pyd
Microsoft (R) COFF/PE Dumper Version 14.29.30040.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file build_1\lib.win-amd64-2.7\opentimelineio\_otio.pyd

File Type: DLL

  Image has the following dependencies:

    python27.dll
    MSVCP140.dll
    VCRUNTIME140_1.dll
    VCRUNTIME140.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-heap-l1-1-0.dll
    api-ms-win-crt-runtime-l1-1-0.dll
    api-ms-win-crt-string-l1-1-0.dll
    api-ms-win-crt-convert-l1-1-0.dll
    api-ms-win-crt-filesystem-l1-1-0.dll
    api-ms-win-crt-math-l1-1-0.dll
    KERNEL32.dll

  Summary

        7000 .data
        7000 .pdata
       23000 .rdata
        1000 .reloc
        1000 .rsrc
       B2000 .text

How does that sound?

(I can now close 50 Chrome tabs 😛)

@JeanChristopheMorinPerso
Copy link
Member Author

I just pushed the necessary change. Note that I still have to test the other Windows wheels.

@JeanChristopheMorinPerso
Copy link
Member Author

I tested the Python 2.7 Windows wheels and they seem to work. I also tested the Python 3.7 wheels on Windows (just to be sure).

  • Python 3.7 64 bits wheel tested in Blender 3.7 (with and without the UI) and I also tested with Python downloaded from python.org.
  • Python 2.7 32 and 64 bits wheels tested on Python downloaded from python.org.

In the best of worlds, I would have tested the 2.7 wheels on DCCs but I don't have any DCCs at hands right now. The idea is I wanted to test that statically linking the Windows runtimes would not interfere with other runtimes (the ones used by the DCCs, etc).
Also note that I didn't particularly focus on blender. It's just that Blender is free and as I just said, I don't have any licenses available to test any other DCCs.

The latest wheels can be found here: https://github.com/PixarAnimationStudios/OpenTimelineIO/suites/3504415054/artifacts/83528771.

@jminor jminor added this to Reviewer approved in WG: Build Improvements Aug 19, 2021
@ssteinbach ssteinbach added this to the Public Beta 14 milestone Aug 19, 2021
@JeanChristopheMorinPerso
Copy link
Member Author

JeanChristopheMorinPerso commented Sep 24, 2021

I tested the x64 Windows wheels for all supported Python versions (2.7, 3.7 and 3.8) and they all work perfectly! This PR is officially ready to be merged. 🥳 🍾 .

@meshula meshula merged commit 1ba6d78 into AcademySoftwareFoundation:master Sep 24, 2021
WG: Build Improvements automation moved this from Reviewer approved to Done Sep 24, 2021
@JeanChristopheMorinPerso JeanChristopheMorinPerso deleted the windows_wheels branch September 24, 2021 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Can't install with pip (on windows, in blender)
5 participants