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

Make local build with Imath #1219

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

theta682
Copy link
Contributor

@theta682 theta682 commented Jan 18, 2022

add_subdirectory(${IMATH_ROOT} Imath)
add_subdirectory(${OPENEXR_ROOT} OpenEXR)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 18, 2022

CLA Signed

The committers are authorized under a signed CLA.

@theta682
Copy link
Contributor Author

@cary-ilm can you merge this PR?

@cary-ilm cary-ilm requested a review from kdt3rd January 18, 2022 22:44
@cary-ilm
Copy link
Member

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.

@kthurston
Copy link

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

@theta682
Copy link
Contributor Author

Instead of Imath::Imath can be used Imath::ImathConfig. However, it is incorrectly exported as Imath::Config at https://github.com/AcademySoftwareFoundation/Imath/blob/master/config/CMakeLists.txt#L31

@lgritz
Copy link
Contributor

lgritz commented Jan 19, 2022

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.

@meshula
Copy link
Contributor

meshula commented Jan 19, 2022

I'm a fan of @lgritz's suggestion for a Imath::Headers target.

@theta682
Copy link
Contributor Author

The issue with old code I can't build when I have both Imath and OpenEXR added in my own CMake script with add_directory. OpenEXRCore either can't find ImathConfig.h or half.h

```
add_subdirectory(${IMATH_ROOT} Imath)
add_subdirectory(${OPENEXR_ROOT} OpenEXR)
```

Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
@theta682 theta682 changed the title Simplify linking in CMake Make local build with Imath Jan 19, 2022
@theta682
Copy link
Contributor Author

@lgritz @meshula @kthurston see temporary workaround

@meshula
Copy link
Contributor

meshula commented Jan 19, 2022

@theta682 are you referring to the latest push? i.e. this bit?

 if(NOT TARGET Imath::ImathConfig AND TARGET Imath AND TARGET ImathConfig)
     get_target_property(imathinc Imath INTERFACE_INCLUDE_DIRECTORIES)
     get_target_property(imathconfinc ImathConfig INTERFACE_INCLUDE_DIRECTORIES)
     list(APPEND imathinc ${imathconfinc})
     set(IMATH_HEADER_ONLY_INCLUDE_DIRS ${imathinc})
     message(STATUS "Imath interface dirs ${IMATH_HEADER_ONLY_INCLUDE_DIRS}")

I think that works.

@theta682
Copy link
Contributor Author

@kdt3rd can you merge this?

@cary-ilm
Copy link
Member

Apologies for the delay, merging now.

@cary-ilm cary-ilm merged commit 854f013 into AcademySoftwareFoundation:master Jan 27, 2022
@theta682 theta682 deleted the patch-1 branch January 27, 2022 22:32
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Apr 2, 2022
```
add_subdirectory(${IMATH_ROOT} Imath)
add_subdirectory(${OPENEXR_ROOT} OpenEXR)
```

Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
@cary-ilm cary-ilm mentioned this pull request Apr 2, 2022
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Apr 2, 2022
```
add_subdirectory(${IMATH_ROOT} Imath)
add_subdirectory(${OPENEXR_ROOT} OpenEXR)
```

Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
cary-ilm pushed a commit that referenced this pull request Apr 7, 2022
```
add_subdirectory(${IMATH_ROOT} Imath)
add_subdirectory(${OPENEXR_ROOT} OpenEXR)
```

Signed-off-by: Timothy Lyanguzov <timothy.lyanguzov@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants