Skip to content

Conversation

@PatTheMav
Copy link
Member

@PatTheMav PatTheMav commented Oct 27, 2023

Description

Cleans up the CMake code-base based on experience after the recent CMake build system updates:

  • Change OS-specific code branches to platform-specific generator expressions
  • Enforce list sorting in some places to ensure consistency
  • Disable CMake formatting for custom functions that are not recognised by cmake-format or for updated CMake functions whose updated signatures it doesn't support
  • Switch interface libraries to object libraries where possible
  • Remove custom CMake policy code for policies that were set to NEW by CMake 3.22+ anyway.
  • Remove support for Qt5 in dependency resolution
  • FFmpeg package selection uses an explicit version parameter (6.0.0+ on Windows and macOS, 5.1.0+ on Ubuntu)
  • Windows 🪟: Update Windows warning suppressions by investigating their root causes (some warnings' root causes need to be fixed upstream) and making suppressions more specific
  • Windows 🪟: Enable fallback configurations for libraries' optimised build types
  • macOS 🍏: Remove support for any generator besides Xcode on macOS

Important

An accompanying update of obs-browser will also
requires the CEF wrapper library to be re-built for Windows following the proper procedure outlined in the documentation: https://bitbucket.org/chromiumembedded/cef/wiki/LinkingDifferentRunTimeLibraries.md

Without updating the submodule the project will still build without issues.

Motivation and Context

Moving platform-specific source file and library dependency information into global blocks with generator expressions makes the requirements more explicit (and also groups all source code dependencies into one logical place), removing the need to search for their inclusion in other parts of the file (and reduces amount if code branches, which also improves cmake-lint checks).

Changes are mostly cosmetic or attempt to reduce complexity by removing seldom-used options or features introduced in recent updates. Windows-specific changes also aim to make more informed decisions about which warnings are suppressed and why.

How Has This Been Tested?

Preliminary tests looked fine, specific tests are still pending:

  • Builds on Windows 11 (Browser disabled)
  • Builds on Windows 11 (With fixed runtime library for CEF-wrapper)
  • Builds on Windows 11 (CMake Legacy, Browser disabled)
  • Builds on Windows 11 (CMake Legacy, with fixed runtime library for CEF-wrapper)
  • Builds on macOS 13
  • Builds on macOS 14
  • Builds on Ubuntu 22.04 (CMake Legacy, AJA+WebRTC+New MPEGTS output disabled)
  • Builds on Ubuntu 23.04 (CMake Legacy)
  • Builds on FreeBSD 13.2 (CMake Legacy, AJA+JACK disabled)

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 self-assigned this Oct 27, 2023
@RytoEX RytoEX added the Code Cleanup Non-breaking change which makes code smaller or more readable label Oct 27, 2023
@tytan652
Copy link
Collaborator

tytan652 commented Oct 28, 2023

Some changes around shared UI code might conflicts with #9350.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Mostly questions seeking rationale and clarification. Builds locally on Windows 11. Haven't done any further in-depth testing.

@PatTheMav
Copy link
Member Author

Force-pushed an update to address some reviews, also:

  • Added explicit checks for OS_OPENBSD and OpenBSD platform id
  • Switched from C_COMPILE_ID checks to COMPILE_LANG_AND_ID, which has the benefit of applying the expression when the target's source code language and associated compiler id match (and not just if the compiler id for the language in general matches).
  • Removed the addition if the -py3 flag to swig, which has been deprecated a while ago.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Second pass.

@PatTheMav PatTheMav force-pushed the cmake-cleanup branch 2 times, most recently from 9d072ea to df3ae0f Compare November 9, 2023 17:13
@PatTheMav PatTheMav marked this pull request as ready for review November 17, 2023 15:52
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Quick review of files I hadn't marked viewed since last review.

@PatTheMav PatTheMav force-pushed the cmake-cleanup branch 5 times, most recently from 3aa635a to ac34889 Compare December 11, 2023 14:29
@RytoEX
Copy link
Member

RytoEX commented Dec 14, 2023

Remove support for Qt5, removed helper function used during transition

I think this was left in for now to ease the transition of submodules to removing their use of it. We can remove the actual CMake function in the next major release. In any case, the PR description may need updating.

@PatTheMav
Copy link
Member Author

Remove support for Qt5, removed helper function used during transition

I think this was left in for now to ease the transition of submodules to removing their use of it. We can remove the actual CMake function in the next major release. In any case, the PR description may need updating.

Updated the description - Qt5 was indeed removed in the parts that resolve dependencies of the main app but the helper function is still there.

@PatTheMav
Copy link
Member Author

Rebased and squashed commits into logical units.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Local tests seemed fine. CI is green.

@RytoEX RytoEX changed the title cmake: Clean-up pass and code optimisation to prepare for Linux update cmake: Clean-up pass and code optimization to prepare for Linux update Dec 19, 2023
@RytoEX RytoEX merged commit c4ce858 into obsproject:master Dec 19, 2023
@PatTheMav PatTheMav deleted the cmake-cleanup branch December 19, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Cleanup Non-breaking change which makes code smaller or more readable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants