-
-
Notifications
You must be signed in to change notification settings - Fork 425
use built-in FindOpenGL module for GLES with cmake 3.23+ #929
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
base: master
Are you sure you want to change the base?
use built-in FindOpenGL module for GLES with cmake 3.23+ #929
Conversation
bae1912 to
2a79f95
Compare
2a79f95 to
f86ffa5
Compare
|
Probably needs to be done in the same way in the config package script here. I also always forget about this file, as seen in the 4.1.6 PR 😆 Guess you could also squash the changes into a single commit before merging, as it's only a few lines. |
kblaschke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add the version check conditional around the same line in the CMake package config file.
ecfb4c8 to
09941b5
Compare
kblaschke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small details left.
|
|
||
| # We use a local find script for OpenGL::GLES3 until the proposed changes are merged upstream. | ||
| list(APPEND CMAKE_MODULE_PATH "${PROJECTM_SOURCE_DIR}/cmake/gles") | ||
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.22" OR CMAKE_SYSTEM_NAME STREQUAL Android) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked CMake's sources to be sure and saw there that the GLES2 and GLES3 targets were actually added to CMake's find module in version 3.27 and not in v3.23 as I was expecting when I wrote the ReadMe in the module dir. So I think to be on the safe side, this should rather read:
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.22" OR CMAKE_SYSTEM_NAME STREQUAL Android) | |
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.26" OR CMAKE_SYSTEM_NAME STREQUAL Android) |
Ditto in the package config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If you want, you can update the ReadMe.md in cmake/gles accordingly.)
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.22" OR CMAKE_SYSTEM_NAME STREQUAL Android) | ||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") | ||
| endif() | ||
| find_dependency(OpenGL COMPONENTS GLES3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better only temporarily append the current dir to the module path, like this:
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.22" OR CMAKE_SYSTEM_NAME STREQUAL Android) | |
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") | |
| endif() | |
| find_dependency(OpenGL COMPONENTS GLES3) | |
| if (CMAKE_VERSION VERSION_LESS_EQUAL "3.22" OR CMAKE_SYSTEM_NAME STREQUAL Android) | |
| set(PROJECTM4_PREV_MODULE_PATH ${CMAKE_MODULE_PATH}) | |
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") | |
| find_dependency(OpenGL COMPONENTS GLES3) | |
| set(CMAKE_MODULE_PATH ${PROJECTM4_PREV_MODULE_PATH}) | |
| else() | |
| find_dependency(OpenGL COMPONENTS GLES3) | |
| endif() |
This will still allow the calling CMake project to override the find module, but will load projectM's module before the CMake-supplied one, and doesn't add unexpected paths to the module dir list.
No description provided.