Skip to content
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

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Usability improvements for submodule use. #955

merged 1 commit into from
Mar 12, 2021

Conversation

Anteru
Copy link
Contributor

@Anteru Anteru commented Mar 8, 2021

  • 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*

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 8, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@cary-ilm cary-ilm left a 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.

@cary-ilm
Copy link
Member

cary-ilm commented Mar 8, 2021

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.

Copy link
Contributor

@lgritz lgritz left a 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

@Anteru
Copy link
Contributor Author

Anteru commented Mar 8, 2021

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)
Copy link
Contributor

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...

Copy link
Contributor Author

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.

@cary-ilm
Copy link
Member

@Anteru, any progress on the CLA? We'd like to include this in the 3.0 release.

@Anteru
Copy link
Contributor Author

Anteru commented Mar 11, 2021

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 <dev@anteru.net>
@cary-ilm cary-ilm merged commit 5b210d0 into AcademySoftwareFoundation:master Mar 12, 2021
lgritz added a commit to lgritz/openexr that referenced this pull request Mar 13, 2021
This was causing OpenEXR builds (since AcademySoftwareFoundation#955 was merged) to
NOT INSTALL. Oh boy.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
cary-ilm pushed a commit that referenced this pull request Mar 13, 2021
This was causing OpenEXR builds (since #955 was merged) to
NOT INSTALL. Oh boy.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@cary-ilm cary-ilm added the Build A problem with building or installing the library. label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build A problem with building or installing the library. v3.0.0-beta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants