nih-at
Make CMake options consistent
Currently libzip uses many cmake options which are not prefixed with LIBZIP_ but some which do eg. LIBZIP_DO_INSTALL, wouldn't it be a good idea to prefix all of the options with LIBZIP_ because it would make them consistent and reduce the chance of conflict when including libzip with add_subdirectory?
Do you have a pointer to documentation about using subprojects with CMake and best practices for variables in that case?
This change is quite invasive since all packagers and users setting options will need to update their variables, and we can't add backward compatibility since this would defeat the point of the change.
(At least BUILD_SHARED_LIBS should consider the project-wide setting without a LIBZIP_ prefix.)
Do you have a pointer to documentation about using subprojects with CMake and best practices for variables in that case? This change is quite invasive since all packagers and users setting options will need to update their variables, and we can't add backward compatibility since this would defeat the point of the change. (At least
BUILD_SHARED_LIBSshould consider the project-wide setting without aLIBZIP_prefix.)
https://discourse.cmake.org/t/best-practices-for-option-naming/2039 for an example, a lot of libraries do that. And yeah you are right about BUILD_SHARED_LIBS, it actually affects add_library behavior so it should be kept as is (I reverted that in the PR). The issue with needing to change the options when packaging is indeed kind of annoying one, I am not really sure what would be the best solution for that.
We've come up with an idea how to support this without breaking backwards compatibility:
We add a new option LIBZIP_AS_SUBPROJECT. If that option is set to ON, then all option names are prefixed with LIBZIP_, otherwise the old names are used.
CMake supports that quite easily:
include(CMakeDependentOption)
option(LIBZIP_AS_SUBPROJECT "Prefix option names with LIBZIP_" OFF)
cmake_dependent_option(RUN_REGRESS "Run regression tests" ON "NOT LIBZIP_AS_SUBPROJECT" ON)
cmake_dependent_option(LIBZIP_RUN_REGRESS "Run regression tests" ON "LIBZIP_AS_SUBPROJECT" ${RUN_REGRESS})
Could you please update your PR to duplicate all options as described above?
We've come up with an idea how to support this without breaking backwards compatibility:
We add a new option
LIBZIP_AS_SUBPROJECT. If that option is set toON, then all option names are prefixed withLIBZIP_, otherwise the old names are used.CMake supports that quite easily:
include(CMakeDependentOption) option(LIBZIP_AS_SUBPROJECT "Prefix option names with LIBZIP_" OFF) cmake_dependent_option(RUN_REGRESS "Run regression tests" ON "NOT LIBZIP_AS_SUBPROJECT" ON) cmake_dependent_option(LIBZIP_RUN_REGRESS "Run regression tests" ON "LIBZIP_AS_SUBPROJECT" ${RUN_REGRESS})Could you please update your PR to duplicate all options as described above?
Hi, sorry that it took me so long to respond, I had other things to do and didn't remember this. It looks like doing it like that forcefully sets RUN_REGRESS to ON (or OFF if specified as the last argument to cmake_dependent_option) if LIBZIP_AS_SUBPROJECT is specified instead of not modifying it at all so that's not ideal.
I don't see it, but since this was only a proposal, you could fix this in your pull request :-)