-
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
add lerc/2.1 #1159
add lerc/2.1 #1159
Conversation
All green in build 1 (
|
All green in build 2 (
|
+) | ||
+ | ||
+add_library(LercLib ${SOURCES}) | ||
+set_property(TARGET LercLib PROPERTY CXX_STANDARD 11) |
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.
The upstream cmake script requires the c++ 17 standard.
Is upstream too stringent?
Should self.settings.compiler.cppstd
be checked in configure()
?
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.
C++17 is not required, it's a mistake in their CMakeLists, or maybe they have introduced C++17 features in latest commits.
+ | ||
+add_library(LercLib ${SOURCES}) | ||
+set_property(TARGET LercLib PROPERTY CXX_STANDARD 11) | ||
+if(BUILD_SHARED_LIBS) |
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.
It would be nice if these things would be upstreamed.
The upstream cmake script has none of these options.
That way, we won't need patches in future releases.
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.
It doesn't look like upstream will answer soon.
If you're finished, ask for a final review so this can get merged.
All green in build 3 (
|
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.
If we have a problem in the future we can update this recipe. This PR is 1 month old, because we need an answer from the upstream
Specify library name and version: lerc/2.1
conan-center hook activated.