ibireme / yyjson

The fastest JSON library in C
https://ibireme.github.io/yyjson/doc/doxygen/html/
MIT License
2.98k stars 262 forks source link

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

Closed AntonShalgachev closed 9 months ago

AntonShalgachev commented 9 months ago

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 commented 9 months ago

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[bot] commented 9 months ago

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 commented 9 months ago

Thanks!

ibireme commented 9 months ago

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.