Link to the OpenGL::GL target instead of OPENGL_LIBRARIES #1603
Conversation
pmolodo
force-pushed
the
pr/cmake-GL-abs-path
branch
3 times, most recently
from
b1648f1 to
c110ef3
Compare
|
Fixed an issue with requiring OpenGL on iOS builds. Refactored the way COMMON_LIBRARIES was used to make the iOS library changes clearer / grouped with other target_link_library statements |
The point of COMMON_LIBRARIES was apparently to allow all library linking to be done with a single command; however, continuing this paradigm becomes difficult as more complex conditions are built up - for instance, in MaterialXRenderMsl/CMakeLists.txt, we were altering COMMON_LIBRARIES if APPLE and NOT iOS... which, at best, made the name COMMON_LIBRARIES unclear. Refactored to simply make multiple calls to target_link_libraries, which is the way most other CMakeLists.txt in this repo handle this.
This prevents absolute paths from creeping into the output
MaterialXConfig.cmake, which makes it not portable.
Also, in general in modern CMake, targets are preferred over baked
paths. Plus, at least on Linux, ${OPENGL_LIBRARIES} includes libGLU,
which is not actually a required dependency
pmolodo
force-pushed
the
pr/cmake-GL-abs-path
branch
from
c110ef3 to
6d6a9cc
Compare
|
This looks like a good proposal, @pmolodo, and the one caveat is that we'll need to raise our minimum required version of CMake, which is currently set to 3.5: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/CMakeLists.txt#L8 We've been interested in raising this minimum version for quite a while, and one option would be to align with OpenImageIO and raise the minimum version of CMake to 3.15: https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/master/CMakeLists.txt#L5 Let's bring this up as a topic in the MaterialX channel of the ASWF Slack, so that we can gather consensus on what minimum version we should align with. |
|
Following up on a discussion on the MaterialX channel of the ASWF Slack, we've raised the minimum CMake version to 3.16: |
jstone-lucasfilm
changed the title
jstone-lucasfilm
merged commit b50dea8
into
AcademySoftwareFoundation:main
|
Great, thanks! |
This prevents absolute paths from creeping into the output MaterialXConfig.cmake, which makes it not portable.
Also, in general in modern CMake, targets are preferred over baked paths. Additionally, at least on Linux,
${OPENGL_LIBRARIES} includes libGLU, which is not actually a required dependency