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

Add basic mingw_x86_64 build support #1354

Merged

Conversation

markreidvfx
Copy link
Contributor

@markreidvfx markreidvfx commented Jul 15, 2022

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)

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.

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():

@JeanChristopheMorinPerso
Copy link
Member

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...

@markreidvfx
Copy link
Contributor Author

markreidvfx commented Jul 15, 2022

@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.

@meshula
Copy link
Collaborator

meshula commented Jul 16, 2022

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?

@markreidvfx
Copy link
Contributor Author

Yeah we can discuss it during the next TSC meeting.

@markreidvfx
Copy link
Contributor Author

markreidvfx commented Jul 21, 2022

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.

Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

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.

Signed-off-by: Mark Reid <mindmark@gmail.com>
@JeanChristopheMorinPerso
Copy link
Member

Your last commit looks much better @markreidvfx !

@markreidvfx
Copy link
Contributor Author

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.

@ssteinbach
Copy link
Collaborator

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

@markreidvfx
Copy link
Contributor Author

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.

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.

Discussed; seems ready to land. Further improvements are possible pending community feedback.

@meshula meshula merged commit 8b430a6 into AcademySoftwareFoundation:main Sep 15, 2022
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 19, 2022
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants