Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a CMakeLists.txt file #101

Closed
wants to merge 0 commits into from
Closed

Conversation

bsergean
Copy link

Hi there, thanks for this super useful and fast library.

I added a simple CMake file which should make it so that more users can use your library. One good recent feature of cmake is called FetchContent, which make it real easy to include another library by downloading it from github and building it. I added some doc about it in the README file.

@bsergean bsergean mentioned this pull request Oct 28, 2020
@ebiggers
Copy link
Owner

Maintaining multiple build systems would be error-prone and extra work. (And I only have time to work in libdeflate in my free time, so anything that avoids introducing additional maintenance burden would be very helpful.) So the only way I might accept this is if CMake fully replaced the current Makefile. That means the CMake replacement would need to support everything the current Makefile supports, and the README would need to be fully updated, and all scripts in the scripts/ directory that are currently running make would need to be updated to use the CMake replacement instead.

Generally, my preference is to keep the plain Makefile. FWIW, libdeflate actually used CMake originally, but pretty quickly I replaced it with a plain Makefile since it seemed just as simple, and it was easier to use (e.g. make -j8 <options> instead of rm -r build; mkdir build; cd build; cmake .. <options>; make -j8 instead of just make -j8). And I don't really buy claims like "CMake is becoming a standard now", as lots of projects instead use GNU autotools, a plain Makefile, Meson, Bazel, etc...

It's still possible that I could be convinced if there were large benefits to switching to CMake in particular, though, and if it actually replaced the current Makefile as mentioned above, and if you did a really good job with the changes.

@bsergean
Copy link
Author

Thanks for taking a look, you're making some good points.

I have a project where I still have a very tiny makefile in the top folder which takes care of invoking cmake for the common case which can be a chore. I do all my development on mac or linux.

I don't know at which pace I can improve this PR, but the current version is relatively complete, I'll look in more details at what is inside the scripts folder.

One advantage of cmake is that it is truly cross platform and works well on Windows, while autotools is not, and I haven't followed the latest trends, but bazel seems to be for niche projects or google only projects, same with buck (the facebook one). Not sure what big project uses Meson, I can't think of any, maybe jsoncpp. Also FetchContent is quite cool I think.

I know C++ more, but serious projects that now have cmake support are libcurl, libuv, spdlog, rapidjson, openssl. If your project has cmake it can be included more easily in vcpkg which is Microsoft attempt at adding a real package manager for C/C++. (there's something called cdeps too for just C).

@bsergean
Copy link
Author

I found something interesting where a build fail through cmake, but not with the regular make build. (the xenial warning config). The other outstanding thing I'm looking at is making shared libraries, where I get missing symbols.

In file included from /home/travis/build/bsergean/libdeflate/lib/adler32.c:63:0:

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h: In function ‘adler32_neon_chunk’:

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h:102:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]

  *s1 += v_s1[0] + v_s1[1] + v_s1[2] + v_s1[3];

  ^

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h:103:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]

  *s2 += v_s2[0] + v_s2[1] + v_s2[2] + v_s2[3];

  ^

cc1: all warnings being treated as errors

@ebiggers
Copy link
Owner

In file included from /home/travis/build/bsergean/libdeflate/lib/adler32.c:63:0:

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h: In function ‘adler32_neon_chunk’:

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h:102:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]

  *s1 += v_s1[0] + v_s1[1] + v_s1[2] + v_s1[3];

  ^

/home/travis/build/bsergean/libdeflate/lib/arm/adler32_impl.h:103:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]

  *s2 += v_s2[0] + v_s2[1] + v_s2[2] + v_s2[3];

  ^

cc1: all warnings being treated as errors

That's a known false positive compiler warning with some gcc versions. In .travis.yml (https://github.com/ebiggers/libdeflate/blob/master/.travis.yml#L43), this warning is suppressed on xenial + gcc + arm64, which is the only tested case where this warning is seen.

CMakeLists.txt Outdated

if (BUILD_SHARED_LIBS)
target_compile_options(deflate PRIVATE -fvisibility=hidden -D_ANSI_SOURCE)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
Copy link

Choose a reason for hiding this comment

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

Not required. In CMake, shared libs always have POSITION_INDEPENDENT_CODE ON

CMakeLists.txt Outdated
# The CFLAGS environment variable is read and handled automatically by cmake

if (BUILD_SHARED_LIBS)
target_compile_options(deflate PRIVATE -fvisibility=hidden -D_ANSI_SOURCE)
Copy link

@SpaceIm SpaceIm Jan 11, 2021

Choose a reason for hiding this comment

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

There is an abstraction of symbols visibility in CMake: https://cmake.org/cmake/help/latest/prop_tgt/LANG_VISIBILITY_PRESET.html

Copy link

@SpaceIm SpaceIm Jan 11, 2021

Choose a reason for hiding this comment

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

Moreover LIBDEFLATE_DLL PUBLIC definition should be added if WIN32 and shared:

$(CC) -c /Fo$@ $(CFLAGS) /DLIBDEFLATE_DLL $**

$(SHARED_LIB_CFLAGS) -DLIBDEFLATE_DLL $<

#ifdef LIBDEFLATE_DLL

@bsergean
Copy link
Author

bsergean commented Jan 11, 2021 via email

@bsergean
Copy link
Author

$ mkdir build
$ cd build
$ cmake -DDEFLATE_BUILD_TEST_PROGRAMS=ON ..
$ make ...
$ make test
Running tests...
Test project /Users/benjaminsergeant/src/foss/github/libdeflate/build
    Start 1: test_checksums
1/6 Test #1: test_checksums ...................   Passed    0.25 sec
    Start 2: test_custom_malloc
2/6 Test #2: test_custom_malloc ...............   Passed    0.24 sec
    Start 3: test_incomplete_codes
3/6 Test #3: test_incomplete_codes ............   Passed    0.22 sec
    Start 4: test_litrunlen_overflow
4/6 Test #4: test_litrunlen_overflow ..........   Passed    0.24 sec
    Start 5: test_slow_decompression
5/6 Test #5: test_slow_decompression ..........   Passed    0.21 sec
    Start 6: test_trailing_bytes
6/6 Test #6: test_trailing_bytes ..............   Passed    0.21 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) =   1.37 sec

@bsergean
Copy link
Author

@ebiggers I've changed the PR to be purely additive, and not conflict with any existing makefiles or scripts.

I know you made your point about wanting to have a single build-system, but would you consider accepting the cmake files as community maintained, which can be highlighted even more in the readme? Project like libuv or curl have multiple build systems.

A bunch of folks use IDE projects generators, and having cmake support will let them build libdeflate when used in their project that way. Also the cmake ninja generator will work fine here.

@rleigh-codelibre
Copy link

rleigh-codelibre commented Mar 7, 2021

I was just looking at CMake support for libdeflate (for use with libTIFF), and noticed all the hard work had already been done! This is really nice.

For building with Windows with MinGW or MSVC, CMake is a real improvement on the two Makefiles. It would make it much easier to package and distribute with vcpkg for example. The Makefile.msc has no install targets and no support for debug vs release runtimes. CMake can provide all that directly.

Some comments on the PR:

  • If libdeflate depends upon ZLIB, it would be nice to have a transitive dependency on ZLIB::ZLIB and have the exported configuration use find_dependency to automatically find and create the imported target. You can see an example of this here L226-249 and this template. Note: this was originally done with a fairly old CMake version; there may now be a more transparent way of importing the transitive dependencies.
  • Shared library versioning doesn't look quite right; if the existing Makefiles don't do this either it would be nice to set up some proper versions rather than 0.0.0 for all build systems
  • You might want to set the debug postfix to "d" for Windows, so we can have debug and release libraries coexisting (would also be nice for getting libdeflate packaged with vcpkg). Example:
# Debug postfix
if(MSVC)
    set(CMAKE_DEBUG_POSTFIX "d")
endif()

Edit: One other oddity noticed while working on libdeflate support in libtiff. Why does libdeflate have a "lib" prefix on Windows? It's easy enough to work around, but CMake's find library doesn't find "deflate" by default because it only adds a "lib" prefix on Unix platforms. Why are the platform library naming conventions not being followed on Windows (in Makefile.msc)? Looks like this is one thing that the CMake build will do correctly.

As an aside, FindDeflate might be of interest. The library naming doesn't quite match the static Makefile names; having the debug postfixes would be necessary to make this work properly. If you can make the exported configuration stuff work properly, this can be removed, but just mentioning it in case it's of use to others who want to use libdeflate with CMake (as opposed to building with CMake).

Kind regards,
Roger

@bsergean
Copy link
Author

bsergean commented Mar 7, 2021 via email

@SpaceIm
Copy link

SpaceIm commented Mar 7, 2021

I have created libdeflate recipe in conan public repository, and I'm also interested in CMake support in libdeflate. Indeed libdeflate is now an optional dependency of libtiff (since 4.2.0) and gdal (since 3.2.0), enabled by default, so it's indirectly included in the dependency graph of a lot of libraries. Therefore a robust build would be welcome.

@ivan-volnov
Copy link

Any news here?

@serge2016
Copy link

I am also very interested!

@ebiggers ebiggers mentioned this pull request Jan 1, 2022
@diizzyy
Copy link

diizzyy commented Jan 14, 2022

I think most would appreciate using CMake as framework :-)

@ckerr
Copy link

ckerr commented Jan 15, 2022

At the risk of piling on, I'd also like to see CMake support for libdeflate. It would make embedding it in my own CMake-based application much simpler. :)

@ebiggers
Copy link
Owner

Given the interest, I'm willing to switch to CMake, but it has to replace the current Makefile instead of being added alongside it.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
Comment on lines 151 to 152
ARCHIVE DESTINATION lib
PUBLIC_HEADER DESTINATION include/
Copy link

Choose a reason for hiding this comment

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

Suggested change
ARCHIVE DESTINATION lib
PUBLIC_HEADER DESTINATION include/
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}

CMakeLists.txt Outdated
Comment on lines 155 to 159
# This is required to work with FetchContent
install(EXPORT deflate
FILE deflate-config.cmake
NAMESPACE deflate::
DESTINATION lib/cmake/deflate)
Copy link

@SpaceIm SpaceIm Jan 27, 2022

Choose a reason for hiding this comment

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

  • Misleading comment, CMake config files are mainly for find_package().

  • Instead I would write a basic deflate-config.cmake.in file and rely on CMakePackageConfigHelpers module and its configure_package_config_file. I would export targets in deflate-targets.cmake. Do not forget to also export a target file in the build tree. A version file would be nice also (it requires to add VERSION to the project.
    Basically something like this:

    configure_package_config_file(
        ${PROJECT_SOURCE_DIR}/cmake/deflate-config.cmake.in
        deflate-config.cmake
        INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/deflate
    )
    
    write_basic_package_version_file(
        deflate-config-version.cmake
        VERSION ${PROJECT_VERSION}
        COMPATIBILITY AnyNewerVersion
    )
    
    export(
        EXPORT deflate
        NAMESPACE deflate::
        FILE deflate-targets.cmake
    )
    
    install(
        EXPORT deflate
        NAMESPACE deflate::
        FILE deflate-targets.cmake
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/deflate
    )
    
    install(
        FILES
            ${CMAKE_CURRENT_BINARY_DIR}/deflate-config.cmake
            ${CMAKE_CURRENT_BINARY_DIR}/deflate-config-version.cmake
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/deflate
    )
  • The makefile installs a pkgconfig file, it should be ported to the CMakeLists. The previous one can be reused.

@bsergean
Copy link
Author

I think there should be a mini transition period where both makefile and cmakefiles exists, so that we can know that the new system actually works.

I'm still very interested in having this PR land, but I have also a lot of other things on my plate so I don't know when I'll get to this. @SpaceIm / how you could you add your proposed changes to this PR ? By making a PR in my repo ?

CMakeLists.txt Outdated

# This is required to work with FetchContent
install(EXPORT deflate
FILE deflate-config.cmake

Choose a reason for hiding this comment

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

The existing pkg-config export is called libdeflate.pc, used via

  • dependency('libdeflate') (meson)
  • pkg_check_modules(LIBDEFLATE libdeflate IMPORTED_TARGET GLOBAL) (cmake)
  • pkg-config --cflags --libs libdeflate (Makefile)

Hence, the cmake-config name must be libdeflate-config.cmake, used via:

  • find_package(libdeflate) (cmake)
  • dependency('libdeflate') (meson)

It is extremely important the names be consistent between pkg-config and cmake. e.g. Meson can look up this project as a dependency using multiple methods: pkg-config, cmake, checking for a standalone ini file defining the location to download the dependency from...

However, in order for that to work, the names must be the same, or Meson will look for libdeflate-config.cmake when it is strangely named deflate-config.cmake.

@SpaceIm
Copy link

SpaceIm commented Jan 27, 2022

@SpaceIm / how you could you add your proposed changes to this PR ? By making a PR in my repo ?

Sure.

@SpaceIm
Copy link

SpaceIm commented Jan 27, 2022

@bsergean Please consider bsergean#1

  • fix export of symbols for non-Windows if shared
  • change target to libdeflate, and imported target to libdeflate::libdeflate
  • honor lib name from original Makefile if windows & static (but still drop the lib prefix if msvc, and .lib for mingw in favor of .a or .dll.a)
  • add VERSION
  • improve CMake config file
  • install a pkgconfig file
  • always build the cli & honor name from Makefile. Also install it in bin folder
  • better separation between cli & tests
  • link zlib with tests only & don't forget to find it with find_package(). Also properly express the scope of dependencies of each target.
  • edit README regarding CMake:
    • more generic commands
    • don't speak about FetchContent. Proper way to consume a library is to install it, then call find_package() in your own CMakeLists

So I think it also addresses @eli-schwartz review.

Tested on macOS with AppleClang 13, static & shared.

@eli-schwartz
Copy link

I commented on that PR. The generated pkg-config file has an edge case that leads to extremely broken behavior if the user configures with -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_LIBDIR=/usr/lib (using an absolute path for values other than prefix).

This is very difficult to fix properly due to the design of cmake (although meson handles it perfectly), although there are ways to make it less terrible.

@SpaceIm
Copy link

SpaceIm commented Jan 27, 2022

another one: bsergean#2

MinGW is broken by the way (for utility & tests). -municode must be injected during compilation of the the utility & tests, but I can't make it work with CMake.

@ebiggers
Copy link
Owner

At the risk of piling on, but for the record: I for one would likely lose any interest in contributing to libdeflate if it switched from a Makefile to CMake.

Can you elaborate on why that is?

@ivan-volnov
Copy link

At the risk of piling on, but for the record: I for one would likely lose any interest in contributing to libdeflate if it switched from a Makefile to CMake.

But no great loss: I haven't yet contributed to libdeflate in any case. 😃

Nice try ))

Well, I personally believe that CMake is very important for many developers. CMake support allows easy integration with current big projects that use the library.

Even Boost, having their own build system, is gradually switching to CMake.

So, in my opinion, the library can gain a lot of contemporary developers by only switching the build system.

@eli-schwartz
Copy link

(Boost's own build system also has the curious status of being worse than handwritten Makefiles.)

ebiggers added a commit that referenced this pull request Sep 20, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 21, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 22, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 22, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
@bsergean bsergean closed this Sep 26, 2022
@bsergean
Copy link
Author

I just closed this, this is where the work is happening now -> #235

ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
ebiggers added a commit that referenced this pull request Sep 29, 2022
By popular request, use CMake instead of a plain Makefile.

Some parts of this commit are based on the pull request
#101, which was written by
Benjamin Sergeant <bsergean@gmail.com> and
SpaceIm <30052553+SpaceIm@users.noreply.github.com>.
@ebiggers
Copy link
Owner

The master branch is using CMake now. I'd appreciate it if people would verify that it's working properly for their use cases.

@bsergean
Copy link
Author

I just tried to build master with ExternalProject, and it works fine.

ExternalProject_Add(
  project_deflate
  EXCLUDE_FROM_ALL TRUE
  PREFIX ${EXTERNAL_PROJECTS_DIR}
  GIT_REPOSITORY "https://github.com/ebiggers/libdeflate.git"
  GIT_TAG master
  CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
)

@diizzyy
Copy link

diizzyy commented Sep 29, 2022

Basic compile and unit tests works as expected on FreeBSD 13.1-STABLE

@ckerr
Copy link

ckerr commented Sep 30, 2022

The master branch is using CMake now. I'd appreciate it if people would verify that it's working properly for their use cases.

@ebiggers 03fba38 is working for my project 👍. LGTM

@rleigh-codelibre
Copy link

I created #236 after some initial testing on Windows. There is a portability issue with the shared/static library naming which will need fixing to make the software buildable and usable on Windows.

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

Successfully merging this pull request may close these issues.