rapidfuzz / rapidfuzz-cpp

Rapid fuzzy string matching in C++ using the Levenshtein Distance
https://rapidfuzz.github.io/rapidfuzz-cpp
MIT License
238 stars 37 forks source link

added missing lines necessary to call CPack and create .deb file #61

Closed stephanecharette closed 2 years ago

stephanecharette commented 2 years ago

Give people the opportunity to run make package and create a .deb file. For some of us, this is better than make install since it uses the usual package manager to get the files installed, and allows us to easily move the .deb file to another Ubuntu computer.

maxbachmann commented 2 years ago

This sounds like a great addition. I have a couple of comments: 1) It appears like CPACK_PACKAGE_VERSION is only set starting cmake 3.12. So we should probably specify it for earlier versions similar to catch2:

    # CPack/CMake started taking the package version from project version 3.12
    # So we need to set the version manually for older CMake versions
    if(${CMAKE_VERSION} VERSION_LESS "3.12.0")
        set(CPACK_PACKAGE_VERSION ${PROJECT_VERSION})
    endif()

2) could you add a test for this to github actions? I assume it could be the same as https://github.com/maxbachmann/rapidfuzz-cpp/blob/fe182872ce8728bf8b6e1bb657f84723792c0346/.github/workflows/cmake.yml#L45, but uses make package + dpkg instead of make install

stephanecharette commented 2 years ago

I have no idea, I've never used github actions before.

stephanecharette commented 2 years ago

Why have the if() block? If the version number is available in a variable, why not set it regardless of the cmake version?

maxbachmann commented 2 years ago

Why have the if() block? If the version number is available in a variable, why not set it regardless of the cmake version?

I would assume that catch2 mostly does this, so it is clear that this can be dropped once a newer cmake version is mandatory.

maxbachmann commented 2 years ago

I have no idea, I've never used github actions before.

I can write the test myself after this is merged

maxbachmann commented 2 years ago

I applied the changes and merged the PR. Thank you for your work.