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: Use C17 only with CMake 3.21 or above #89

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

umireon
Copy link
Contributor

@umireon umireon commented Aug 29, 2023

Description

C17 is not supported until CMake 3.21 and C17 must be used with CMake 3.21 or above.
https://cmake.org/cmake/help/latest/prop_tgt/C_STANDARD.html

Motivation and Context

CMake 3.20 is provided on openSUSE Leap 15.5 and the plugin building with CMake commands fails with the message that CMake does not understand the C standard of C17.
I need this bug to be fixed to build on openSUSE Leap.

How Has This Been Tested?

This change is tested in our plugin.
locaal-ai/obs-backgroundremoval#430

The building log of our plugin with this change is available:
https://pmbs.links2linux.org/package/live_build_log/home:umireon/obs-backgroundremoval/15.5/x86_64

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

@umireon umireon changed the title C17 is not supported until CMake 3.21 Fix not to use C17 with CMake 3.20 or below Aug 29, 2023
@umireon umireon changed the title Fix not to use C17 with CMake 3.20 or below Fix to use C17 only with CMake 3.21 or above Aug 29, 2023
@umireon umireon marked this pull request as ready for review August 29, 2023 10:41
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.

The change itself is fine, and mirrors what we do on obs-studio.

However, please use the commit prefix "cmake: " and simply continue the message with "Use C17" (drop "Fix to ").

@umireon umireon changed the title Fix to use C17 only with CMake 3.21 or above cmake: Use C17 only with CMake 3.21 or above Aug 29, 2023
@umireon
Copy link
Contributor Author

umireon commented Aug 29, 2023

@RytoEX I've fixed the commit message.

@umireon umireon requested a review from RytoEX August 29, 2023 16:15
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.

Looks fine at a glance. @PatTheMav ?

@PatTheMav
Copy link
Member

Should be fine, we expect more recent versions on Windows and macOS anyway.

@RytoEX RytoEX merged commit c80d019 into obsproject:master Aug 29, 2023
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.

3 participants