-
Notifications
You must be signed in to change notification settings - Fork 626
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
Usability improvements for submodule use. #955
Conversation
|
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, although one of the cmake experts should also confirm.
Thank you for this! Are you able sign the CLA? Per ASWF policy, we also require that commits be signed, can you add a "signed-off-by" line to the commit? Thanks. |
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, I like these changes
Thanks for the super-timely review. I'm figuring out how to sign the CLA, I should know tomorrow. Will add the sign-off line then as well! |
@@ -159,9 +159,11 @@ endif() | |||
|
|||
option(OPENEXR_FORCE_INTERNAL_ZLIB "Force using an internal zlib" OFF) | |||
if (NOT OPENEXR_FORCE_INTERNAL_ZLIB) | |||
find_package(ZLIB QUIET) | |||
if(NOT TARGET ZLIB::ZLIB) |
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's surprising/not-surprising that find_package(ZLIB) doesn't know about aliases. One more corner case to think about when maintaining cmake scripts...
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.
Might be worth raising with CMake upstream. I don't know if there's an official policy for it, but it feels like the existence of the target should be checked because worst-case you'll redefine it.
@Anteru, any progress on the CLA? We'd like to include this in the 3.0 release. |
I just signed it, let me get the commit signed-off next. |
* Make all installation code conditional * Add OPENEXR_INSTALL for the base library, and OPENEXR_INSTALL_TOOLS which can be used to disable installing the tools * Improve ZLIB search logic -- if a ZLIB::ZLIB target is present, use that and skip searching for the ZLIB library * Rename INSTALL_OPENEXR* to OPENEXR_INSTALL* Signed-off-by: Matthäus G. Chajdas <[email protected]>
This was causing OpenEXR builds (since AcademySoftwareFoundation#955 was merged) to NOT INSTALL. Oh boy. Signed-off-by: Larry Gritz <[email protected]>
This was causing OpenEXR builds (since #955 was merged) to NOT INSTALL. Oh boy. Signed-off-by: Larry Gritz <[email protected]>
which can be used to disable installing the tools
that and skip searching for the ZLIB library