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

Provide CMake package #453

Merged
merged 4 commits into from
Feb 4, 2024
Merged

Provide CMake package #453

merged 4 commits into from
Feb 4, 2024

Conversation

cNoNim
Copy link
Contributor

@cNoNim cNoNim commented Jan 30, 2024

Closes #452

@cNoNim cNoNim marked this pull request as ready for review January 30, 2024 20:55
@cNoNim
Copy link
Contributor Author

cNoNim commented Jan 30, 2024

I tested on Windows and Linux.

tested by

  • installing the module via
pip install .
  • manually copying GladConfig.cmake to $PREFIX/lib/cmake/Glad
find_package(Glad REQUIRED)

glad_add_library(glad_gl_core_33 REPRODUCIBLE API gl:core=3.3)

Also tested by

  • manual copy sources to ${PROJECT_SOURCE_DIR}/external/glad/ of sample project
set(GLAD_SOURCES_DIR "${PROJECT_SOURCE_DIR}/external/glad/")
add_subdirectory("${GLAD_SOURCES_DIR}/cmake" glad_cmake)

glad_add_library(glad_gl_core_33 REPRODUCIBLE API gl:core=3.3)

All works

@cNoNim cNoNim mentioned this pull request Jan 30, 2024
@Dav1dde Dav1dde merged commit b3a3644 into Dav1dde:glad2 Feb 4, 2024
1 check passed
@spnda
Copy link
Contributor

spnda commented Feb 22, 2024

When using the following CMake snippet in my local project, CMake will fail saying it can't find glad_add_library. Though, interestingly enough, this only happens for the CMake configurations which use MSVC as the compiler.

add_subdirectory("${FASTGLTF_DEPS_DIR}/glad/cmake" glad_cmake)

# Let the glad script generate the glad headers
glad_add_library(fg_glad_gl46 REPRODUCIBLE EXCLUDE_FROM_ALL LOADER API gl:core=4.6)

However, if I change the cmake/CMakeLists.txt file like this, it works again as it did before.

-list(APPEND CMAKE_PREFIX_PATH ${GLAD_SOURCES_DIR})
-find_package(Glad REQUIRED)
+include(./GladConfig.cmake)

Should I make this into an issue or provide the PR to fix this? Note that I am on Windows using CLion and CMake 3.27.8.

@cNoNim
Copy link
Contributor Author

cNoNim commented Feb 22, 2024

Consider opening an issue.
The logic described in the wiki suggests the need to set GLAD_SOURCES_DIR, but it may not be necessary.
If it is unnecessary, it can be fixed as you suggest. Keep in mind that GLAD_SOURCES_DIR defines the working directory.

"Directory containing glad sources (python modules), used as working directory. Must be absolute."

@cNoNim
Copy link
Contributor Author

cNoNim commented Feb 22, 2024

set(GLAD_SOURCES_DIR "${FASTGLTF_DEPS_DIR}/glad")
add_subdirectory("${GLAD_SOURCES_DIR}/cmake" glad_cmake)

I assume this is how it will work.

@cNoNim
Copy link
Contributor Author

cNoNim commented Feb 22, 2024

I now understand why everything worked without specifying the path.
In this case, replacing 'find_package' with 'include' seems correct.
I apologize for the regression.

@spnda
Copy link
Contributor

spnda commented Feb 22, 2024

set(GLAD_SOURCES_DIR "${FASTGLTF_DEPS_DIR}/glad")
add_subdirectory("${GLAD_SOURCES_DIR}/cmake" glad_cmake)

I assume this is how it will work.

That would not have made any difference because glad's CMakeLists.txt defines it too. So setting it or not setting it should make absolutely no difference as far as I can tell.

I now understand why everything worked without specifying the path.
In this case, replacing 'find_package' with 'include' seems correct.
I apologize for the regression.

Shall I open a pull request? Are you 100% positive it doesn't induce any other issues?

@cNoNim
Copy link
Contributor Author

cNoNim commented Feb 22, 2024

I thought that find_package and CMAKE_PREFIX_PATH were the same as include.
However, exist issue https://gitlab.kitware.com/cmake/cmake/-/issues/16644.
Therefore, using include is more consistent with the previous behavior.

I don't mind PR. I guess @Dav1dde feels the same way.

And thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide cmake-package
3 participants