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: Remove Qt version selection and Qt 5 support #119

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Apr 11, 2024

Description

This is a port of obsproject/obs-studio#9263 (obsproject/obs-studio@9d611a0).

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

  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

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

@RytoEX RytoEX requested a review from PatTheMav April 11, 2024 01:08
@RytoEX RytoEX self-assigned this Apr 11, 2024
@RytoEX
Copy link
Member Author

RytoEX commented Apr 11, 2024

cc @gxalpha

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RytoEX
Copy link
Member Author

RytoEX commented Apr 12, 2024

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
Copy link
Member

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
Copy link
Member Author

RytoEX commented Apr 12, 2024

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
Copy link
Member Author

RytoEX commented Apr 12, 2024

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

@PatTheMav
Copy link
Member

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 and others added 2 commits April 12, 2024 18:49
We currently no longer need this functionality.
@RytoEX
Copy link
Member Author

RytoEX commented Apr 12, 2024

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

@RytoEX RytoEX requested a review from PatTheMav April 13, 2024 00:19
@RytoEX RytoEX merged commit a1289fd into obsproject:master Apr 15, 2024
6 checks passed
@RytoEX RytoEX deleted the remove-qt5 branch April 15, 2024 22:16
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.

2 participants