kossebau added a comment.
A bit unsure if the arg name "TEMPLATES" is good, or if perhaps should be
renamed to "INPUT". Just mentioning, not preferring one over the other. So far
have not found existing samples to take as lead for consistent argument naming.
For completeness, would be good to test for presence of ARGS_DESTINATION,
given this is a required arg, and do an error report, instead of falling flat
internally on the install() command.
ARGS_TEMPLATES should be fine to be empty, no need to error out on that,
might happen if input on caller side is based on a var which gets filled
conditionally and might eventually end with empty list,
And while talking checking input, I recently learned about the foillowing
handling, and think it provides users of the macros some service in case of
typos or accidental misuse, so propose to also add here:
if(ARGS_UNPARSED_ARGUMENTS)
message(FATAL_ERROR "Unknown arguments given to
ecm_install_configured_file(): \"${ARGS_UNPARSED_ARGUMENTS}\"")
endif()
Otherwise looks good to me as well, no showstoppers on my mind. Not yet
tested myself though, only looked at patch code.
INLINE COMMENTS
> ECMConfiguredInstall.cmake:5
> +#
> +# Take a list of files, runs configure_file and installs the resultant
> configured file in the given location.
> +#
"Takes". And: configured "files", given "list of files"?
> CMakeLists.txt:2
> +project(ECMConfiguredInstallTest)
> +cmake_minimum_required(VERSION 3.5)
> +set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../modules)
cmake_minimum_required should be first line always, for consistent pattern.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D28355
To: davidedmundson, #build_system
Cc: apol, kossebau, pino, kde-frameworks-devel, kde-buildsystem, LeGast00n,
cblack, bencreasy, michaelh, ngraham, bruns
