-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(protobuf): Add an option to remove RTTI from binaries #4682
feat(protobuf): Add an option to remove RTTI from binaries #4682
Conversation
I detected other pull requests that are modifying protobuf/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
Some configurations of 'protobuf/3.9.1' failed in build 1 (
|
It seems be a feature for protobuff project, not a patch only for Conan. Could you open a PR to Upstream ? |
Yes, I agree with you and I think it makes a lot of sense. Now, I just want to make sure of something: instead of having a "patch" of sorts, we want to have a clean CMake definition, correct? |
Some configurations of 'protobuf/3.9.1' failed in build 2 (
|
All green in build 3 (
|
Mmmh, why does it work now? I've been struggling all day yesterday trying to reproduce it with different compiler/configs, but I was never able to reproduce the issue. Any idea what changed? Was there a change in the |
All green in build 4 (
|
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.
Wait protocolbuffers/protobuf#8347 first
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.
Let's wait for the upstream decision.
If they merge, we can add similar change to the recipe.
Good news folks! The change has been accepted upstream. So IIRC, I should have a new option that sets the CMake definition in the Conanfile. For all versions before 3.16.0 (or whatever the version may be), if users request with_rtti=False, I should raise a ConanInvalidConfiguration? |
Git error in build 5:
|
Failed in build 6:
|
Failure in build 7 (
|
@uilianries > I know the SHA256 is not right. I will fix it as soon as I get it :) |
Some configurations of 'protobuf/3.15.4' failed in build 8 (
|
You should delete this option for the versions which do not support it. |
On top of the ConanInvalidConfiguration thingy? Or instead of it? If your answer to that question is "instead", wouldn't that be dangerous if users set it to |
You could add it on top if you want to, but it might be an overkill. This is an example, how you can delete options (just to make it easier for you): |
I didn't know about |
Some configurations of 'protobuf/3.15.4' failed in build 9 (
|
Failure in build 10 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Failure in build 11 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
…emoval of RTTI, remove it That also means there will be no CMake definition, and that Conan will crash if you request it.
Failure in build 12 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Failure in build 13 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
@floriansimon1 the error doesn't seem to be random, so closing/opening PR won't help, you need to investigate the problem. |
Looks like they dropped clang 3 support |
Yes, but I wanted to try a rebuild before wasting my time like last time where the failure was random and I spent a whole afternoon trying to replicate it. |
… with clang < 4 This change comes from #4776
All green in build 14 (
|
@uilianries > have your concerns been addressed? |
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.
Not it looks good! Thank you!
@mathbunnyru > can you maybe validate those changes if you think they're fine please? |
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.
Everything is good 👍
Protobuf (all versions)
conan-center hook activated.
Before pulling this PR, let's make sure this Protobuf PR gets rejected.