Update CMake script #151
Update CMake script #151
Conversation
This is a compromise between getting useful features and supporting operating systems still in use. https://gitlab.kitware.com/cmake/community/wikis/doc/cmake/Life-Cycle-Considerations
Define the library version in one place. If the minimum CMake version is ever updated to 3.0, this could be added to the project command.
When using zopfli as a subproject, users can do: add_subdirectory(zopfli) target_link_libraries(my_target libzopfli)
For Makefile generators, the default build type (CMAKE_BUILD_TYPE empty) uses CMAKE_C_FLAGS and by extension CFLAGS. Instead of forcing the build type in this case, print a status message letting the user know.
This replaces BUILD_SHARED_LIBS, which should not be inadvertently set by Zopfli if built as a subproject. Instead we use BUILD_SHARED_LIBS as the default value, if set. Also do not add install target for Zopfli by default when built as a static library subproject.
Recent versions of CMake support exporting all symbols when building a DLL. Enable this feature as a workaround for not explicitly exporting the public symbols, and warn on older version of CMake.
|
Digging around the libm issue a bit, I found a number of projects that test specifically for BeOS and Haiku before adding the library -- I've added this to the PR. If it turns out this is not enough, we could explicitly check if we can link in log() without including libm; include(CheckFunctionExists)
check_function_exists(log zopfli_has_log)
if(NOT zopfli_has_log)
target_link_libraries(libzopfli PRIVATE m)
endif() |
|
I built zopfli with the new CMakeLists.txt for this pull request, and also with the current CMakeLists.txt before this pull request. The zopfli binary of before appears to run 3x slower after than before this pull request. Might some compiler flags be different? I don't see any flags like O3 in either of them so I'm not sure how to tell. Do you measure any speed difference between the two on your system? I built like this in both cases, on Linux: cmake . -DBUILD_SHARED_LIBS=ON && make VERBOSE=1 Ideally it should have the same speed as before |
|
Yes, as noted above, I made it so the build type is not forced to Release if unset. This means you have to specify the Release build if you want it: cmake . -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON && make VERBOSE=1The reason for this change was the issues @hartwork had in #143 with being unable to override the build flags with CFLAGS/CXXFLAGS. If you prefer having the build default to Release and using one of the workarounds from that thread to support CFLAGS, I'll change that back. |
|
What do open source projects normally do, do people expect release mode or debug mode by default? Is the override of CFLAGS an issue in other open source projects and how do they deal with it? As a data point, personally when I build (make) a project created by somebody else I assume that by default it builds a fast release mode version. What is your experience with this? Your input is very helpful by the way :) I'm just trying to keep building zopfli straightforward and compatible, it is a dependencyless library after all |
I am afraid it varies. I wish there were some clear best practices for things like this, but in general I think as long as you clearly state in the documentation how to build it, people can work it out. Regarding the CFLAGS issue, I don't have any experience with this myself, I believe it is something package maintainers use? but I was hoping some of the people who have raised issues around it might chime in.
I would agree that If a project has a bare Makefile, I assume it will build a release version. If it has a CMake script, I assume I can choose which build type I'd like when running CMake (for a single-configuration generator). This also works if we default to Release of course, the question is if it is better or worse to change the default behavior of CMake? I can see arguments in favor of both, personally I am gravitating towards it being best to try to avoid changing default behavior and global variables unless your build depends on it -- especially if your project might be used as a subproject. But of course you risk people building the latest version without reading the docs on setting a build type and going "Zopfli is slow!". Here are some more datapoints:
As you can see, there are examples of pretty much every possible choice -- the question is what do you prefer? (and what works best for your users of course). |
As a distro packager you want the package to build 100% your distro-specific flags without need to patch any files. With a source-based distro like Gentoo, flags are user specific even. |
CMakeLists.txt
Outdated
| if(MSVC) | ||
| add_definitions(/D_CRT_SECURE_NO_WARNINGS) | ||
| # If standalone or shared subproject, default to building install | ||
| if(zopfli_standalone OR ZOPFLI_BUILD_SHARED) |
There was a problem hiding this comment.
I'm unsure why that guard was chosen for enabling the install feature. I'd have expected something like "enabled unless on WIN32" or so. What is the idea?
There was a problem hiding this comment.
If we are building Zopfli only (standalone), we want to enable install.
If Zopfli is being built as a subproject of some other project, and as static libraries, then we will most likely be linked into the parent project, and should not add our installation files to the parent project (unless asked to).
If however Zopfli is being built as a subproject as shared libraries, it will most likely need to be installed alongside the parent project for it to work.
I don't know, perhaps this is too much logic to put into the CMake script to try and cover obscure use cases?
Release with optimizations by default, since it is an app where performance matters after all. If somebody wants to override compiler flag, maybe a non-default mode can be used for that use case? |
|
Great feedback @hartwork, thanks! |
Avoids duplicating the zopfli code and simplifies the build a bit. Also move source file listings to the respective add_library and add_executable calls so we don't have extra variables.
Not supported in CMake 2.8.11.
Make based single-configuration generators default to an empty build type, if that is the case set it to Release instead. Also add an option to disable this behavior in case someone needs the default empty build type.
|
Updated some of the comments to try to better explain the idea behind the options. I switched to libzopflipng linking with libzopfli since it simplified the build somewhat. We'll see if anyone complains. Also by default set the build type to Release if empty, with an option (ZOPFLI_DEFAULT_RELEASE) to disable this behavior. If you want to have the empty build type so you can control the flags, something like this should work: CFLAGS='-O2' CXXFLAGS='-Os' LDFLAGS='-Wl,--as-needed' cmake -DBUILD_SHARED_LIBS=ON -DZOPFLI_DEFAULT_RELEASE=OFF . |
Install config files that allow other CMake based projects to use Zopfli with find_package(Zopfli). Also add aliases, so targets are available with the same names when built as a subproject. This allows the "Modern CMake" usage pattern: find_package(Zopfli REQUIRED) # or add_subdirectory(Zopfli) ... target_link_libraries(my_target PRIVATE Zopfli::libzopfli) These are only enabled for CMake 3.0+. While strictly speaking much of the functionality is available in earlier versions, the namespace support is not.
|
The config file package support was the last thing I'd been meaning to add. It installs a few extra files that allow other CMake based projects to easily use an installed Zopfli with find_package(). |
|
Seems to build optimized binaries again by default, good one to try, let's see if maintainers will like it. Thank you for the updates! |
Okay, as mentioned in #143 I've taken a shot at updating the CMake script to try to address some of the issues raised there. Specifically:
I also improved support for using Zopfli as a subproject from other CMake based projects.
Some issues/things to discuss that remain: