CCI PR 24857 Suggestions for Review

[uilianries]

Instead doing repetitive reviews for the PR 24857, I preferred to send this patch with my suggestions instead:

• Json is mandatory requirement, do not need an option: https://github.com/getml/reflect-cpp/blob/v0.14.0/CMakeLists.txt#L83
• Use CMakeToolchain.cache_variables instead of cli args
• Use the latest version available in Conan Center for each dependency
• Raise invalid configuration in case cppstd or compilers do not fit
• Follow options vs cmake definitions
• Fix shared option definition to REFLECTCPP_BUILD_SHARED
• Use version range to install CMake as tool requires
• Use cmake_layout to define the source folder
• Add VirtualBuildEnv to load cmake environment
• Copy license file to package folder
• Remove upstream cmake config file from package. Use CMakeDeps instead.
• Add libm as system requirement. Used by src/yyjson.c:10:#include <math.h>
• Simplify test package to consume only mandatory components
• Validated on Linux + GCC13

[liuzicheng1987]

@uilianries, to be honest, I don't think this is a good idea:

tc.cache_variables["REFLECTCPP_BUILD_SHARED"] = self.options.shared
tc.cache_variables["REFLECTCPP_USE_BUNDLED_DEPENDENCIES"] = False
tc.cache_variables["REFLECTCPP_USE_VCPKG"] = False
tc.cache_variables["REFLECTCPP_CBOR"] = self.options.with_cbor
tc.cache_variables["REFLECTCPP_FLEXBUFFERS"] = self.options.with_flatbuffers
tc.cache_variables["REFLECTCPP_MSGPACK"] = self.options.with_msgpack
tc.cache_variables["REFLECTCPP_TOML"] = self.options.with_toml
tc.cache_variables["REFLECTCPP_XML"] = self.options.with_xml
tc.cache_variables["REFLECTCPP_YAML"] = self.options.with_yaml

The problem here is that the CMakeFile will then rely on vcpkg to try and install these libraries. I don't think Conan should be relying on vcpkg for dependency management.

More importantly, it's also unnecessary. reflectcpp is simply a reflection-based layer on top of these libaries. As long as they are installed somewhere, it will work.

Is there any particular reason you wanted it to be that way?

[liuzicheng1987]

@uilianries , never mind, you explicitly turned off vcpkg. Sorry for the confusion.

[uilianries]

@liuzicheng1987 The vcpkg should not be used, because REFLECTCPP_USE_VCPKG is "OFF".

All dependencies will be solved by Conan, and it will load a cmake toolchain file automatically, to inject those dependencies. Am I missing something?

[liuzicheng1987]

@uilianries , no, you are not missing something...I missed that fact that you turned REFLECTCPP_USE_VCPKG off...sorry for the confusion.

[uilianries]

@liuzicheng1987 Thank you for reviewing!