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

CMakeify #85

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

CMakeify #85

wants to merge 2 commits into from

Conversation

ekg
Copy link

@ekg ekg commented Mar 14, 2019

This adds CMakeLists.txt to make it easier to build a static library from lodepng and also the examples. I don't compile all of them. I'm worried that the dependency on SDL might be difficult in some settings when all that's needed is the static library, but I am compiling some of them because this can be an important test for correct build system configuration.

Compile with: cmake -H. -Bbuild -DCMAKE_BUILD_TYPE=Release && cmake --build build -- -j 3

@lvandeve
Copy link
Owner

Thanks for the suggestion.

I agree with the point about the SDL dependency: LodePNG is a dependency-less library, while the SDL example is just a programming example.

This makelists is not the direction I'd like to go with this library (the examples better serve only as programming source code examples to e.g. copypaste from, having cmake build them does not do much to aid in serving as source code example :)), in general it's not actually intended as a dynamic library but as a C or C++ source file to use in your project.

Only two source files really matter: lodepng.cpp (which you can rename to lodepng.c for C, does any build system support this transformation?) and lodepng.h.

The main intended use case is adding the source file directly (or indirectly as module to keep version up to date) to your project and adding them to your own makefile (or other build type or IDE project file type you're using) in your project.

Does that work for you?

@ekg
Copy link
Author

ekg commented Mar 17, 2019 via email

@lvandeve
Copy link
Owner

lvandeve commented Mar 17, 2019

Copy pasting code is not an ideal situation

I was referring to the examples for this. All of the examples are small binary demo-programs that don't have a lot of real world functionality other than demonstrating code (they generate test images and things like that).

In a project using LodePNG, you would have to call LodePNG's API from your own project. These examples show how to call its API, and allow copypasting (and modifying) their code in your project where you use LodePNG's API. You would be unable to use those examples without involving copypasting in the first place, since the examples have a main function, so they can't be included directly in your project, which would normally have its own main function.

I've seen a cmake file of somebody's project that uses LodePNG, and all it contained, in their own cmake file, was the following, and this was sufficient to use it:

add_library(lodepng STATIC
lodepng/lodepng.cpp
lodepng/lodepng.h
)

So this is something easy to add to your own cmake file in your own project instead. Not everybody uses cmake (it is indeed a very popular one currently though, true that!) so it is not of help to everybody.

A cmake file certainly makes sense if LodePNG would be an actual program or library that you would compile as standalone. But LodePNG isn't standalone and cannot compile on its own (no main function, no dynamic link target). Instead, it's source files you can use in your own project and your own make system, no matter what system it is (make, cmake, bazel, visual studio project, ...)

Does this sound reasonable?

EDIT: or do you mean you would like a cmake file for the examples because you'd actually like to work in the examples themselves and easily compile them using cmake? In that case it makes sense (in the examples directory itself them), but for now most examples have a g++ compile command in a comment at the top, is using g++ or clang++ directly an option?

@ekg
Copy link
Author

ekg commented Mar 17, 2019 via email

@lvandeve
Copy link
Owner

It still is so that it's is a single source file, so extremely easy to add to your own cmake file :)

There are also so many compile time options possible for this (such as, #defines to disable parts of the code to build for very small systems, and the option to rename the file to .c), it would be almost impolite to propose one set in a (cmake) makefile, and on the other hand, a lot of work and maintenance to make targets for all those different options in a makefile.

It's really intended as an "include in source" library.

@stinos
Copy link

stinos commented Mar 18, 2019

Good point about the compile time options. And then there's the extra time spent on documentation (which is absent in this PR btw), keeping the CMakeLists up-to-date, then having to explain why that was chosen and not a makefile which is going to be the next thing asked for, etc. I'm also not sure the 'but CMake is time-saving' is true. Or at least not in all cases. The only thing one has to do now is get the source (1 to 2 commandline commands with git), add 1 source file to the build system used and possibly instruct it with a new include path depending on where the source is. Depending on how CMake is used, that is not more work but less, I think?

@Johnex
Copy link

Johnex commented Mar 27, 2019

Honestly i am with @lvandeve on this one. You will need to add some lines to your own CMake files to even include the shared or dynamic library. Instead just link directly to the source files. It's the same amount of work.

You can even make cmake download the latest LodePNG source files (no need to copy paste) using ExternalProject_Add and find_package(Git) automatically.

On top of that, your CMakeLists is only targeting linux, on windows static libraries are called .lib

@cosinekitty
Copy link
Contributor

I also vote for leaving out CMake support. The thought is nice, but this is a case where less is more. It would add to the overhead of the project owner to maintain and support something that is not central to the purpose of lodepng. I wish more projects would hold to a discipline of minimalism like lodepng does.

Copy link

@aleksey-nikolaev aleksey-nikolaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main idea was good. Having a cmake file can help to build/test and integrate to other project, but I'm sorry, but the current example is ugly.

  1. exclude examples from main cmakelists
  2. add install for library, configuration and examples (optional).

set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3")

# Project's name
project(lodepng)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One project(lodepng) should be enough

Comment on lines +18 to +24
set(CMAKE_BINARY_DIR ${CMAKE_SOURCE_DIR}/bin)
set(EXECUTABLE_OUTPUT_PATH ${CMAKE_BINARY_DIR})
set(LIBRARY_OUTPUT_PATH ${CMAKE_SOURCE_DIR}/lib)

# The following folder will be included
include_directories("${PROJECT_SOURCE_DIR}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-no-no don't do this. Export configuration instead

Comment on lines +28 to +67
add_executable(example_4bit_palette ${CMAKE_SOURCE_DIR}/examples/example_4bit_palette.cpp)
target_link_libraries(example_4bit_palette "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_4bit_palette PROPERTIES OUTPUT_NAME "example_4bit_palette")
add_dependencies(example_4bit_palette lodepng)
add_executable(example_bmp2png ${CMAKE_SOURCE_DIR}/examples/example_bmp2png.cpp)
target_link_libraries(example_bmp2png "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_bmp2png PROPERTIES OUTPUT_NAME "example_bmp2png")
add_dependencies(example_bmp2png lodepng)
add_executable(example_decode ${CMAKE_SOURCE_DIR}/examples/example_decode.cpp)
target_link_libraries(example_decode "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_decode PROPERTIES OUTPUT_NAME "example_decode")
add_dependencies(example_decode lodepng)
add_executable(example_encode ${CMAKE_SOURCE_DIR}/examples/example_encode.cpp)
target_link_libraries(example_encode "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_encode PROPERTIES OUTPUT_NAME "example_encode")
add_dependencies(example_encode lodepng)
add_executable(example_encode_type ${CMAKE_SOURCE_DIR}/examples/example_encode_type.cpp)
target_link_libraries(example_encode_type "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_encode_type PROPERTIES OUTPUT_NAME "example_encode_type")
add_dependencies(example_encode_type lodepng)
add_executable(example_gzip ${CMAKE_SOURCE_DIR}/examples/example_gzip.cpp)
target_link_libraries(example_gzip "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_gzip PROPERTIES OUTPUT_NAME "example_gzip")
add_dependencies(example_gzip lodepng)
add_executable(example_optimize_png ${CMAKE_SOURCE_DIR}/examples/example_optimize_png.cpp)
target_link_libraries(example_optimize_png "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_optimize_png PROPERTIES OUTPUT_NAME "example_optimize_png")
add_dependencies(example_optimize_png lodepng)
add_executable(example_png2bmp ${CMAKE_SOURCE_DIR}/examples/example_png2bmp.cpp)
target_link_libraries(example_png2bmp "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_png2bmp PROPERTIES OUTPUT_NAME "example_png2bmp")
add_dependencies(example_png2bmp lodepng)
add_executable(example_png_info ${CMAKE_SOURCE_DIR}/examples/example_png_info.cpp)
target_link_libraries(example_png_info "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_png_info PROPERTIES OUTPUT_NAME "example_png_info")
add_dependencies(example_png_info lodepng)
add_executable(example_reencode ${CMAKE_SOURCE_DIR}/examples/example_reencode.cpp)
target_link_libraries(example_reencode "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_reencode PROPERTIES OUTPUT_NAME "example_reencode")
add_dependencies(example_reencode lodepng)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why here? This should be in the example folder in separate cmakelists.txt

Comment on lines +60 to +63
add_executable(example_png_info ${CMAKE_SOURCE_DIR}/examples/example_png_info.cpp)
target_link_libraries(example_png_info "${LIBRARY_OUTPUT_PATH}/liblodepng.a")
set_target_properties(example_png_info PROPERTIES OUTPUT_NAME "example_png_info")
add_dependencies(example_png_info lodepng)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_executable(example_png_info example_png_info.cpp)
target_link_libraries(example_png_info lodepng)
  • that is all you need move this to examples/cmakelists.txt

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

Successfully merging this pull request may close these issues.

6 participants