D28355: Introduce function ecm_install_configured_file

[David Edmundson]

davidedmundson added a comment.


  Moved to invent.

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

D28355: Introduce function ecm_install_configured_file

[David Edmundson]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R240:2976b27a6c0b: Introduce function 
ecm_install_configured_file (authored by davidedmundson).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28355?vs=79176&id=83249

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

AFFECTED FILES
  docs/module/ECMConfiguredInstall.rst
  modules/ECMConfiguredInstall.cmake
  tests/CMakeLists.txt
  tests/ECMConfiguredInstallTest/CMakeLists.txt
  tests/ECMConfiguredInstallTest/check_tree.cmake.in
  tests/ECMConfiguredInstallTest/configured.txt
  tests/ECMConfiguredInstallTest/configured_atOnly.txt.in
  tests/ECMConfiguredInstallTest/expected/configured.txt
  tests/ECMConfiguredInstallTest/expected/configured_atOnly.txt
  tests/ECMConfiguredInstallTest/expected/multi1.txt
  tests/ECMConfiguredInstallTest/expected/multi2.txt
  tests/ECMConfiguredInstallTest/multi1.txt.in
  tests/ECMConfiguredInstallTest/multi2.txt.in

To: davidedmundson, #build_system
Cc: apol, kossebau, pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, bencreasy, michaelh, ngraham, bruns

D28355: Introduce function ecm_install_configured_file

[Friedrich W. H. Kossebau]

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

D28355: Introduce function ecm_install_configured_file

[Friedrich W. H. Kossebau]

kossebau added a reviewer: Build System.

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

D28355: Introduce function ecm_install_configured_file

[David Edmundson]

davidedmundson added a dependent revision: D28305: Systemd Startup.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson
Cc: apol, kossebau, pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, bencreasy, michaelh, ngraham, bruns

D28355: Introduce function ecm_install_configured_file

[Aleix Pol Gonzalez]

apol added a comment.


  Are we stuck somewhere? The patch looks good to me.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson
Cc: apol, kossebau, pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, bencreasy, michaelh, ngraham, bruns

D28355: Introduce function ecm_install_configured_file

[David Edmundson]

davidedmundson updated this revision to Diff 79176.
davidedmundson added a comment.


  extra escape
  
  Not needed for the tests, yet weirdly threw an error in plasma...

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28355?vs=79115&id=79176

BRANCH
  master

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

AFFECTED FILES
  docs/module/ECMConfiguredInstall.rst
  modules/ECMConfiguredInstall.cmake
  tests/CMakeLists.txt
  tests/ECMConfiguredInstallTest/CMakeLists.txt
  tests/ECMConfiguredInstallTest/check_tree.cmake.in
  tests/ECMConfiguredInstallTest/configured.txt
  tests/ECMConfiguredInstallTest/configured_atOnly.txt.in
  tests/ECMConfiguredInstallTest/expected/configured.txt
  tests/ECMConfiguredInstallTest/expected/configured_atOnly.txt
  tests/ECMConfiguredInstallTest/expected/multi1.txt
  tests/ECMConfiguredInstallTest/expected/multi2.txt
  tests/ECMConfiguredInstallTest/multi1.txt.in
  tests/ECMConfiguredInstallTest/multi2.txt.in

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

[David Edmundson]

davidedmundson updated this revision to Diff 79115.
davidedmundson marked an inline comment as done.
davidedmundson added a comment.


  update

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28355?vs=78887&id=79115

BRANCH
  master

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

AFFECTED FILES
  docs/module/ECMConfiguredInstall.rst
  modules/ECMConfiguredInstall.cmake
  tests/CMakeLists.txt
  tests/ECMConfiguredInstallTest/CMakeLists.txt
  tests/ECMConfiguredInstallTest/check_tree.cmake.in
  tests/ECMConfiguredInstallTest/configured.txt
  tests/ECMConfiguredInstallTest/configured_atOnly.txt.in
  tests/ECMConfiguredInstallTest/expected/configured.txt
  tests/ECMConfiguredInstallTest/expected/configured_atOnly.txt
  tests/ECMConfiguredInstallTest/expected/multi1.txt
  tests/ECMConfiguredInstallTest/expected/multi2.txt
  tests/ECMConfiguredInstallTest/multi1.txt.in
  tests/ECMConfiguredInstallTest/multi2.txt.in

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

[David Edmundson]

davidedmundson marked 3 inline comments as done.
davidedmundson added inline comments.

INLINE COMMENTS

> kossebau wrote in ECMConfiguredInstall.cmake:62
> Actually, _configure_args could be a list  (starting with empty, not "") and 
> one would do list(APPEND). And cmake would then properly resolve that var 
> when used with configure_file I would expect (to be tested).

COPY_ONLY I think is mutually exclusive anyway.

List are nicer than messing with a string anyway. I've done that.

> kossebau wrote in check_tree.cmake.in:4-11
> 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).

Heh, I'm wary of the trap where you end up needing tests to test the tests.

I've done it anyway. As a function. It's not in test_helpers yet, but would 
serve as a base.

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

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in ECMConfiguredInstall.cmake:62
> 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.

Actually, _configure_args could be a list  (starting with empty, not "") and 
one would do list(APPEND). And cmake would then properly resolve that var when 
used with configure_file I would expect (to be tested).

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

[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  [ [...]] 
> DESTINATION  [COPYONLY] [ESCAPE_QUOTES] [@ONLY] [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  [ [...]]
  #DESTINATION 
  #[COPYONLY]
  #[ESCAPE_QUOTES]
  #[@ONLY]
  #[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 

`# 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

D28355: Introduce function ecm_install_configured_file

[David Edmundson]

davidedmundson updated this revision to Diff 78887.
davidedmundson added a comment.


  All of the things!
  
  Only strips suffix ".in".

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28355?vs=78693&id=78887

BRANCH
  master

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

AFFECTED FILES
  docs/module/ECMConfiguredInstall.rst
  modules/ECMConfiguredInstall.cmake
  tests/CMakeLists.txt
  tests/ECMConfiguredInstallTest/CMakeLists.txt
  tests/ECMConfiguredInstallTest/check_tree.cmake.in
  tests/ECMConfiguredInstallTest/configured.txt
  tests/ECMConfiguredInstallTest/configured_atOnly.txt.in
  tests/ECMConfiguredInstallTest/expected/configured.txt
  tests/ECMConfiguredInstallTest/expected/configured_atOnly.txt
  tests/ECMConfiguredInstallTest/expected/multi1.txt
  tests/ECMConfiguredInstallTest/expected/multi2.txt
  tests/ECMConfiguredInstallTest/multi1.txt.in
  tests/ECMConfiguredInstallTest/multi2.txt.in

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

[David Edmundson]

davidedmundson added a comment.


  I agree with almost all those suggestions. Though it's a lot more complex, so 
I might need some help with some of that.
  
  As for intended usages, it came up on: D28305 
 for a lot of .service files with a binary 
location that needs to be derived at configure time. 
  But it seemed if I made it generic enough it was a pattern that showed up in 
other locations as shown by porting remaining bits of plasma-workspace: D28422 


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

[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  [ [...]] 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 Toscano]

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

[David Edmundson]

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 Toscano]

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

[David Edmundson]

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&id=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 Toscano]

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

[David Edmundson]

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