-
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
CMakeify #85
base: master
Are you sure you want to change the base?
CMakeify #85
Conversation
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? |
Copy pasting code is not an ideal situation. I'd like to keep things
organized by referring to a repository and commit or tag when building
code. I think others have good reasons to use cmake as well. In the end you
just include a single new source file to specify the build, and it won't
affect the simpler copy paste use you have in mind.
…On Sun, Mar 17, 2019, 12:21 Lode Vandevenne ***@***.***> wrote:
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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#85 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAI4EUSr0d32ifaBuVud7K9xCQZh8y4Zks5vXjNSgaJpZM4bzpk8>
.
|
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 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? |
I think the many PRs about this show that many people expect this project
to provide a build function that produces a library that can be linked into
another application. It seems trivial to do this, but some time can be
saved if it is already done. This multiplied by hundreds or thousands of
people per year is a substantial amount of work.
If I were you I would be collecting as many kinds of build scripts as
possible. In general, it only saves time to have exact documentation of how
to build something. With cmake you get as close to a module system as
widely exists in C++, and maybe that's why you have so many PRs proposing
cmake build scripts. Even a makefile would be helpful. They take time to
write no matter how simple they are. In the end somewhere your users are
having to write their own build commands for this library.
I should also be clear that I'm interested in this because your library has
really helped me by providing an efficient and dependency-free way to write
PNG images. I think everyone in need of a simple interface to raster images
should be using it. Kudos.
…On Sun, Mar 17, 2019, 23:00 Lode Vandevenne ***@***.***> wrote:
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 make 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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#85 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAI4EY9WWlatWYpYYnWNYbEU5EavSzU1ks5vXskXgaJpZM4bzpk8>
.
|
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. |
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? |
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 |
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. |
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.
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.
- exclude examples from main cmakelists
- add install for library, configuration and examples (optional).
set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3") | ||
|
||
# Project's name | ||
project(lodepng) |
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.
One project(lodepng) should be enough
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}") | ||
|
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.
no-no-no don't do this. Export configuration instead
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) |
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.
Why here? This should be in the example folder in separate cmakelists.txt
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) |
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.
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
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