obsproject / obs-plugintemplate

GNU General Public License v2.0
285 stars 133 forks source link

cmake: Remove Qt version selection and Qt 5 support #119

Closed RytoEX closed 4 months ago

RytoEX commented 4 months ago

Description

This is a port of https://github.com/obsproject/obs-studio/pull/9263 (https://github.com/obsproject/obs-studio/commit/9d611a064f042cec6cba1d64818c8e810ebc4744).

This does not remove find_qt, but just forces it to Qt 6 for now.

Motivation and Context

We no longer support building OBS Studio with Qt 5.

How Has This Been Tested?

I haven't tested this, This is a port of an existing commit, which has been tested.

Types of changes

Checklist:

RytoEX commented 4 months ago

cc @gxalpha

RytoEX commented 4 months ago

Probably worth to update the template itself to not use find_qt itself (but the normal find_package call for Qt) but retain the function to not immediately break all plugins that just fetch&merge upstream changes.

As far as I can tell from the linked obs-studio PR, we continued to use find_qt in obs-studio, so I just mirrored that here. If we want to do more cleanup, which I am in favor of, we can probably do more in both obs-studio and here in new PRs.

PatTheMav commented 4 months ago

Probably worth to update the template itself to not use find_qt itself (but the normal find_package call for Qt) but retain the function to not immediately break all plugins that just fetch&merge upstream changes.

As far as I can tell from the linked obs-studio PR, we continued to use find_qt in obs-studio, so I just mirrored that here. If we want to do more cleanup, which I am in favor of, we can probably do more in both obs-studio and here in new PRs.

We use find_package(Qt6 REQUIRED Widgets Network Svg Xml) in all updated CMake files - only the legacy paths should be using find_qt at this point.

RytoEX commented 4 months ago

Probably worth to update the template itself to not use find_qt itself (but the normal find_package call for Qt) but retain the function to not immediately break all plugins that just fetch&merge upstream changes.

As far as I can tell from the linked obs-studio PR, we continued to use find_qt in obs-studio, so I just mirrored that here. If we want to do more cleanup, which I am in favor of, we can probably do more in both obs-studio and here in new PRs.

We use find_package(Qt6 REQUIRED Widgets Network Svg Xml) in all updated CMake files - only the legacy paths should be using find_qt at this point.

Hmm. Why do we still define the macro in non-legacy CMake? To avoid breakage at the time? https://github.com/obsproject/obs-studio/blob/97b03e3c5441199703cd76ea10fb26d7f2f682b2/cmake/common/helpers_common.cmake#L110-L111

RytoEX commented 4 months ago

That said, I can certainly remove it entirely from here if obs-plugintemplate has no notion of legacy/modern CMake.

PatTheMav commented 4 months ago

Probably worth to update the template itself to not use find_qt itself (but the normal find_package call for Qt) but retain the function to not immediately break all plugins that just fetch&merge upstream changes.

As far as I can tell from the linked obs-studio PR, we continued to use find_qt in obs-studio, so I just mirrored that here. If we want to do more cleanup, which I am in favor of, we can probably do more in both obs-studio and here in new PRs.

We use find_package(Qt6 REQUIRED Widgets Network Svg Xml) in all updated CMake files - only the legacy paths should be using find_qt at this point.

Hmm. Why do we still define the macro in non-legacy CMake? To avoid breakage at the time? https://github.com/obsproject/obs-studio/blob/97b03e3c5441199703cd76ea10fb26d7f2f682b2/cmake/common/helpers_common.cmake#L110-L111

Correct, at first neither obs-browser nor obs-websocket were updated, but we chose not to have that block merging the CMake update and allow the submodules to migrate separately.

RytoEX commented 4 months ago

I've pushed a second commit that outright removes find_qt. Can squash as needed.