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

CMake: Don't parallel-test on Windows, protect FFmpeg version checks #684

Merged
merged 4 commits into from Jun 7, 2021

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jun 6, 2021

Somehow the old UnitTest++ version of tests/KeyFrame_Tests.cpp
got resurrected back into the repo. Shotgun blast to the head.

(@jonoomph : I THINK this is the cause of the Win32 builder issues, oddly enough. We'll find out as soon as I blow it away.)

Update: I was wrong. It appears that on Windows the tests just experience occasional hangs. I'm hoping it's just because the tests are being run in parallel. I've added logic to turn off the parallel testing on Windows platforms, we'll see if that clears it up. It "felt" to me like the hangs were the result of multiple test runs trampling all over each other. (Possibly some race condition as they contend over the same ZeroMQ port bindings, or the like.)

Also, it seems I missed something pretty important, when I recently added new version logic to CMake: The version variables for the FFmpeg components are only set if FFmpeg is detected using PkgConfig. If it's not (i.e., on Windows) those variables will exist but contain the empty string, something I hadn't properly protected against. This should fix it.

Somehow the old UnitTest++ version of tests/KeyFrame_Tests.cpp
got resurrected back into the repo. Shotgun blast to the head.
@ferdnyc ferdnyc added the tests Changes related to the unit tests and/or code coverage label Jun 6, 2021
@codecov
Copy link

codecov bot commented Jun 6, 2021

Codecov Report

Merging #684 (8689b62) into develop (8a81452) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #684   +/-   ##
========================================
  Coverage    50.41%   50.41%           
========================================
  Files          155      155           
  Lines        13315    13315           
========================================
  Hits          6713     6713           
  Misses        6602     6602           

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 8a81452...8689b62. Read the comment docs.

avcodec_VERSION and friends can be the empty string, on Windows
@ferdnyc ferdnyc changed the title Unit tests: Destroy zombie test CMake: Remove zombie unit test, protect version checks Jun 6, 2021
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 6, 2021

Also, it seems I missed something pretty important, when I recently added new version logic to CMake: The version variables for the FFmpeg components are only set if FFmpeg is detected using PkgConfig. If it's not (i.e., on Windows) those variables will exist but contain the empty string, something I hadn't properly protected against.

The only reason our project builder Windows builds were still working at all, I think, is that we don't start from a completely fresh build directory each time. (Or do we? I don't think so, or none of the Windows builds would have been able to get past the CMake configuration stage.) The existing build files were already set up, so the failure to detect FFmpeg component versions never came up.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 6, 2021

Checks out on Linux, but I'm going to let my own local Windows test builds finish before merging.

If we don't have a version number and can't conclusively determine
whether hwaccel is available, but the user hasn't disabled it
explicitly, just don't report its status at all. Better than
reporting it's disabled when that may not be true at all.
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 7, 2021

(@jonoomph : I THINK this is the cause of the Win32 builder issues, oddly enough. We'll find out as soon as I blow it away.)

I was wrong. It appears that on Windows they just experience occasional hangs. I'm hoping it's just because the tests are being run in parallel. I've added logic to turn off the parallel testing on Windows platforms, we'll see if that clears it up.

@ferdnyc ferdnyc changed the title CMake: Remove zombie unit test, protect version checks CMake: Don't parallel-test on Windows, protect FFmpeg version checks Jun 7, 2021
@ferdnyc ferdnyc merged commit 403d9a6 into OpenShot:develop Jun 7, 2021
@ferdnyc ferdnyc deleted the zombie-unit-test branch June 7, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Changes related to the unit tests and/or code coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant