D28355: Introduce function ecm_install_configured_file
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 [ [...]] DESTINATION
[COPYONLY] [ESCAPE_QUOTES] [@ONLY] [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
D28355: Introduce function ecm_install_configured_file
pino added inline comments. INLINE COMMENTS > davidedmundson wrote in ECMConfiguredInstall.cmake:46-48 > Maybe. > > My rationale for not forcing a suffix is sometimes we do this configure dance > for .desktop files and there we have to be a bit careful with scripty. > > But generally it's neater and easier to use when a suffix is used. Currently > Plasma is all over the place with what suffix to have, so I picked one at > random. No problem either way. My idea is that if a precise suffix is required, then setting it //before// this function is merged is better, otherwise doing it once it is already in use means breaking potential (mis)users. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D28355 To: davidedmundson Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
D28355: Introduce function ecm_install_configured_file
davidedmundson added inline comments. INLINE COMMENTS > pino wrote in ECMConfiguredInstall.cmake:46-48 > considering we are documenting the input file as `.cmake.in`, should we > enforce this here and ignore any file not ending like that? Maybe. My rationale for not forcing a suffix is sometimes we do this configure dance for .desktop files and there we have to be a bit careful with scripty. But generally it's neater and easier to use when a suffix is used. Currently Plasma is all over the place with what suffix to have, so I picked one at random. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D28355 To: davidedmundson Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
D28355: Introduce function ecm_install_configured_file
pino added a comment.
Nice one! I cannot test right now though, I might do it over the weekend (do
not hold on me though).
I took the liberty of doing some formatting changes to the header of the new
file, what do you think?
#.rst:
# ECMConfiguredInstall
#
#
# Take as ``.cmake.in`` file, runs ``configure_file`` and installs it in the
# given location.
#
# ::
#
# ecm_install_configured_file( )
#
# Example usage:
#
# .. code-block:: cmake
#
# ecm_install_configured_file(foo.txt.cmake.in
${KDE_INSTALL_FULL_DATADIR})
#
# This will install the file as ``foo.txt`` with any cmake replacements made
# into the data directory.
#
# Since 5.69.0.
#=
# Copyright 2020 David Edmundson
Also, not sure whether you need a .rst file in the docs/module/ directory.
INLINE COMMENTS
> ECMConfiguredInstall.cmake:46-48
> +# strip .cmake.in from the end
> +# if that isn't there, we continue as-is
> +string(REGEX REPLACE ".cmake.in$" "" _name ${_name})
considering we are documenting the input file as `.cmake.in`, should we enforce
this here and ignore any file not ending like that?
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D28355
To: davidedmundson
Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2,
bencreasy, michaelh, ngraham, bruns
D28355: Introduce function ecm_install_configured_file
davidedmundson updated this revision to Diff 78693. davidedmundson added a comment. Good idea. Done REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28355?vs=78678=78693 BRANCH master REVISION DETAIL https://phabricator.kde.org/D28355 AFFECTED FILES modules/ECMConfiguredInstall.cmake To: davidedmundson Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
D28355: Introduce function ecm_install_configured_file
pino added a comment. Can you please adapt it so `_template` can be an absolute path? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D28355 To: davidedmundson Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
D28355: Introduce function ecm_install_configured_file
davidedmundson created this revision. Herald added projects: Frameworks, Build System. Herald added subscribers: kde-buildsystem, kde-frameworks-devel. davidedmundson requested review of this revision. REVISION SUMMARY This, as the name suggests, configures a file and installs it. It's not very complicated but it's a repeated pattern in plasma that gets quite messy dealing with temporary files. TEST PLAN Used in a project REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D28355 AFFECTED FILES modules/ECMConfiguredInstall.cmake To: davidedmundson Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, michaelh, ngraham, bruns
