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

Gitlab-CI: Update Windows builder configs, OpenShotAudio linking; enable Windows unit tests #657

Merged
merged 16 commits into from Apr 19, 2021

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Apr 18, 2021

This PR (which has OpenShot/libopenshot-audio#118 and OpenShot/libopenshot-audio#119 as prerequisites) similarly updates the Windows project builder configs to match their enhanced MinGW32/64 environments, but also makes some changes of general note:

  1. The FindOpenShotAudio.cmake module is deleted from cmake/Modules/. CMake will now use the EXPORTED CMake configuration produced (pending the merge of CMake: Create and install EXPORTED configuration libopenshot-audio#118) when building and installing the "OpenShotAudio" project — aka libopenshot-audio — to confgure that dependency, making the Find module unnecessary.

    If OpenShotAudio is built and installed in a system default location like /usr/ or C:\msys64\mingw32\, one where CMake is configured to look for package configs by default, then the <prefix>/share/cmake/OpenShotAudio/OpenShotAudioConfig.cmake files will be picked up automatically when configuring the libopenshot build ­— nothing needs to be done.

    If the OpenShotAudio build has been installed in a non-standard location (say, /opt/OpenShotAudio, then any CMake >= 3.12 can be directed to the configuration by setting the OpenShotAudio_ROOT variable to that prefix path. e.g.:

    cmake -B build -S . -DOpenShotAudio_ROOT=/opt/OpenShotAudio ...

    CMake will discover the /opt/OpenShotAudio/share/cmake/OpenShotAudio/ directory created by the OpenShotAudio install, and configure the build in accordance with its contents.

    OpenShotAudio_ROOT can also be pointed at a build directory to pick up the dependency there, without installing the project. However, this is likely to cause problems on Linux once libopenshot is installed, unless at that time (if not before) libopenshot-audio is also installed to the same (system) library path. The current build configs conform to CMake's default Linux convention of disabling all RPATHs on installed binaries and executables.

  2. The OpenShotAudio main header file will have a new name as of CMake: Create and install EXPORTED configuration libopenshot-audio#118, so all #includes in the libopenshot code have been replaced:

    // Old form
    #include "JuceHeader.h"
    // New form
    #include <OpenShotAudio.h>

    The header is also still installed under the old name, for backwards-compatibility, but this should be considered deprecated and subject to future removal. A compile-time warning may eventually be emitted when the JuceHeader.h file is included, to raise awareness of this change.

    Only the header file name has changed; audio classes defined in OpenShotAudio are still under the juce:: namespace, same as before.

  3. Catch2 unit tests are now runnable on any Windows system that has the mingw-w64-*-catch package(s) installed in MSYS2. (Or any system where Catch2 is manually installed and visible to CMake.)

    The only other requirement is that every DLL dependency other than libopenshot-audio.dll must be located on the $PATH and accessible to the unit test openshot-*-test.exe files when they're run during the build. The build script will copy the necessary libopenshot and libopenshot-audio DLLs into the unit test space so they can be found, it's only external dependencies that need to be accessible via $PATH.

  4. Minor, but the <build>/examples/openshot-html-test program compiled from examples/ExampleHtml.cpp is renamed to <build>/examples/openshot-html-example. All of the new unit test executables are named <build>/tests/openshot-<class>-test, so the old name was destined to cause confusion.

When `#include`-ing the whole mess, building the file was leading
to what looked like out-of-memory errors. Replacing the monolithic
header with just the necessary ones cleared it up.
- Copying the built and depended (libopenshot-audio) DLLs into
  the unit test dir before building the tests ensures that when
  the test executables are run by the Catch.cmake module to
  discover their contents, the executables will find the DLLs
  they need in order to run.
(The former name, openshot-html-test, was too close to the new
name format for all of our unit test executables, making things
unnecessarily confusing.)
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 18, 2021

(Again, this is expected to fail to build until OpenShot/libopenshot-audio#118 is merged, so the failures serve as a handy reminder that the other PR needs to go in first.)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 18, 2021

Well, OpenShot/libopenshot-audio#118 is merged, but the PPA manifest needs to have the /usr/share/cmake/OpenShotAudio/*.cmake files added to it so they'll be packaged. @jonoomph , I submitted a merge request @ launchpad for that, if you have a moment to review it? https://code.launchpad.net/~ferdnyc/openshot-packaging/+git/openshot-packaging-1/+merge/401348

@github-actions
Copy link

Merge conflicts have been detected on this PR, please resolve.

@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Apr 19, 2021
@ferdnyc ferdnyc removed the conflicts A PR with unresolved merge conflicts label Apr 19, 2021
@jonoomph
Copy link
Member

@ferdnyc LaunchPad merge request approved! Thx

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 19, 2021

@jonoomph Thanks! I triggered a new build of the libopenshot-audio DEBs, and it looks from the build log like the CMake configs are indeed packaged, so fingers crossed that this will now build successfully in CI and can be merged.

With the switch to an EXPORTED CMake configuration for
libopenshot-audio, its dependencies become our dependencies.
Which means that CMake now correctly requires that the ALSA
libs be installed.
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 19, 2021

UGGGGGGGGH.

CMake Error at /usr/share/cmake/OpenShotAudio/OpenShotAudioTargets.cmake:84 (message):
-- Configuring incomplete, errors occurred!
See also "/home/runner/work/libopenshot/libopenshot/build/CMakeFiles/CMakeOutput.log".
  The imported target "OpenShot::openshot-audio-demo" references the file

     "/usr/bin/openshot-audio-demo"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/share/cmake/OpenShotAudio/OpenShotAudioTargets.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  /usr/share/cmake/OpenShotAudio/OpenShotAudioConfig.cmake:72 (include)
  src/CMakeLists.txt:178 (find_package)


Error: Process completed with exit code 1.

(I just merged OpenShot/libopenshot-audio#121 to remove the demo binary from the EXPORTED configuration.)

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #657 (b8b711e) into develop (7a6ff7c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #657   +/-   ##
========================================
  Coverage    52.42%   52.42%           
========================================
  Files          151      151           
  Lines        12346    12346           
========================================
  Hits          6473     6473           
  Misses        5873     5873           
Impacted Files Coverage Δ
src/AudioReaderSource.h 0.00% <ø> (ø)
src/Clip.h 75.00% <ø> (ø)
src/Frame.cpp 46.30% <ø> (ø)
src/Frame.h 100.00% <ø> (ø)
src/Settings.h 50.00% <ø> (ø)
src/ZmqLogger.h 33.33% <ø> (ø)

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 7a6ff7c...b8b711e. Read the comment docs.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Apr 19, 2021

FINALLY. Successfully built in CI; merging so we can get builds back on track for other branches in the project Windows builders.

@ferdnyc ferdnyc merged commit 4e4a95c into develop Apr 19, 2021
@ferdnyc ferdnyc deleted the mingw-paths branch April 19, 2021 20:00
@jonoomph
Copy link
Member

Nice job!

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

2 participants