-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add basic mingw_x86_64 build support #1354
Add basic mingw_x86_64 build support #1354
Conversation
c43b215
to
5f08783
Compare
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 change looks easy to maintain, especially if there's a way to add it to CI.
Assuming we can sort CI, I'd suggest we factor the platform tests, as the sort of logic we need here has a tendency to decay over time...
In other words, instead of
if platform.system() == "Windows" and not self.plat_name.startswith('mingw'):
this would probably be more maintainable, assuming we write the needed functions:
if WIndows() and MinGW():
I'm not entirely sure what we should test in CI. Maybe just test that it compiles and that it can be imported? Or maybe run the whole thing but just on one version of Python... |
@JeanChristopheMorinPerso I wasn't sure what the best approach would be either. I guess we would want to pin the mingw MSYS2 version and run the unit test for its version of python. I was going to see what other projects might be doing but haven't found anything yet. It doesn't seem to be a cibuildwheel supported platform either. I'll do a little more investigation. Supporting this platform is probably not be worth the effort if there is no demand for it. |
The reason I mentioned CI is that I don't think we have expertise to maintain it on the team, so CI would be the only way we'd know if something needs addressing, unless there was an active community around the target. Since the change is mostly detecting the platform and picking a generator, I wouldn't expect it to be overwhelming to maintain as long as otioview isn't part of it, and as long as it's as Mark suggested, pinned to a version in CI. As a counterpoint, OpenEXR is an example project that is known to work with MinGW, and the community has been gracious enough to do the maintenance over the years. So perhaps a non-rigorous approach is fine, as long as we aren't making an over arching promise that it always works. This could be a good topic to raise at an upcoming TSC meeting to take the temperature? |
Yeah we can discuss it during the next TSC meeting. |
I have a rough github actions building / testing example here. Getting it integrated into otio's build matrix might still be tricky, but I'll keep chipping away at it. |
5f08783
to
49490b0
Compare
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
49490b0
to
914b863
Compare
Codecov Report
@@ Coverage Diff @@
## main #1354 +/- ##
=======================================
Coverage 86.27% 86.27%
=======================================
Files 196 196
Lines 19865 19865
Branches 2309 2309
=======================================
Hits 17138 17138
Misses 2161 2161
Partials 566 566
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Mark Reid <mindmark@gmail.com>
Your last commit looks much better @markreidvfx ! |
I'm happier with it too! The only thing I haven't figured out is how to pin the MSYS2 version. There is some discussion of being able to do that here: msys2/setup-msys2#180, but the feature has not been implemented. |
@markreidvfx does the MSYS2 pinning represent a blocking fix? Is this otherwise ready to land? Thanks! |
Pining the MSYS2 version is the only thing I haven't been able to solve. |
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.
Discussed; seems ready to land. Further improvements are possible pending community feedback.
* Add basic mingw_x86_64 build support * Add mingw64 testing to CI * Add mingw64 to py_build matrix and reduced MSYS2 dependencies Signed-off-by: Mark Reid <mindmark@gmail.com> Signed-off-by: Michele Spina <michelespina96@gmail.com>
This is the minimal amount of changes required to add mingw_x86_64 support on windows and pass all the tests.
This pull request is mainly for discussion as to if this is a platform OTIO wishes to support. I haven't looked into adding the platform to CI yet, but feels like it might be a bit of a headache to get working.
I also haven't been able to get it to build in 32-bit mode yet, just in 64-bit.
Not being able to build on mingw has been mentioned here #1342 (comment)