-
Notifications
You must be signed in to change notification settings - Fork 427
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
Add cmake support with cmake-config file #82
base: master
Are you sure you want to change the base?
Conversation
eb0e9e6
to
acc4305
Compare
I just realized that there are many cmakeification PRs here. Is this project not active anymore? |
not dead, @lvandeve still commits, but I think he is not interested in cmake support |
In the end, it's just a CMakeLists.txt. It's very helpful when integrating this into other projects. |
Add full cmake support. The project can either be used with `add_subdirectory` or be installed into the system (or some other directory) and be found with `find_package(lodepng)`. In both cases the cmake target `lodepng::lodepng` is all that needs to be linked. Having an install target also makes packaging easier. - add libraries - loadepng - lodepng_util - add executables (also installed) - pngdetail - generate lodepng-config.cmake and install in lib/cmake/lodepng - add unittest and target `test` (enabled by flag ENABLE_TESTING) - add benchmark (enabled by flag BUILD_BENCHMARK) - add examples (enabled by flag BUILD_EXAMPLES) note: I was only able to install SDL2 from my package manager, but benchmark, example_opengl and example_sdl use SDL (1)
acc4305
to
c698025
Compare
@aleksey-nikolaev could you review my attempt at adding CMake support for lodepng? Anything missing here? |
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.
I think you need to refactoring your project first of all
option(ENABLE_TESTING "enable creation of unittest" ON) | ||
if(ENABLE_TESTING) | ||
enable_testing() | ||
add_executable(${CMAKE_PROJECT_NAME}_unittest lodepng_unittest.cpp) |
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.
You can move this block to the tests/CMakeList.txt
. I think, it will be much readable if your main folder will not contain tests or any other unnecessary code.
option(BUILD_BENCHMARK "build benchmark for ${CMAKE_PROJECT_NAME}, requires SDL2" OFF) | ||
if(BUILD_BENCHMARK) | ||
add_executable(${CMAKE_PROJECT_NAME}_benchmark lodepng_benchmark.cpp) | ||
target_link_libraries(${CMAKE_PROJECT_NAME}_benchmark PRIVATE ${CMAKE_PROJECT_NAME}::${CMAKE_PROJECT_NAME}) |
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.
As for the tests move this to the benchmark/CMakeList.txt
add_executable(${CMAKE_PROJECT_NAME}_benchmark lodepng_benchmark.cpp) | ||
target_link_libraries(${CMAKE_PROJECT_NAME}_benchmark PRIVATE ${CMAKE_PROJECT_NAME}::${CMAKE_PROJECT_NAME}) | ||
# SDL2 Dependency | ||
find_package(SDL2 CONFIG REQUIRED) |
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.
I don't know what SDL2
is, but are you shure that when system will not contain this library the next if
will work as expected?
if (TARGET SDL2::SDL2) | ||
message(STATUS "using TARGET SDL2::SDL2") | ||
else() | ||
message(STATUS "no TARGET SDL2::SDL2, or SDL2, using variables") |
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.
How can you get here? Did you expect an older version of this library without SDL2::SDL2
? If so, maybe it should be better to create your own SDL2Find.cmake
instead of using SDL2_INCLUDE_DIRS
directly.
else() | ||
message(STATUS "no TARGET SDL2::SDL2, or SDL2, using variables") | ||
endif() | ||
foreach(example |
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.
This for
do nothing, isn' it?
project(lodepng) | ||
|
||
# define target to install and link tests against | ||
add_library(${CMAKE_PROJECT_NAME} |
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.
Also you can use option(BUILD_SHARED_LIBS "Build ${CMAKE_PROJECT_NAME} as shared library" OFF)
if you care about type of library to being build
Add full cmake support. The project can either be used with
add_subdirectory
or be installed into the system (or some otherdirectory) and be found with
find_package(lodepng)
. In both cases thecmake target
lodepng::lodepng
is all that needs to be linked.Having an install target also makes packaging easier.
test
(enabled by flag ENABLE_TESTING)note: I was only able to install SDL2 from my package manager, but
benchmark, example_opengl and example_sdl use SDL (1)
If you have any questions about this commit I'd be happy to clear out any misunderstandings