-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
reflect-cpp: add version 0.10.0 #22574
Conversation
This comment has been minimized.
This comment has been minimized.
@toge wouldn't we need to at the very least add the requirements to the recipe? |
Anyone who would want to use them could just require them to their own project that uses reflect-cpp and wants those backends. Core reflect-cpp won't have issues compiling without the serialization libraries as backends unless you try to include its specific header for that format/backend. That said, I'm not sure what the best practice is for something like this. When I made the initial recipe I thought having them in would be helpful, but in that case we should probably keep being consistent with having them. Also, libbson in particular would be a bit awkward to require here, since it needs the newer one from mongo-c-driver, and the recipe for mongo-c-driver doesn't have a libbson-only option/mode. |
@RubenRBS any thoughts on this? |
Hi @RazielXYZ didn't realize you answered back, sorry for the delay, will have something for you tomorrow once I get back to reviewing PRs :) |
We have a similar recipe CCI with optional backend, so users need to include them as extra dependency. I would follow the same approach here, not installing these dependencies. Reading the headers and the cmake file, there is no mechanism like a ifdef or linkage that could break the project. |
So the suggestion is to remove the with_* options and requires from the recipe? Sounds fine to me |
Not exactly, let's keep with the current options to avoid breaking users. |
Should there be a separate PR for v0.8.0 that got out today? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can take an action over the options in another PR.
@lkotsonis Yes, please. Could you open a new PR? Otherwise we would need to re-build this PR, but is actually good to be merged. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
oh. |
Any update on this? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@RubenRBS If you think the options for dependand packages are mandatory(bson, toml, messagepack etc), I will add these options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@toge Thanks a lot for the PR, it looks pretty well! Only one thing before moving this forward. We have to report that patch upstream and wait for the official solution to this bug. Otherwise, we could report it and add an extra step at validation time for that temporary invalid configuration while waiting for the official patch.
@franramirez688 |
This comment has been minimized.
This comment has been minimized.
@franramirez688 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@toge Great job! Thanks a lot for taking the time to report the patch upstream 👏 That's really appreciated.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the upstream PR, it is supper appreciated! :)
Conan v1 pipeline ✔️All green in build 12 (
Conan v2 pipeline ✔️
All green in build 11 ( |
@RubenRBS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! Looks great now
Specify library name and version: reflect-cpp/0.10.0
reflect-cpp/0.7.0 supports bson and cbor, but this PR does not mention the two features.
Since reflect-cpp is a header-library and these features do not require extra definitions or configuration files, I do not see the need to add two options for them.