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

Don't overwrite CMake configuration types if yyjson is used as a dependency #142

Merged

Conversation

AntonShalgachev
Copy link
Contributor

Hi!
I've noticed that the CMake script overwrites the value of CMAKE_CONFIGURATION_TYPES for multi-config CMake generators. That effectively locks the configuration list to Debug and Release for CMake projects that add yyjson as a dependency with add_subdirectory (either directly, with FetchContent or by any other means). In my case I would like to keep the default value of CMAKE_CONFIGURATION_TYPES which has more configurations

The fix is to avoid changing CMAKE_BUILD_TYPE or CMAKE_CONFIGURATION_TYPES if yyjson is not the root CMake project. As far as I can see those variables are meant to be set only when building the library itself (but let me know if that's not correct)

Tested by generating and compiling yyjson with Visual Studio on Windows, both Debug and Release (Visual Studio 2022, CMake 3.25.1)

Let me know if you have another solution in mind

@AntonShalgachev
Copy link
Contributor Author

A note about the if(XCODE OR MSVC) check

It seems to me that the intention of this check was probably to check for Visual Studio (because this is a multi-config generator in CMake and MSVC is usually used with Visual Studio). However that wouldn't work for other generators. For example, when generating a project for Ninja with the MSVC compiler this condition would be true (but Ninja is a single configuration generator so setting CMAKE_CONFIGURATION_TYPES would have no effect). Also at least in my environment CMAKE_BUILD_TYPE is "Debug" by default, so it wouldn't be changed to "Release"... but that's a different story

I haven't addressed that in this PR (because it doesn't affect me), so I'm just pointing this out for the future

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Merging #142 (af0f7e0) into master (e01ae9d) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #142   +/-   ##
=======================================
  Coverage   98.87%   98.87%           
=======================================
  Files           2        2           
  Lines        6594     6594           
=======================================
  Hits         6520     6520           
  Misses         74       74           

@ibireme ibireme merged commit 5128fc3 into ibireme:master Oct 15, 2023
37 checks passed
@ibireme
Copy link
Owner

ibireme commented Oct 15, 2023

Thanks!

@ibireme
Copy link
Owner

ibireme commented Oct 15, 2023

A note about the if(XCODE OR MSVC) check

I set the configuration to "Debug;Release" because Xcode and Visual Studio default to only these two configurations when creating a new project. It seems unnecessary, and I will remove it later.

@AntonShalgachev AntonShalgachev deleted the respect-cmake-configuration-types branch October 15, 2023 17:03
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