Update plugintemplate to CMake 3.0 build system and modern code requirements

PatTheMav commented 1 year ago

Description

This PR represents an overhaul of the entire plugin to stay compatible with changes also being introduced at the main repository, including:

Update of CMake scripts to the new framework introduced in the obs-studio repository
Update of all build flags to include stricter code requirements and more enabled warnings
Update of all GitHub Actions workflows to use a more modular design and support schedule as well as dispatch workflow runs
Reduce complexity of scripts by moving build dependency resolution into CMake scripts
Remove all functionality to build obs-studio on Linux and rely on libobs-dev package instead
Remove reliance on generated header and macro definitions as well as macro functions
Uses optimised build configurations:
- Debug for pull requests and dispatches
- RelWithDebInfo for pushes on the master branch and scheduled builds
- Release for tag pushes
Releases now also contain a "source tarball"
Added aarch64 as available Linux build configuration (CMake toolchain scripts for cross compilation from/to x86_64 from/to aarch64 are included)

Motivation and Context

Update scripts and source code to updated standards on the main repository, which also allows to share more code between both.

How Has This Been Tested?

Tested pull requests, dispatches, pushes, and releases on separate fork, including macOS code signing and notarization.

Types of changes

New feature (non-breaking change which adds functionality)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code has been run through clang-format.
[x] I have read the contributing document.
[x] My code is not on the master branch.
[x] The code has been tested.
[x] All commit messages are properly formatted and commits squashed where appropriate.
[x] I have included updates to all appropriate documentation.

PatTheMav commented 1 year ago

Small update: The build system itself has support for clang on Windows and Linux, but it's not enabled by default:

On Linux the aarch64-linux-clang.cmake and x86_64-linux-clang.cmake toolchain files need to be used to switch
On Windows -T clangcl needs to be added to the cmake configuration command line to enable it

I also removed the mandatory flags to treat warnings as errors, as CMake has introduced the CMAKE_COMPILE_WARNING_AS_ERROR boolean variable in CMake 3.24 that allows switching this behaviour on and off - so CI builds have it enabled, but local development does not have, which should be a nice middle ground.

gxalpha commented 1 year ago

Hi, just tried this on macOS, and noticed two things:

libobs' obs.hpp C++ helper header is not available in the libobs framework used here.
With ENABLE_QT=ON I get a bunch of warnings-turned-into-errors inside of Qt (Possible misuse of comma operator here, Double-quoted include "qapplication.h" in framework header, expected angle-bracketed instead, etc). It might make sense to disable warnings-as-errors just in Qt if that's somehow possible. I worked around this for now by setting -DCMAKE_COMPILE_WARNING_AS_ERROR=OFF

Seeing as I couldn't use the AutoRelease types because of the missing header I can't say much more than "the plugin built and got loaded and would probably work".

PatTheMav commented 1 year ago

Hi, just tried this on macOS, and noticed two things:

libobs' obs.hpp C++ helper header is not available in the libobs framework used here.

Needs to be fixed in obs-studio.

With ENABLE_QT=ON I get a bunch of warnings-turned-into-errors inside of Qt (Possible misuse of comma operator here, Double-quoted include "qapplication.h" in framework header, expected angle-bracketed instead, etc). It might make sense to disable warnings-as-errors just in Qt if that's somehow possible. I worked around this for now by setting -DCMAKE_COMPILE_WARNING_AS_ERROR=OFF

Cannot reproduce - how do you end up in this state?

gxalpha commented 1 year ago

@gxalpha if you're running this on a plugin repo, I'd appreciate additional feedback or approval

Sure, I’ll give it a further look when I have time (might not be until Thursday).

Needs to be fixed in obs-studio.

Very unfortunate, I suspect as-is this would block many developers whose code base already uses the types from switching to the new template. How difficult would fixing it be (or is it by any chance part of one of your PRs already)?

Cannot reproduce - how do you end up in this state?

I think I just used the macos cmake preset, but will double-check when I have time (again, perhaps not until Thursday). It probably requires including (and maybe using) the headers in the source file.

PatTheMav commented 1 year ago

@gxalpha if you're running this on a plugin repo, I'd appreciate additional feedback or approval

Sure, I’ll give it a further look when I have time (might not be until Thursday).

Needs to be fixed in obs-studio.

Very unfortunate, I suspect as-is this would block many developers whose code base already uses the types from switching to the new template. How difficult would fixing it be (or is it by any chance part of one of your PRs already)?

Easy to fix - it's a one-liner PR.

gxalpha commented 1 year ago

Easy to fix - it's a one-liner PR.

Would be worth including in a potential 29.1.2 then imo

PatTheMav commented 1 year ago

Easy to fix - it's a one-liner PR.

Would be worth including in a potential 29.1.2 then imo

https://github.com/obsproject/obs-studio/pull/8917

gxalpha commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

PatTheMav commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

Ah yes, Qt generates lots of stuff that is actually wrong on macOS, but we also need to suppress those on obs-studio.

I added suppression for both types of warnings as there is little chance those will be fixed upstream anytime soon.

RytoEX commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

Ah yes, Qt generates lots of stuff that is actually wrong on macOS, but we also need to suppress those on obs-studio.

I added suppression for both types of warnings as there is little chance those will be fixed upstream anytime soon.

Is there an open QTBUG at all documenting that their UIC generation is incorrect?

RytoEX commented 1 year ago

https://github.com/obsproject/obs-plugintemplate/compare/b8048701caefc069b40cd9fafbbd21e4f8233823..c5963d2f1ec5b815648017a4de4b44b63cdd8451

~~It's C Extensions, not CXX Extensions, that we need to disable on Linux/POSIX.~~

Disregard, I misread.

PatTheMav commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

Ah yes, Qt generates lots of stuff that is actually wrong on macOS, but we also need to suppress those on obs-studio. I added suppression for both types of warnings as there is little chance those will be fixed upstream anytime soon.

Is there an open QTBUG at all documenting that their UIC generation is incorrect?

It's not a UIQ generation issue, it's a code style issue. They always use quoted includes instead of proper system library includes (as expected by Xcode's default settings) even in the main Qt frameworks themselves.

RytoEX commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

Ah yes, Qt generates lots of stuff that is actually wrong on macOS, but we also need to suppress those on obs-studio. I added suppression for both types of warnings as there is little chance those will be fixed upstream anytime soon.

Is there an open QTBUG at all documenting that their UIC generation is incorrect?

It's not a UIQ generation issue, it's a code style issue. They always use quoted includes instead of proper system library includes (as expected by Xcode's default settings) even in the main Qt frameworks themselves.

Even so, that would perhaps be good to document upstream as "this is not what Xcode expects to see" so that they can consider addressing it.

PatTheMav commented 1 year ago

Cannot reproduce - how do you end up in this state?

@PatTheMav These seem to happen when you have includes like #include <QtWidgets/QDockWidget> instead of #include <QDockWidget> (the latter doesn't throw warnings). Unfortunately this is what Qt UIC uses when generating the ui_[...].h headers.

Ah yes, Qt generates lots of stuff that is actually wrong on macOS, but we also need to suppress those on obs-studio. I added suppression for both types of warnings as there is little chance those will be fixed upstream anytime soon.

Is there an open QTBUG at all documenting that their UIC generation is incorrect?

It's not a UIQ generation issue, it's a code style issue. They always use quoted includes instead of proper system library includes (as expected by Xcode's default settings) even in the main Qt frameworks themselves.

Even so, that would perhaps be good to document upstream as "this is not what Xcode expects to see" so that they can consider addressing it.

I've done some more research on this and it seems that this requirement stems from the needs for module import support (using @import Module by consumers vs directly including headers) as the compiler will figure out which headers to actually include and optimise that usage accordingly, and improved support in Swift.

The major issue I saw is that using @import or the suggested #import directive both only work for ObjC code. Given that Qt macOS projects can be entirely C++ based, this would make the frameworks incompatible to use for such projects.

PatTheMav commented 1 year ago

Addressed last set of comments by @gxalpha, removed some architectural redundancy (Qt version and macOS deployment target were specified in build spec and CMake presets file), also updated README file.

gxalpha commented 1 year ago

Windows and macOS work fine now, Ubuntu appears to fail on CI with Qt enabled:

  CMake Warning at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6Gui/Qt6GuiConfig.cmake

    but it set Qt6Gui_FOUND to FALSE so package "Qt6Gui" is considered to be
    NOT FOUND.  Reason given by package:

    Qt6Gui could not be found because dependency WrapOpenGL could not be found.

  Call Stack (most recent call first):
    /usr/local/share/cmake-3.26/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/QtPublicDependencyHelpers.cmake:14 (find_dependency)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsDependencies.cmake:96 (_qt_internal_find_dependencies)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake:50 (include)
    cmake/linux/defaults.cmake:83 (_find_package)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake:219 (find_package)
    cmake/linux/defaults.cmake:83 (_find_package)
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

  CMake Warning at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake

    but it set Qt6Widgets_FOUND to FALSE so package "Qt6Widgets" is considered
    to be NOT FOUND.  Reason given by package:

    Qt6Widgets could not be found because dependency Qt6Gui could not be found.

  Call Stack (most recent call first):
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake:219 (find_package)
    cmake/linux/defaults.cmake:83 (_find_package)
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

  CMake Error at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake

    but it set Qt6_FOUND to FALSE so package "Qt6" is considered to be NOT
    FOUND.  Reason given by package:

    Failed to find Qt component "Widgets".

    Expected Config file at
    "/usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake" exists

  Call Stack (most recent call first):
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

PatTheMav commented 1 year ago

Windows and macOS work fine now, Ubuntu appears to fail on CI with Qt enabled:

  CMake Warning at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6Gui/Qt6GuiConfig.cmake

    but it set Qt6Gui_FOUND to FALSE so package "Qt6Gui" is considered to be
    NOT FOUND.  Reason given by package:

    Qt6Gui could not be found because dependency WrapOpenGL could not be found.

  Call Stack (most recent call first):
    /usr/local/share/cmake-3.26/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/QtPublicDependencyHelpers.cmake:14 (find_dependency)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsDependencies.cmake:96 (_qt_internal_find_dependencies)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake:50 (include)
    cmake/linux/defaults.cmake:83 (_find_package)
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake:219 (find_package)
    cmake/linux/defaults.cmake:83 (_find_package)
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

  CMake Warning at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake

    but it set Qt6Widgets_FOUND to FALSE so package "Qt6Widgets" is considered
    to be NOT FOUND.  Reason given by package:

    Qt6Widgets could not be found because dependency Qt6Gui could not be found.

  Call Stack (most recent call first):
    /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake:219 (find_package)
    cmake/linux/defaults.cmake:83 (_find_package)
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

  CMake Error at cmake/linux/defaults.cmake:83 (_find_package):
    Found package configuration file:

      /usr/lib/x86_64-linux-gnu/cmake/Qt6/Qt6Config.cmake

    but it set Qt6_FOUND to FALSE so package "Qt6" is considered to be NOT
    FOUND.  Reason given by package:

    Failed to find Qt component "Widgets".

    Expected Config file at
    "/usr/lib/x86_64-linux-gnu/cmake/Qt6Widgets/Qt6WidgetsConfig.cmake" exists

  Call Stack (most recent call first):
    cmake/common/helpers_common.cmake:74 (find_package)
    CMakeLists.txt:25 (find_qt)

Looks like Qt requires OpenGL to be installed (which is not correctly reproduced by the package it seems) - installing libgles2-mesa-dev seems to have fixed the issue.

So here's the $64k question:

Historically we disabled the frontend-api and Qt by default to not insinuate that plugins are required to be built with either. But as far as CI and this template is concerned, we should enable them both by default to check if the template itself builds correctly.

I'd argue that now that both can be disabled as a CMake option (without code changes necessary), we should enable them by default in the template. Too many issues could've been found if we had compiled it that way on CI by default.

RytoEX commented 1 year ago

Historically we disabled the frontend-api and Qt by default to not insinuate that plugins are required to be built with either. But as far as CI and this template is concerned, we should enable them both by default to check if the template itself builds correctly.

Originally, they were enabled by default, which led to some plugins leaving them enabled when they did not use any Qt features, which caused those plugins to break when we updated from Qt 5 to Qt 6 due to linking to Qt. See: https://github.com/obsproject/obs-plugintemplate/blob/cdc6df7c3ac35d09c975c5d71db1a44a887fe4dd/CMakeLists.txt#L25-L31 https://github.com/obsproject/obs-plugintemplate/blob/cdc6df7c3ac35d09c975c5d71db1a44a887fe4dd/CMakeLists.txt#L47-L52

I'd argue that now that both can be disabled as a CMake option (without code changes necessary), we should enable them by default in the template. Too many issues could've been found if we had compiled it that way on CI by default.

I'd suggest leaving them disabled by default, but running two separate jobs on CI for our own testing. Mark the test job clearly as "test-frontend-api" or "test-qt" or some such, and provide a note inside it that it is not meant to be kept by plugin repos.

Alternatively, we fork the template ourselves into a test repo and build the test jobs there to avoid polluting the template.

CodeYan01 commented 1 year ago

is 29.1.2 a hard minimum requirement on new plugins going forward? and existing plugins would need to implement this PR on their repo?

PatTheMav commented 1 year ago

Historically we disabled the frontend-api and Qt by default to not insinuate that plugins are required to be built with either. But as far as CI and this template is concerned, we should enable them both by default to check if the template itself builds correctly.

Originally, they were enabled by default, which led to some plugins leaving them enabled when they did not use any Qt features, which caused those plugins to break when we updated from Qt 5 to Qt 6 due to linking to Qt. See:

https://github.com/obsproject/obs-plugintemplate/blob/cdc6df7c3ac35d09c975c5d71db1a44a887fe4dd/CMakeLists.txt#L25-L31

https://github.com/obsproject/obs-plugintemplate/blob/cdc6df7c3ac35d09c975c5d71db1a44a887fe4dd/CMakeLists.txt#L47-L52

I'd argue that now that both can be disabled as a CMake option (without code changes necessary), we should enable them by default in the template. Too many issues could've been found if we had compiled it that way on CI by default.

I'd suggest leaving them disabled by default, but running two separate jobs on CI for our own testing. Mark the test job clearly as "test-frontend-api" or "test-qt" or some such, and provide a note inside it that it is not meant to be kept by plugin repos.

Alternatively, we fork the template ourselves into a test repo and build the test jobs there to avoid polluting the template.

That would require changing the preset file on the fly in CI jobs as there is no external flag or build script switch to enable this - that would've spread the concern into places where it doesn't belong.

Marking these additional jobs for removal after forking would also make merging upstream changes harder (and goes against the idea that the scripts and CI workflows can be synced from the origin without much care).

Just changing the default in the preset is simpler, more obvious, and is also precisely the place where this choice should be made and the preset file is literally the first thing people should review/customise.

PatTheMav commented 1 year ago

is 29.1.2 a hard minimum requirement on new plugins going forward? and existing plugins would need to implement this PR on their repo?

New plugins should always target the current stable version, but authors are free to change the version and source hashes as they need with the caveats mentioned in the updated readme file.

CodeYan01 commented 1 year ago

thanks

CodeYan01 commented 1 year ago

I suggest adding steps in the readme for building for local development. Currently, only github actions are explained, but plugin authors would want to build it for themselves locally.

RytoEX commented 1 year ago

Just changing the default in the preset is simpler, more obvious, and is also precisely the place where this choice should be made and the preset file is literally the first thing people should review/customise.

Unfortunately, evidence shows that they don't.

obsproject / obs-plugintemplate