add cmake building support #704
Conversation
|
|
||
| # ~~~ Options | ||
| option(HUNSPELL_BUILD_TOOLS "Build Hunspell tools and parser." ON) | ||
| option(HUNSPELL_BUILD_STATIC_LIB "Build static hunspell library." ON) |
There was a problem hiding this comment.
Please do not duplicate CMake built-in functionality: https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html
| set(HUNSPELL_INCLUDE_INSTALL_DIR "include") | ||
| set(HUNSPELL_LIBRARY_INSTALL_DIR "lib") | ||
| set(HUNSPELL_RUNTIME_INSTALL_DIR "bin") |
There was a problem hiding this comment.
Please do not duplicate CMake built-in functionality: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
| set(HUNSPELL_RUNTIME_INSTALL_DIR "bin") | ||
| set(HUNSPELL_INSTALL_TARGETS "" CACHE INTERNAL "") | ||
| set(HUNSPELL_EXTRA_EXPORT_TARGETS "" CACHE INTERNAL "") | ||
| set(VERSION ${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}.${PROJECT_VERSION_PATCH}) |
There was a problem hiding this comment.
Please do not duplicate CMake built-in functionality: https://cmake.org/cmake/help/latest/variable/PROJECT_VERSION.html
| # Set RPATH for installed executables | ||
| set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/${HUNSPELL_LIBRARY_INSTALL_DIR}") | ||
| set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) |
There was a problem hiding this comment.
Please do not set CMAKE_* variables that are intended to be manipulated from the command-line by users.
| include(CheckCXXCompilerFlag) | ||
|
|
||
| check_cxx_compiler_flag("-fvisibility=hidden" HAVE_VISIBILITY) | ||
| if(NOT HAVE_VISIBILITY) | ||
| set(HAVE_VISIBILITY 0) | ||
| endif() |
There was a problem hiding this comment.
I assume the goal here is to check for DLL exports, in which case please avoid duplicating CMake built-in functionality. This example shows how to properly create a dual static/shared project: https://github.com/friendlyanon/cxx-static-shared-example
| set(_TEST_OUTPUT_NAME "test") | ||
|
|
||
| set(HUNSPELL_PARSERS_TARGET_PREFIX "parsers" CACHE INTERNAL "") | ||
| set(HUNSPELL_PARSERS_LIB_TARGET "${HUNSPELL_PARSERS_TARGET_PREFIX}_lib" CACHE INTERNAL "") |
There was a problem hiding this comment.
What's with the suffix? See comment above about idiomatic naming on different platforms.
| if(WIN32 OR MINGW) | ||
| copy_dll_if_changed(${HUNSPELL_LIB_TARGET_SHARED} ${HUNSPELL_PARSERS_TEST_TARGET}) | ||
| endif() |
There was a problem hiding this comment.
This is harmful. See comment above.
| find_package(Curses) | ||
|
|
||
| if(NOT CURSES_FOUND) | ||
| message(FATAL_ERROR "Could not find curses library. Set paths manually.") | ||
| endif() |
| find_package_handle_standard_args(Readline | ||
| REQUIRED_VARS | ||
| READLINE_LIBRARY | ||
| READLINE_INCLUDE_PATH | ||
| ) |
There was a problem hiding this comment.
What is this doing smack dab in the middle of everything? This is a function made available by FindPackageHandleStandardArgs for ONLY inside find modules. For one, the module is not included anywhere, but more importantly this code is not even in a find module.
| $<$<BOOL:${_HAVE_LOCALE_H}>:HAVE_LOCALE_H> | ||
| $<$<BOOL:${_HAVE_LANGINFO_H}>:HAVE_LANGINFO_H> | ||
| $<$<BOOL:${Iconv_FOUND}>:HAVE_ICONV> | ||
| $<$<BOOL:${Iconv_FOUND}>:ICONV_CONST=> | ||
| $<$<BOOL:${_HAVE_CURSES_H}>:HAVE_CURSES_H> | ||
| $<$<BOOL:${_HAVE_NCURSESW_H}>:HAVE_NCURSESW_CURSES_H> | ||
| $<$<BOOL:${Readline_FOUND}>:HAVE_READLINE> |
There was a problem hiding this comment.
These are known at configure time, please do not defer them to generation time using genexes.
Hi there @friendlyanon , here is the author of the original PR #617. I see you have had some time to perform a basic code review. If you need this functionality and have some better ideas on how to improve the build architecture of 'hunspell' using CMake, then feel free to implement and update these scripts, I am afraid I do not have time nor am I interested in CMake anymore. I would suggest that you and others here that intend to add or want official support for CMake to have a discussion with the maintainer regarding the details of the original build system and lay down some proper requirements, before doing any further unnecessary work. This has not been the case at the time of implementing the initial CMake scripts. I do also believe that the maintainer would consider a replacement of current makefiles only if the complete functionality is provided by CMake. |
|
Who is going to benefit from a CMake build system? I think that's the fist thing to figure out. |
#758 sort of aims to be that. So far there has been no response there.
The entire ecosystem! That is if you consider surveys such as these: Supporting clients using CMake (via a CMake package) would also make it trivial to have hunspell as a transitive dependency in the de facto standard build system of C++ and C projects. CMake also just makes it easier to build projects in general. So much so that I wrote my own CMakeLists.txt for packaging hunspell for Conan, because I'm too old to be trying to make |
|
Hunspell is impossible to compile on Windows in April 2024 since autotools is no longer installable after the osdn mirror lost stability, so quite a few folks will benefit from the cmake-based build process. |
Update for #617
Based on the most recent upstream code HEAD "8a2fdfe" (origin/master, origin/HEAD, master) replace archaic &std::vector::operator[0] with c++11 std::vector::data
Details:
src/hunspell/CMakeLists.txt:92
# $<$<CXX_COMPILER_ID:Clang>:-Wl,-no-undefined,error>This setting does not work on Void Linux (Clang, musl). It's not necessary for this toolchain.
src/hunspell/CMakeLists.txt:101
VERSION 0.0.${PROJECT_VERSION_PATCH} # ${PROJECT_VERSION}If I set project version to 1.7.2, the patch version would be 2. We will get "libhunspell-1.7.so.0.0.2". Otherwise from the original version setting we got "libhunspell-1.7.so.1.7.0"
src/hunspell/CMakeLists.txt:161
VERSION 0.0.${PROJECT_VERSION_PATCH} # ${PROJECT_VERSION}Same as above.