lvandeve / lodepng

PNG encoder and decoder in C and C++.
zlib License
2.06k stars 421 forks source link

CMakeify #85

Open ekg opened 5 years ago

ekg commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 notifications@github.com 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 https://github.com/lvandeve/lodepng/pull/85#issuecomment-473660404, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI4EUSr0d32ifaBuVud7K9xCQZh8y4Zks5vXjNSgaJpZM4bzpk8 .

lvandeve commented 5 years ago

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 commented 5 years ago

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 notifications@github.com 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 https://github.com/lvandeve/lodepng/pull/85#issuecomment-473724256, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI4EY9WWlatWYpYYnWNYbEU5EavSzU1ks5vXskXgaJpZM4bzpk8 .

lvandeve commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.