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

Update CMake #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update CMake #60

wants to merge 3 commits into from

Conversation

RytoEX
Copy link
Member

@RytoEX RytoEX commented Feb 19, 2025

Description

This PR consists of three commits to make it easier to differentiate between the intended changes. The commits will be squashed, or the second commit can be dropped if we intend to keep support for non-MSVC compilers/toolchains.

Modernize the project's CMake:

  • Update to C/C++17
  • Use targets instead of global set and add calls
  • Assume WIN32 is always the target platform
  • Replace CMake 2.x with CMake 3.x compile language and ID checks
  • Remove unnecessary FindCXX11 module

Updating to C/C++17 is required due to code in the capture-device-support submodule using C++17 features.

Remove support for non-MSVC compilers.
Since libdshowcapture is a Windows-only library, supporting GCC, Clang, and MinGW adds extra complication that we otherwise would not have to handle if we only support MSVC. This could be added back in the future if there is a reasonable justification for it.

Reformat CMake with gersemi
This matches the CMake formatting preferences in obs-studio.

Motivation and Context

The CMake currently in the repo fails to configure. I figured this was a good opportunity to do some cleanups. This could probably go further, and I do additionally intend to remove the vs folder containing the solution and project files. For now, I wanted to get this off of my local machine and placed here for public comment/review.

How Has This Been Tested?

Tested locally on Windows 11. The CMake configures, generates a solution, and the solution can be built with VS2022.

You should be able to configure and generate a solution with:

cmake -S <libdshowcapture-repo-directory> -B <build-directory>

Then you should be able to open the solution and build it with Visual Studio, or build it via CLI with CMake:

cmake --build <build-directory>

As far as I know, obs-studio itself does not use this repo's CMake, so these changes should not affect usage there.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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 February 19, 2025 20:36
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.

Looks fine apart from the more specific comments. If this indeed is supposed to require MSVC, then maybe we should provide a FATAL_ERROR message if the MSVC generator is not used.

CMakeLists.txt Outdated
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN TRUE)

set(
Copy link
Member

Choose a reason for hiding this comment

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

If we're in the process of effectively rewriting this file, then we should use target_sources directly and just move the library definition further up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

CMakeLists.txt Outdated
# Disable pointless constant condition warnings
target_compile_options(libdshowcapture PRIVATE /W4 /wd4127 /wd4201)

if(NOT CMAKE_SIZEOF_VOID_P EQUAL 8)
Copy link
Member

Choose a reason for hiding this comment

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

Would probably make sense to explicit check the VS generator platform here and check explicitly for x86/x64/arm64 here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

From the documentation, /SAFESEH is only valid when linking for x86 targets, so I think this is the platform where we override it to be /SAFESEH:NO to make sure all platforms do not use SAFESEH.

https://learn.microsoft.com/en-us/cpp/build/reference/safeseh-image-has-safe-exception-handlers?view=msvc-170

RytoEX added 3 commits March 3, 2025 19:38
* Update to C/C++17
* Use targets instead of global set and add calls
* Assume WIN32 is always the target platform
* Replace CMake 2.x with CMake 3.x compile language and ID checks
* Remove unnecessary FindCXX11 module
@RytoEX
Copy link
Member Author

RytoEX commented Mar 4, 2025

Looks fine apart from the more specific comments. If this indeed is supposed to require MSVC, then maybe we should provide a FATAL_ERROR message if the MSVC generator is not used.

Done.

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