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

[cmake] fix FindOpenEXR.cmake so it can deal with windows debug libs with _d suffix #1844

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Apr 27, 2022

Description of Change(s)

Default debug builds of OpenEXR 2.3.0 on windows create libraries with a "_d" suffix.

The current FindOpenEXR.cmake included with USD can't deal with this, and will therefore fail to find OpenEXR. This happens even with the supplied build_usd.py, if you try to build in debug mode on windows. This fix will work both with build_usd.py, and with cmake builds with a user supplied OpenEXR.

Fixes Issue(s)

  • USD can't find debug OpenEXR libs on windows

  • I have submitted a signed Contributor License Agreement

@meshula
Copy link
Member

meshula commented Apr 27, 2022

This is a good solution for debug builds.

Someday, not necessarily as part of this PR, it would be nice to engineer the find script to make an interface library, so that the debug and release information could both carried through the build. The use of _LIBRARIES convention stands in the way of creating vcproj files that can build for Release or Debug, but the interface library scheme does allow for it. OpenEXR 2.5 and greater set things up in the modern cmake way with namespaced OpenEXR and Imath interface libraries, and the script could pass that much richer information through as is. Unfortunately OpenEXR 2.4, which is in the vfxplatform until VFX2019, does not have this nicety, so we're going to need to maintain the find script for a few more years. Or issue a 2.4 maintenance release of OpenEXR.... hmmmm

@pmolodo
Copy link
Contributor Author

pmolodo commented Apr 27, 2022

Larry Gritz made a much nicer FindOpenEXR.cmake which creates imported targets for 2.4+, as part of OSL:

https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/blob/main/src/cmake/modules/FindOpenEXR.cmake

Would be nice to migrate to that, but it has it's own license, plus it would need to be verified to work with USD, yada yada.

It also currently doesn't really deal elegantly with the debug vs release libs things - it just adds in a check for libs with _d suffix, which is better than nothing, but doesn't do anything where it can find both, and attach them to targets based on the current config.

@jilliene
Copy link

Filed as internal issue #USD-7345

@pmolodo pmolodo force-pushed the pr/FindOpenEXR_debug branch from c00f058 to 9d325ac Compare November 21, 2024 23:49
@pmolodo
Copy link
Contributor Author

pmolodo commented Nov 21, 2024

Updated this because I was going through old PRs... but given that:

  • USD now only looks for the OpenEXR package if it can't find Imath
  • Currently recommended OpenEXR version (3.1) uses a separate Imath lib

...I actually think a better solution now might be to simply require OpenEXR >= 3.0, only look for the Imath package, and remove FindOpenEXR.cmake entirely, so we don't need to keep maintaining it.

@pmolodo pmolodo force-pushed the pr/FindOpenEXR_debug branch from 9d325ac to 6ffbed7 Compare November 22, 2024 00:46
@meshula
Copy link
Member

meshula commented Nov 22, 2024

Thank you for that note @pmolodo, you're correct that we don't need the Find script any more since OpenEXR now publishes working cmake config files!

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@asluk asluk added the build Build-related issue/PR label Dec 3, 2024
pixar-oss pushed a commit that referenced this pull request Dec 9, 2024
Updates OpenEXR dependency from v3.1.11 -> v3.1.13
- This was originally part of PR #3077 from @nvidia-jomiller

Removes FindOpenEXR.cmake because OpenEXR now publishes its own config scripts which supersede a Find script.

Fixes #585
Closes #1844
Closes #3077

(Internal change: 2349684)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-related issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants