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
