D28355: Introduce function ecm_install_configured_file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Quick review while I had some spare minutes, to keep things going.

INLINE COMMENTS

> ECMConfiguredInstall.cmake:5
> +#
> +# Take a list of files, runs configure_file and installs it in the given location.
> +#

Perhaps "install" -> "install the result"

> ECMConfiguredInstall.cmake:9
> +# ::
> +#   ecm_install_configured_files(TEMPLATES <file> [<file2> [...]] DESTINATION <INSTALL_DIRECTORY> [COPYONLY] [ESCAPE_QUOTES] [@ONLY] [COMPONENT <component>])
> +#

Might be nicer to have each argument on an own line, like done in the (most?) other docs, like this:

  #   ecm_install_configured_files(
  #        TEMPLATES <file> [<file2> [...]]
  #        DESTINATION <INSTALL_DIRECTORY>
  #        [COPYONLY]
  #        [ESCAPE_QUOTES]
  #        [@ONLY]
  #        [COMPONENT <component>]
  #    )

> ECMConfiguredInstall.cmake:18
> +# This wil install the file as foo.txt with any cmake variable replacements made into the data directory.
> +
> +# Copyright 2020  David Edmundson <davidedmundson at kde.org>

`# Since 5.69/70.0.`

> ECMConfiguredInstall.cmake:56
> +
> +        string(REGEX REPLACE ".in$"  "" _name ${_name})
> +

Being a regexp, the "." in ".in$" might need escaping.

> ECMConfiguredInstall.cmake:62
> +        if (ARGS_COPY_ONLY)
> +                string(APPEND _configure_args COPY_ONLY)
> +        endif()

These strings (besides the last obviously) should get added with whitespace suffix, to handle the case where multiple are added, no? Not yet got to test/run things, just guessing by reading code.

> check_tree.cmake.in:4-11
> +execute_process(COMMAND ${CMAKE_COMMAND} -E compare_files ${ACTUAL}/test1/configured.txt
> +                                   ${EXPECTED}/configured.txt
> +                                   RESULT_VARIABLE test1_result
> +)
> +
> +If (NOT test1_result EQUAL 0)
> +    message(FATAL_ERROR "Configured files differ!")

This could become a macro/function perhaps, instead of repeating the same logic 4 times. Actually one that should get moved to tests/test_helpers.cmake later, as I remember other places which also check generated files against file samples.

But can also be done as follow-up by someone (tm).

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D28355

To: davidedmundson
Cc: kossebau, pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20200330/b8d4cb07/attachment-0001.html>