-
Notifications
You must be signed in to change notification settings - Fork 632
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
Make local build with Imath #1219
Conversation
theta682
commented
Jan 18, 2022
•
edited
Loading
edited
|
@cary-ilm can you merge this PR? |
Thanks for the contribution. I'd like to hear from @kdt3rd and @meshula, who might have some additional insight into why this was done this way. ASWF projects require "Developer Certificate of Origin" (DCO) which requires that each commit have a "signed-off-by" line. Can you click on the "Details" link on the "DCO" line in the list of checks above and follow the instructions for re-submitting the commit. Note that you can't just submit an additional commit, the existing commit needs to be replaced. Apologies in advance for the hassle. |
I am hesitant to accept this change (but appreciate the efforts to simplify!) Reasoning follows: We do not want to link against libImath.so, that would add a dependency on a c++ library to a C library, requiring libstdc++.so and such to be loaded when we can stay a pure C library. We only need the header files from Imath. However, it may be easy to use a generator expression and extract the include directories from imath, such as $<TARGET_PROPERTY:Imath::Imath,INTERFACE_INCLUDE_DIRECTORIES> but I have not tested that |
Instead of |
We could modify the exported cmake config to have an additional target, Imath::Headers that just has an include portion and doesn't pull in any library dependencies. |
I'm a fan of @lgritz's suggestion for a Imath::Headers target. |
d492fd7
to
c5c805f
Compare
The issue with old code I can't build when I have both |
``` add_subdirectory(${IMATH_ROOT} Imath) add_subdirectory(${OPENEXR_ROOT} OpenEXR) ``` Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
@lgritz @meshula @kthurston see temporary workaround |
@theta682 are you referring to the latest push? i.e. this bit?
I think that works. |
@kdt3rd can you merge this? |
Apologies for the delay, merging now. |
``` add_subdirectory(${IMATH_ROOT} Imath) add_subdirectory(${OPENEXR_ROOT} OpenEXR) ``` Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
``` add_subdirectory(${IMATH_ROOT} Imath) add_subdirectory(${OPENEXR_ROOT} OpenEXR) ``` Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
``` add_subdirectory(${IMATH_ROOT} Imath) add_subdirectory(${OPENEXR_ROOT} OpenEXR) ``` Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>