Added support for reflect-cpp v0.14.0; added CBOR and TOML #24819
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks a lot for taking the time to add the new version.
I see that you deleted all the code for 0.11.0, which is something we should keep around. Do you think it's worth keeping? As it's a header-only library instead of being built, I see value on keeping it around, but I defer the decision to you :)
If you end up removing the version, please do so from the config.yml and conandata.yml files.
Also please check the linter errors, the missing test_package and the hardcoded version should be reverted :)
recipes/reflect-cpp/all/conanfile.py
Outdated
| dst=os.path.join(self.package_folder, "licenses"), | ||
| src=self.source_folder, | ||
| ) | ||
| copy( |
There was a problem hiding this comment.
What is this used for? Why is the CMakeLists needed when consuming this package?
There was a problem hiding this comment.
Well, how are supposed to build the package without the CMakeLists.txt?
|
Hi @AbrilRBS , thanks for your feedback. I am the main author of reflect-cpp, but I didn't upload any of the previous Conan packages. Other people did that without informing me. I found out about its existence, because users were complaining to me about the Conan package. As far as the tests are concerned: I frankly think the way they were set up in the Conan package before didn't make any sense. We have several hundreds of tests in our original Git repository, if you want to run these tests in the Conan package as well, I'd be happy to set up a proper test pipeline there. And sure, I will remove the hardcoded version. |
|
@AbrilRBS , here are the tests: https://github.com/getml/reflect-cpp/tree/main/tests I could either set up the JSON tests only (roughly 130 tests) or I could set up the tests for all supported formats (several hundred). JSON is the default format and the JSON tests are the most extensive, so there is a case to be made for running the JSON tests only. But let me know which option you prefer. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Changes not allowed in build 8: Only one library can be changed in the same PR. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Changes not allowed in build 8: Only one library can be changed in the same PR. |
|
@AbrilRBS, I'm stuck. So here's the problem: Like I said, I am the main author of reflect-cpp, but someone else uploaded the package to Conan without consulting me. In our CMakeLists.txt, we have always referred to the package as "reflectcpp" not "reflect-cpp": https://github.com/getml/reflect-cpp/blob/main/CMakeLists.txt But a requirement of the Conan package is that the name of the installed package be the same as the name of the package in the CMakeLists.txt. This didn't matter as long as the library was treated as header-only. But this was a workaround from the get-go and in any case it won't work for any of the newer versions. So my view is that the right course of action would be to rename the package "reflectcpp" in the Conan Center, but the Conan linter won't let me do that either. So what should I do now? Open a new PR for a new recipe called "reflectcpp"? |
|
@liuzicheng1987 Thank you for your PR and clarifying about the project name. Sorry hear that you were not informed about your project is packaged in Conan Center already. Renaming a package is not a simple operation, because the CI is not prepared to do it (yet), as packages are "immutable", and it's something rare to occur in ConanCenterIndex. But don't worry, I'll talk to @AbrilRBS to find a proper solution and rename it somehow. |
|
@uilianries , I could open a new PR for a new recipe called "reflectcpp" and tell people to use that in my documentation. We could then kind of deprecate the "reflect-cpp" recipe. |
|
@uilianries and @AbrilRBS , I have opened a new PR: #24857 |
|
Closing this PR as #24857 superseded this one. |
Summary
Changes to recipe: reflect-cpp/0.14.0
Motivation
The most recent version on Conan was 0.11, but the most recent version of the library is 0.14. There have been significant advances between these two versions.
Also, not all serialization formats supported by reflect-cpp were also supported in the Conan package.
Details
reflect-cpp is no longer a header-only library, and some of the changes made reflect that. I now compile the library using the official CMakeLists.txt.