Please re-write/replace Makefile?

[hartwork]

Hi!

Thanks for making zopfli! I'm a happy user of zopflipng, was using optipng prior to that. Files get even smaller now, good work!

The current Makefile is not that large but does many things in suboptimal ways as demonstrated by this list of open tickets all related to the Makefile:

• 9 make file hardcodes compiler options

• 12 Don't assume -lm is required

• 68 Makefile does not properly handle multiple targets + parallel builds

• 69 Properly specify prerequisites in Makefile

• 82 Build issues on Darwin: so vs dylib

• 121 Installation in Linux

• 127 Makefile issue

• 133 Make the Makefile work as expected

• 135 make install for libzopfli{png}

On top of that

• LDFLAGS is piggy-bagged into CFLAGS
• some files are compiled multiple times, and own libraries are not linked to for re-use (on purpose)?

I would like to ask if there are plans to re-write, fix or replace the current Makefile by another tool. What would it take for a pull request to be accepted (besides the contribution agreement)? The least worse I'd use for a re-write is Autoconf + Automake, if that's welcome and understood as a (more high-level, more portable) improvement.

Has anyone checked @Hello71's Makefile patches at https://github.com/Hello71/zopfli/commit/8d51c5f9b954d93825fe7236cb652fcb1ac398fa, yet?

Thanks and best, Sebastian

[hartwork]

Anyone?

[lvandeve]

Thanks for summary of the issues.

I'm fine with a solution that is simple and conservative (with no dependencies and pure C90 it's rather simple to build. And using tools installed by default on most systems like make, and avoiding bringing in things specific to one platform)

I'd need some time to study all these things, so I can't promise an ETA right now, but it is interesting

[hartwork]

Any news?

[hartwork]

PS: How well does CMake do regarding your requirements?

[lvandeve]

Which issue is causing the most trouble and in which way?

Thank you

[hartwork]

On a higher level, the problem is that the build system is so broken that any Linux distro will have a hard time packaging zopfli without downstream repair that slows updates down and multiplies zero-return work that frustrates the very people that help get zopfli to the masses. For two examples, here's the (out-of-sync) patching that Debian and Gentoo do:

• https://sources.debian.org/patches/zopfli/1.0.2-1/
• https://github.com/gentoo/gentoo/tree/master/app-arch/zopfli/files

If you're aiming for the most economic fix I would think it either needs @Hello71 signing a contributor agreement and his https://github.com/Hello71/zopfli/commit/8d51c5f9b954d93825fe7236cb652fcb1ac398fa work reviewed and merged or a proper re-write of the build system once the right high-level tool has been selected.

What do you think?

[bobvanderlinden]

There was also https://github.com/google/zopfli/pull/58 that adds CMake to zopfli. It works well, as we're using that PR for packaging zopfli for NixOS.

[lvandeve]

I merged in #58, the CMake script, and updated the version numbers in it. I tested the script and it works great.

Does this solve the issue?

[bobvanderlinden]

For me it does. Thanks!

EDIT: I would suggest to remove Makefile and add instructions on how to build using cmake in the readme. Otherwise 2 different build systems need to be maintained.

[lvandeve]

Yes indeed, just waiting if there will be more feedback on this before removing the old one and finalizing this

[hartwork]

I merged in #58, the CMake script, and updated the version numbers in it. I tested the script and it works great.

Does this solve the issue?

Cool to see some progress.

Some things I found that need more work in my eyes:

• Installation of files to ${prefix:-/usr/local} seems to be missing. Related to listed #121 #135
• Custom CFLAGS/CXXFLAGS(/LDFLAGS?) flags are not supported properly, e.g. CFLAGS='-O2 -pipe' cmake . will get the C compiled code with -O2 -pipe -O3 -DNDEBUG where unwanted -O3 wins. Related to listed #9
• libzopfli and libzopflipng share zopflilib_obj objects but end up with their own copy. Why is libzopflipng not linking against libzopfli?
• I'm unsure if if(UNIX) target_link_libraries(libzopfli m) endif() is good enough for listed #12
• I would consider defaulting BUILD_SHARED_LIBS to ON, personally
• Having version 1.0.2 at two places has more risk of divergence than a single place

What I ran during inspection:

# cd "$(mktemp -d)"
# git clone https://github.com/google/zopfli.git
# cd zopfli/
# rm Makefile
# CFLAGS='-O2' CXXFLAGS='-Os' LDFLAGS='-Wl,--as-needed' cmake -DBUILD_SHARED_LIBS=ON .
# make VERBOSE=1 all

# find -name \*.so\*
./libzopfli.so
./libzopfli.so.1
./libzopfli.so.1.0.2
./libzopflipng.so
./libzopflipng.so.1
./libzopflipng.so.1.0.2

# lddtree libzopfli*.so.1.0.2
libzopflipng.so.1.0.2 (interpreter => None)
    libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libstdc++.so.6
        ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2
    libm.so.6 => /lib64/libm.so.6
    libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/libgcc_s.so.1
    libc.so.6 => /lib64/libc.so.6
libzopfli.so.1.0.2 (interpreter => None)
    libm.so.6 => /lib64/libm.so.6
        ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2
    libc.so.6 => /lib64/libc.so.6

# make DESTDIR="$PWD/DESTDIR" install
make: *** No rule to make target 'install'.  Stop.

[lvandeve]

Do you have an example of a project with a makefile or cmake which you find a delight to work with, as an example?

Thanks :)

[bobvanderlinden]

CFLAGS is being used but the optimization flag is overridden by the one set by it being a release build. I have not found a way to override this using CFLAGS, but it is still possible to override it using cmake: cmake -DCMAKE_C_FLAGS="-O0"

For -lm this might be more complete: https://github.com/Kitware/CMake/blob/master/Tests/LinkStatic/CMakeLists.txt#L8

It might be a good idea to add the version to project(Zopfli) and reuse that version elsewhere.

[jibsen]

Great feedback. I will try to make a few comments on some of the issues raised.

Installation of files to ${prefix:-/usr/local} seems to be missing. Related to listed #121 #135

Install target could be easily added with something like:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5d20e39..fd15f94 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,7 +1,9 @@
-cmake_minimum_required(VERSION 2.8)
+cmake_minimum_required(VERSION 2.8.6)

 project(Zopfli)

+include(GNUInstallDirs)
+
 option(BUILD_SHARED_LIBS "Build Zopfli with shared libraries" OFF)

 if(NOT CMAKE_BUILD_TYPE)
@@ -86,3 +88,15 @@ target_link_libraries(zopfli libzopfli)
 #
 add_executable(zopflipng src/zopflipng/zopflipng_bin.cc)
 target_link_libraries(zopflipng libzopflipng)
+
+#
+# install
+#
+install(TARGETS libzopfli libzopflipng zopfli zopflipng
+  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+)
+install(FILES include/zopfli.h include/zopflipng_lib.h
+  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
+)

Custom CFLAGS/CXXFLAGS(/LDFLAGS?) flags are not supported properly, e.g. CFLAGS='-O2 -pipe' cmake . will get the C compiled code with -O2 -pipe -O3 -DNDEBUG where unwanted -O3 wins. Related to listed #9

This appears to be how CMake works; it takes the contents of CFLAGS and uses that to initialize CMAKE_C_FLAGS, and then appends the flags for the build type, for instance CMAKE_C_FLAGS_RELEASE for a release build, which contains the -O3 you see.

Two options to work around this come to mind -- you could set these variables directly instead of using CFLAGS, as in:

LDFLAGS='-Wl,--as-needed' cmake -DBUILD_SHARED_LIBS=ON -DCMAKE_C_FLAGS_RELEASE='-O2' -DCMAKE_CXX_FLAGS_RELEASE='-Os' .

or you could empty the release flags to have only the contents of CFLAGS:

CFLAGS='-O2' CXXFLAGS='-Os' LDFLAGS='-Wl,--as-needed' cmake -DBUILD_SHARED_LIBS=ON -DCMAKE_C_FLAGS_RELEASE='' -DCMAKE_CXX_FLAGS_RELEASE='' .

I don't know if there is a better way to have CFLAGS override the defaults.

libzopfli and libzopflipng share zopflilib_obj objects but end up with their own copy. Why is libzopflipng not linking against libzopfli?

I copied what the Makefile did here. Also, on Windows it is not common to install libraries (DLL files), but rather they accompany the executables, so some people might prefer having libzopflipng as a standalone DLL they can distribute with their project?

I'm unsure if if(UNIX) target_link_libraries(libzopfli m) endif() is good enough for listed #12

I have no idea about this, sorry.

I would consider defaulting BUILD_SHARED_LIBS to ON, personally

So would I, but as mentioned in #58 this would require some changes to the include files to make the exported symbols visible on MSVC. Or a warning could be added to the CMake script when building shared libraries with MSVC.

[lvandeve]

Ideally the zopfli binaries would be standalone, no dynamic linking needed. Similarly, the zopfli executables on windows should be standalone, no DLLs needed by them.

But the so's and DLL's can be built for other programs who would like to dynamically link to zopfli.

So the binaries would be fully standalone, but about the dynamic libraries: whether the dynamic libzopflipng library then dynamically links to libzopfli, or has its own copy of libzopfli inside, personally I don't feel strongly about either way so whichever will work out the easiest.

[jibsen]

If nobody else is working on this I might have a look this week when time permits.

[jibsen]

I made a PR with some updates to the CMake script, comments welcome!

This appears to be how CMake works; it takes the contents of CFLAGS and uses that to initialize CMAKE_C_FLAGS, and then appends the flags for the build type, for instance CMAKE_C_FLAGS_RELEASE for a release build, which contains the -O3 you see.

Actually turns out the default build type for Makefile generators on UNIX is empty, which means it uses only CMAKE_C_FLAGS, and by extension CFLAGS. So I removed the code that automatically sets the build type to Release.

[hartwork]

Does this solve the issue?

@lvandeve looks a lot better with #151 merged now. I'd vote for removing the Makefile now (and maybe tagging a release candidate).