D28355: Introduce function ecm_install_configured_file

[Friedrich W. H. Kossebau]

kossebau added a comment.


  A  .rst file in the docs/module/ directory is needed, otherwise the documentation generation will not pick up this, as it runs only over docs/.
  Please enable the documentation generation in your ecm build and check for yourself, by e.g. ensuring `BUILD_HTML_DOCS` is `ON` and browsing the generated html in the build dir.
  
  That might also show that leaving empty lines without leading # as treated as limit for the documentation IIRC, so all lines which should belong to processed docs need a leading #, other than what the current patch does.
  
  Next I wonder why ".cmake.in" is an expected suffix here. That seems 2x suffixes used for input files. Usually in build systems input files have the suffix ".in", while with cmake there has also been a pattern of ".cmake" introduced, for whatever reason not liking `.in`. Having both suffixes at the same time is surprising and odd to me, what do I miss?
  IMHO the code should rather support both ".in" and ".cmake" (first test the one, otherwise the other), and perhaps for covering Plasma  needs additionally ".cmake.in" first, if you have too many files already with that pattern to fix this with the actual files instead rather.
  
  For the actual macro signatur, I wonder if something like
  
    ecm_install_configured_file(TEMPLATES <file> [<file2> [...]] DESTINATION <INSTALL_DIRECTORY> [COPYONLY] [ESCAPE_QUOTES] [@ONLY] [COMPONENT <component>])
  
  would not be better:
  
  - allows to handle multiple files if needed in one go
  - argument block names (TEMPLATES, DESTINATION) makes the code easier to read where the macro is used IMHO, and also allows extensibility in the future, if more arguments are found to be useful.
  - enabling to pass `COPYONLY`, `ESCAPE_QUOTES`, `@ONLY`on to `configure_file()` might be wanted as well
  - COMPONENT is underused, but some use it, so wanted to be passed on to `install()`
  
  Would be interesting to see actual examples where you made use of this new macro and how it improves things.
  
  Also missing; unit test (sorry) :)

INLINE COMMENTS

> ECMConfiguredInstall.cmake:12
> +# .. code-block:: cmake
> +#   ecm_install_configured_file(foo.txt.cmake.in ${KDE_INSTALL_FULL_DATADIR})
> +

The 'FULL' variants are not meant to be used usually, only where really needed, so for consistency better to leave out from the example and just use `KDE_INSTALL_DATADIR`, so people have recommended code to copy, and also do not wonder where the `FULL` variant is different from what they see elsewhere.

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/20200328/99589d39/attachment-0001.html>