OpenTimelineIO icon indicating copy to clipboard operation
OpenTimelineIO copied to clipboard

Add basic mingw_x86_64 build support

Open markreidvfx opened this issue 3 years ago • 8 comments

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 https://github.com/AcademySoftwareFoundation/OpenTimelineIO/discussions/1342#discussioncomment-3088730

markreidvfx avatar Jul 15 '22 20:07 markreidvfx

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.

markreidvfx avatar Jul 15 '22 22:07 markreidvfx

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?

meshula avatar Jul 16 '22 02:07 meshula

Yeah we can discuss it during the next TSC meeting.

markreidvfx avatar Jul 17 '22 01:07 markreidvfx

I have a rough github actions building / testing example here. https://github.com/markreidvfx/otio-MinGW-w64-test

Getting it integrated into otio's build matrix might still be tricky, but I'll keep chipping away at it.

markreidvfx avatar Jul 21 '22 18:07 markreidvfx

Codecov Report

Merging #1354 (7d5b4ec) into main (913f540) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Flag Coverage Δ
py-unittests 86.27% <100.00%> (ø)

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

Impacted Files Coverage Δ
tests/test_plugin_detection.py 81.57% <ø> (ø)
tests/test_url_conversions.py 90.47% <100.00%> (ø)

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 913f540...7d5b4ec. Read the comment docs.

codecov-commenter avatar Jul 25 '22 01:07 codecov-commenter

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: https://github.com/msys2/setup-msys2/issues/180, but the feature has not been implemented.

markreidvfx avatar Jul 30 '22 20:07 markreidvfx

@markreidvfx does the MSYS2 pinning represent a blocking fix? Is this otherwise ready to land? Thanks!

ssteinbach avatar Sep 15 '22 02:09 ssteinbach

Pining the MSYS2 version is the only thing I haven't been able to solve. From what I understand MSYS2 only updates their base packages once a year. Unless you run a pacman sync command to get updates on top of that (which I don't) you get the same base version for that period of time.
I personally think that it should be stable enough to test against and if it proves to be problematic, it is pretty easy to turn off the CI.

markreidvfx avatar Sep 15 '22 06:09 markreidvfx