add windows and mac builds to ci #873

ssteinbach · 2021-01-21T23:21:54Z

Enables GitHub actions CI build/tests for windows and MacOS.

CI builds for each OS and each python version (so you will now see 9 checks in the GitHub actions interface instead of 3).
Sets Linux/3.7 as this is the only platform that computes coverage and submits coverage reports.
Windows file handling required changing how some unit tests were working. This was solved by adopting tempfile.TemporaryDirectory (native on python3, from backports.tempfile on python2).
adds backports.tempfile as a dependency on python2 builds
the autogen plugins script will conform paths to unix-formatting as well as being relative to the project root, to maintain consistent output across all three operating systems
one of the test files, premiere_generators.xml had a "BOM" (Byte Order Mark: https://en.wikipedia.org/wiki/Byte_order_mark) that was tripping up on windows/3.7 runs. This was removed from the file. I'm not really clear on whether this is supposed to be supported or not, but since it was an outlier, I stripped it out for now.
Hit an odd error in the XGES adapter on windows. It looks like that is only supported on linux, so I turned the tests off on non-linux platforms and for python 2.

- seemed like contains didn't do what I expected, but I suspect the environment variable wasn't being read

codecov-io · 2021-02-13T02:49:53Z

Codecov Report

Merging #873 (215de62) into master (546b333) will decrease coverage by 0.51%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
- Coverage   86.63%   86.12%   -0.52%     
==========================================
  Files         183      183              
  Lines       17874    17707     -167     
  Branches     1971     1973       +2     
==========================================
- Hits        15486    15250     -236     
- Misses       1904     1959      +55     
- Partials      484      498      +14

Flag	Coverage Δ
unittests	`86.12% <91.11%> (-0.45%)`	⬇️

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

Impacted Files	Coverage Δ
tests/test_hooks_plugins.py	`95.74% <77.77%> (-2.01%)`	⬇️
tests/test_builtin_adapters.py	`92.15% <81.81%> (-4.14%)`	⬇️
tests/test_adapter_plugin.py	`89.65% <85.71%> (-1.58%)`	⬇️
tests/test_console.py	`93.66% <86.66%> (-2.32%)`	⬇️
tests/test_cmx_3600_adapter.py	`98.89% <90.47%> (-0.55%)`	⬇️
...ontrib/adapters/tests/test_hls_playlist_adapter.py	`98.16% <96.29%> (-0.91%)`	⬇️
...ineio_contrib/adapters/tests/tests_xges_adapter.py	`90.67% <100.00%> (-1.17%)`	⬇️
...timelineio/console/autogen_plugin_documentation.py	`72.72% <100.00%> (+0.45%)`	⬆️
tests/test_serialized_schema.py	`100.00% <100.00%> (ø)`
tests/test_plugin_detection.py	`84.31% <0.00%> (-9.81%)`	⬇️
... and 36 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 546b333...215de62. Read the comment docs.

ssteinbach · 2021-02-17T17:30:20Z

One of the major issues with the tests on windows has to do with the use of NamedTemporaryFile -- windows doesn't want those to be reopened. In the long run, I think we could add a variation of write_to_file that would be write_to_file_handle or something for handling files that are already open(), but for now I think fixing the unit tests is fine. It just means we need to manually clean them the tempfiles.

- windows is stricter about using file handles that are already open, so switching to a pattern where TemporaryDirectory is used to create things should be safer. Its also cleaner than the mkdtemp/try/except pattern that had been used in the tests before.

- by swapping all the paths out the logic that was isolating certain manifests was skipping _all_ the manifests

meshula · 2021-02-18T21:14:40Z

tests/sample_data/premiere_generators.xml

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="UTF-8"?>
+<?xml version="1.0" encoding="UTF-8"?>


it's a shame the BOM doesn't show in the diff

yeah! oddly enough git diff from the command line did show it to me

thiblahute · 2021-02-18T22:35:57Z

Xes tests are supposed to only work on linux though it was where they were developed. Where can I see the errors so I can try and properly fix?

ssteinbach · 2021-02-18T22:38:19Z

Xes tests are supposed to only work on linux though it was where they were developed. Where can I see the errors so I can try and properly fix?

https://github.com/PixarAnimationStudios/OpenTimelineIO/pull/873/checks?sha=85c958cd5739adadd9e935f919e2bdcdd5f58956

The error was:

28
ERROR: test_GESMarker_color_to_otio_color (tests_xges_adapter.AdaptersXGESTest)
29
----------------------------------------------------------------------
30
Traceback (most recent call last):
31
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 2184, in test_GESMarker_color_to_otio_color
32
    marker.set_color_from_otio_color(otio_color)
33
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3568, in set_color_from_otio_color
34
    self.set_color_from_argb(self.OTIO_COLOR_TO_ARGB[otio_color])
35
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3530, in set_color_from_argb
36
    _wrong_type_for_arg(argb, "int", "argb")
37
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
38
    "".format(expect_type_name, arg_name, type(val).__name__))
39
TypeError: Expect a int type for the 'argb' argument. Received a long type.
40

41
======================================================================
42
ERROR: test_GESMarker_colors (tests_xges_adapter.AdaptersXGESTest)
43
----------------------------------------------------------------------
44
Traceback (most recent call last):
45
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 2169, in test_GESMarker_colors
46
    marker.set_color_from_argb(argb)
47
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3530, in set_color_from_argb
48
    _wrong_type_for_arg(argb, "int", "argb")
49
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
50
    "".format(expect_type_name, arg_name, type(val).__name__))
51
TypeError: Expect a int type for the 'argb' argument. Received a long type.
52

53
======================================================================
54
ERROR: test_markers (tests_xges_adapter.AdaptersXGESTest)
55
----------------------------------------------------------------------
56
Traceback (most recent call last):
57
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 1387, in test_markers
58
    _make_ges_marker(23, MarkerColor.RED),
59
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 112, in _make_ges_marker
60
    ges_marker = SCHEMA.GESMarker(position * GST_SECOND, metadatas)
61
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio\core\__init__.py", line 62, in __init__
62
    orig_init(self, *args, **kwargs)
63
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3514, in __init__
64
    _wrong_type_for_arg(position, "int", "position")
65
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
66
    "".format(expect_type_name, arg_name, type(val).__name__))
67
TypeError: Expect a int type for the 'position' argument. Received a long type.
68

ssteinbach · 2021-02-18T22:46:09Z

@thiblahute would you object to me checking in this PR as is? If you'd like to follow up with a fix for windows support in a further PR, that might be easier for you to iterate with.

hwilkes-igalia · 2021-02-19T11:14:14Z

Xes tests are supposed to only work on linux though it was where they were developed. Where can I see the errors so I can try and properly fix?

https://github.com/PixarAnimationStudios/OpenTimelineIO/pull/873/checks?sha=85c958cd5739adadd9e935f919e2bdcdd5f58956

The error was:

28
ERROR: test_GESMarker_color_to_otio_color (tests_xges_adapter.AdaptersXGESTest)
29
----------------------------------------------------------------------
30
Traceback (most recent call last):
31
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 2184, in test_GESMarker_color_to_otio_color
32
    marker.set_color_from_otio_color(otio_color)
33
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3568, in set_color_from_otio_color
34
    self.set_color_from_argb(self.OTIO_COLOR_TO_ARGB[otio_color])
35
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3530, in set_color_from_argb
36
    _wrong_type_for_arg(argb, "int", "argb")
37
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
38
    "".format(expect_type_name, arg_name, type(val).__name__))
39
TypeError: Expect a int type for the 'argb' argument. Received a long type.
40

41
======================================================================
42
ERROR: test_GESMarker_colors (tests_xges_adapter.AdaptersXGESTest)
43
----------------------------------------------------------------------
44
Traceback (most recent call last):
45
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 2169, in test_GESMarker_colors
46
    marker.set_color_from_argb(argb)
47
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3530, in set_color_from_argb
48
    _wrong_type_for_arg(argb, "int", "argb")
49
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
50
    "".format(expect_type_name, arg_name, type(val).__name__))
51
TypeError: Expect a int type for the 'argb' argument. Received a long type.
52

53
======================================================================
54
ERROR: test_markers (tests_xges_adapter.AdaptersXGESTest)
55
----------------------------------------------------------------------
56
Traceback (most recent call last):
57
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 1387, in test_markers
58
    _make_ges_marker(23, MarkerColor.RED),
59
  File "D:\a\OpenTimelineIO\OpenTimelineIO\contrib\opentimelineio_contrib\adapters\tests\tests_xges_adapter.py", line 112, in _make_ges_marker
60
    ges_marker = SCHEMA.GESMarker(position * GST_SECOND, metadatas)
61
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio\core\__init__.py", line 62, in __init__
62
    orig_init(self, *args, **kwargs)
63
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 3514, in __init__
64
    _wrong_type_for_arg(position, "int", "position")
65
  File "c:\hostedtoolcache\windows\python\2.7.18\x64\lib\site-packages\opentimelineio_contrib\adapters\xges.py", line 120, in _wrong_type_for_arg
66
    "".format(expect_type_name, arg_name, type(val).__name__))
67
TypeError: Expect a int type for the 'position' argument. Received a long type.
68

I think this is only a problem with python 2, since it is related to mixing the long and int types (python3 only uses int).

In the GESMarker __init__ method, there is meant to be a fix for this for the position argument under python 2 (https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/contrib/opentimelineio_contrib/adapters/xges.py#L3506):

        if type(position) is not int:
            # TODO: remove below once python2 has ended
            # currently in python2, can receive either an int or
            # a long
            if isinstance(position, numbers.Integral):
                position = int(position)
                # may still be an int if the position is too big
            if type(position) is not int:
                _wrong_type_for_arg(position, "int", "position")

I was trying to stick to one type between both versions of python, int, which is why I don't make an allowance for long. (Note, under python 3, checking isinstance(position, long) raises an exception because long is not defined).

I thought a fix for the current problem with test test_GESMarker_color_to_otio_color and test test_GESMarker_colors might be to add this same leniency to
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/06dc494056321e9cd24da9fbc5f067f8473c8619/contrib/opentimelineio_contrib/adapters/xges.py#L3527

But it seems that test test_markers is also failing for the windows version, which calls GESMarker.__init__ with 23000000000 for the position argument.

On my platform (64bit linux), under python 2.7.18, the following is the behaviour I get:

>>> import numbers
>>> x = long(23000000000)
>>> x
23000000000L
>>> isinstance(x, numbers.Integral)
True
>>> int(x)
23000000000

So the conversion would have worked fine. It would be weird if this was different under windows, but if it is 32bit, it may be because sys.maxint < 23000000000. Someone with the windows interpreter could test this to be sure?

In any case, this is only a problem under python 2. So the tests should be ok to run on any platform with python 3.

If you are going to drop python 2 support anyway since it is deprecated, then this won't need a fix.

hwilkes-igalia · 2021-02-19T13:19:43Z

If you are going to drop python 2 support anyway since it is deprecated, then this won't need a fix.

If you are maintaining python2 support, you could change https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/contrib/opentimelineio_contrib/adapters/xges.py#L3506 to something like

        if type(position) is not int:
            # TODO: remove below once python2 has ended
            # currently in python2, can receive either an int or
            # a long
            if sys.version_info.major == 2:
                if type(position) is not long:
                    _wrong_type_for_arg(position, "int or long", "position")
            else:
                _wrong_type_for_arg(position, "int", "position")

And do the same here on these two lines:

ssteinbach · 2021-02-20T02:22:24Z

We won't be dropping Python 2 support until we're able to drop VFX platform 2020 support (I think thats the version), which will likely be another couple of years.

I'll see if I have time to back port the fixes, thanks.

ssteinbach · 2021-02-20T18:36:05Z

I guess a follow up question is that if Xges itself requires python3, maybe it makes sense to mute the tests for this adapter under python 2 as well.

ssteinbach · 2021-02-22T23:54:26Z

After thinking about it some more, I think what makes sense is to limit the XGES tests to run only on python3/linux. That seems to match with the intended platform, and unblocks this PR, without completely disabling those tests.

Furthermore, longer term we're going to be moving the larger contrib adapters out into their own repositories and use the pkg_resource mechanism to enable them to plug into OTIO, and I think the XGES adapter would be a good candidate for that. Stay posted for information on that coming up! We'll have a template repo soon to get those bootstrapped. Thanks for chiming in!

meshula

All looks reasonable to me. I did wonder after reading what our pathsep story is deeper within OTIO.

ssteinbach · 2021-02-23T03:29:43Z

This is just being mangled for testing purposes. If you generate it manually you'll OS-correct absolute paths. And there are very few places where OTIO actually deals with paths, AFAIK those go through the right abstractions. but its a good question.

ssteinbach added the WIP Work In Progress (Not ready to merge yet) label Jan 21, 2021

meshula approved these changes Jan 21, 2021

View reviewed changes

ssteinbach marked this pull request as draft January 21, 2021 23:38

ssteinbach added 2 commits February 11, 2021 11:22

add windows and mac builds to ci

e551029

fall back to mac 10.15

29686e3

ssteinbach force-pushed the more_os_ci branch from a8d8261 to 29686e3 Compare February 11, 2021 19:22

meshula approved these changes Feb 12, 2021

View reviewed changes

ssteinbach added 7 commits February 11, 2021 22:10

split lcov target out from the ci-postbuild target

1a839ea

Specify which build does C++ coverage w/ lcov

b0e560d

use == syntax instead of contains

b36754f

- seemed like contains didn't do what I expected, but I suspect the environment variable wasn't being read

move the env config out to the top level

dd74941

Skip autogen tests on windows (slashes)

0468291

sanitize paths when writing autogen docs

df626f8

try escaping the slash in the replacement

7247c1e

manifest paths need same treatment

e91e65e

ssteinbach added 8 commits February 17, 2021 14:45

another conversion to TemporaryDirectory

644ec3e

more tempfile cleanup

2c0714f

whoops missed import

072a5c0

testing if this is specific to the generators xml

bc0b554

only remove windows slashes from sanitized paths

29c8012

- by swapping all the paths out the logic that was isolating certain manifests was skipping _all_ the manifests

only run windows ci while I iterate

dfc66bd

run the broken test

eeb94fb

ssteinbach added this to the Public Beta 14 milestone Feb 18, 2021

remove BOM from example xml file

e2a6ea0

meshula reviewed Feb 18, 2021

View reviewed changes

replace more unit test tempfile handling to be more windows compatible

85c958c

ssteinbach added 2 commits February 18, 2021 13:44

only run XGES unit tests on linux

bc66af1

turning MacOS and Linux builds back on.

6ebee23

ssteinbach marked this pull request as ready for review February 18, 2021 22:12

ssteinbach removed the WIP Work In Progress (Not ready to merge yet) label Feb 18, 2021

ssteinbach requested a review from meshula February 18, 2021 22:12

meshula approved these changes Feb 18, 2021

View reviewed changes

Also disable xges for python < 3.

215de62

meshula approved these changes Feb 23, 2021

View reviewed changes

ssteinbach merged commit 45aee51 into AcademySoftwareFoundation:master Feb 23, 2021

ssteinbach deleted the more_os_ci branch February 23, 2021 03:32

ssteinbach mentioned this pull request Feb 23, 2021

Enable macOS and Windows CI on GitHub Actions #333

Closed

thiblahute mentioned this pull request Feb 23, 2021

Enable xges unit tests on all platform/python versions #894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add windows and mac builds to ci #873

add windows and mac builds to ci #873

ssteinbach commented Jan 21, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading

ssteinbach commented Feb 17, 2021

meshula Feb 18, 2021

ssteinbach Feb 18, 2021

thiblahute commented Feb 18, 2021

ssteinbach commented Feb 18, 2021

ssteinbach commented Feb 18, 2021

hwilkes-igalia commented Feb 19, 2021 •

edited

Loading

hwilkes-igalia commented Feb 19, 2021 •

edited

Loading

ssteinbach commented Feb 20, 2021

ssteinbach commented Feb 20, 2021

ssteinbach commented Feb 22, 2021

meshula left a comment

ssteinbach commented Feb 23, 2021

		@@ -1,4 +1,4 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<?xml version="1.0" encoding="UTF-8"?>

add windows and mac builds to ci #873

add windows and mac builds to ci #873

Conversation

ssteinbach commented Jan 21, 2021 • edited Loading

codecov-io commented Feb 13, 2021 • edited Loading

Codecov Report

ssteinbach commented Feb 17, 2021

meshula Feb 18, 2021

Choose a reason for hiding this comment

ssteinbach Feb 18, 2021

Choose a reason for hiding this comment

thiblahute commented Feb 18, 2021

ssteinbach commented Feb 18, 2021

ssteinbach commented Feb 18, 2021

hwilkes-igalia commented Feb 19, 2021 • edited Loading

hwilkes-igalia commented Feb 19, 2021 • edited Loading

ssteinbach commented Feb 20, 2021

ssteinbach commented Feb 20, 2021

ssteinbach commented Feb 22, 2021

meshula left a comment

Choose a reason for hiding this comment

ssteinbach commented Feb 23, 2021

ssteinbach commented Jan 21, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading

hwilkes-igalia commented Feb 19, 2021 •

edited

Loading

hwilkes-igalia commented Feb 19, 2021 •

edited

Loading