-
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
Update laszip to v3.4.4 #23731
Update laszip to v3.4.4 #23731
Conversation
This comment has been minimized.
This comment has been minimized.
49bffb8
to
1fb353e
Compare
This comment has been minimized.
This comment has been minimized.
Hi @tbeu thanks a lot for your patience, I've pushed the last changes needed to get this merged :) |
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.
Thank you for your PR! There is a small detail that will break with this new release:
The version 3.4.4 adds C++17 as requirement. The current recipe is not prepared to work with that, for instance, running older and incompatible compiler will ask for C++17 and will break.
LASzip/LASzip@3.4.3...3.4.4#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR4
As hotfix, I would suggest updating validate()
, based on the version and checking for C++17. As reference, you can use this recipe:
if self.settings.get_safe("compiler.cppstd"): |
Did you try to remove the C++17 requirement in the CMakeLists.txt via patch? I still could compile it on MSVC with /std:c++14 set. |
@tbeu In case the project is C++14 compatible, then you should take this information from upstream to be sure. All packages in ConanCenterIndex should follow as close as possible the upstream behavior, to avoid any surprises. So far, I only found a reference about moving to C++17 (LASzip/LASzip@cafdc6b). |
It's at least resolved upstream by LASzip/LASzip@0a20905. Not sure about any new release though. Should it rather be patched here? |
212eb74
to
cfee455
Compare
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 ✔️All green in build 7 (
Conan v2 pipeline ✔️
All green in build 7 ( |
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
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
Specify library name and version: laszip/3.4.4
This PR also reflects the license change of LASzip.