-
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
antlr4-cppruntime: Upgrade to 4.11.0/4.11.1 #12902
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The compiler errors reported by GCC 7 has been fixed by the PR antlr/antlr4#3885 which is pending review and approval. The minimum compiler requirement will be restored from GCC 8 to GCC 7. Depending upon when the next version of antlr4 will be released, I may choose to include a patch file that fixes the compiler errors to ship
|
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.
Hooks produced the following warnings for commit a73153dantlr4-cppruntime/4.10
antlr4-cppruntime/4.9.3
antlr4-cppruntime/4.10.1
antlr4-cppruntime/4.11.1
antlr4-cppruntime/4.11
|
there are some annotations made by the linter to support conan v2. @0xFireWolf would you mind having a look at them? thank you! |
…ld be `192` instead of `1920`.
…ache variable set in the toolchain.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 9f95ef0antlr4-cppruntime/4.10.1
antlr4-cppruntime/4.9.3
antlr4-cppruntime/4.11.1
|
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
Looking at: https://github.com/conan-io/conan-center-index/blob/master/docs/policy_patching.md |
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.
Fantastic! One minor quibble, but otherwise looks golden.
…ictionary. The first `check_min_vs()` call already ensures that MSVC supports C++17.
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!
The patching being applied here is really in a grey area for me. It's a really tough call and my opinion is not the final word
Now, looking at this conversation and how it has changes for the last month for almost 2 hours I have some suggestions and comments. 😻 💟 ❤️ thank you so much for the time and patients getting this PR updated ❤️ 💟 😻I absolutely agree this patch at one point could be close to the "working code" requirement. However I think a few things have changed in the last two weeks since I started reviewing this PR so I am on the other side of the fence. Let me break down my perspective. Problem 1: Antlr was not using UUID
"its not being use by this library" does not mean the Conan consumer is not expecting it to be present. I absolutely understand this respects the correctness of this libary, but as a ConanCenter contributors my focus is more on the consumers who rely on the packages. How would they see this change if it broke their project? Problem 2: ARM Support
It does in fact work. https://packages.ubuntu.com/bionic/uuid-devhas been shipping for a while on ARM but the recipe just had not been updated to work there. The last PR published packages for it https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/13130/1/libuuid/1.0.3//summary.json so it should be availble now. Again this was added the well after PR was started, the comments made were accurate but the speed at which we are moving is really exciting to see 🚀 so it could be reconsidered. Problem 3: New Generator Support
As for things not working with the new generators https://github.com/conan-io/conan-center-index/blob/d9ad528b30032bb01c5d522224e5b604c6fa6501/recipes/libuuid/all/conanfile.py As for the build problem that was encountered, I am pretty sure it's related to this conan-io/conan#11962 I think it would be worth giving it a second shot with this new information. 😸 I 100% think the work that was done was right, you've all done fantastic work |
Hooks produced the following warnings for commit d9cc6f5antlr4-cppruntime/4.10.1
antlr4-cppruntime/4.9.3
antlr4-cppruntime/4.11.1
|
@prince-chrismc First of all, I want to thank you for the thoughtful comments and suggestions. RE: Problem 1: Antlr was not using UUID
This does not make sense to me at all. If developers need
I duly understand that you care a lot about your consumers, but to some extent your comments make me feel like you are being overprotective of them. Yes, they will observe that their projects cannot be compiled if they import To be honest, I didn't even know that RE: Problem 2: ARM Support I sincerely thank you for the information because I don't have the time to keep track of other recipes. RE: Problem 3: New Generator Support Your comment conan-io/conan#11962 (comment) posted 8 hours ago indicated that the workaround did not fix all the issues. As such, implementing this hacky workaround for the generator of a package which is actually not used by My conclusion I am not inclined to add Thank you and I hope that you can understand my position. |
Suggestion of way of resolving this conflict:
|
Asking the Conan team for a second opinion, they can be the tie breaker 🤞 I am completely on the fence and my personal bias around patches is not a deciding factor 😄 |
Okay, this is a really hard one. So thanks for working with me on it. Let me capture our reasoning as precedence Our criteria for patches is "makes working software" the patches in question do not reach that (per say), they do however allow building more easily. Given this is aligned with the projects expectations, combined with the uncertainty of |
Specify library name and version: antlr4-cppruntime/4.11.0, 4.11.1
This PR contains the following changes:
libuuid
as of 4.11.0.libuuid
.The minimum version of GCC is now 8 (previously 7) to target C++17.(Will be reverted; See Below.)Thank you.