D28355: Introduce function ecm_install_configured_file

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, bru

D28355: Introduce function ecm_install_configured_file

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 CMa

D28355: Introduce function ecm_install_configured_file

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 go

D28355: Introduce function ecm_install_configured_file

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

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, bru

D28355: Introduce function ecm_install_configured_file

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, ng

D28355: Introduce function ecm_install_configured_file

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 BRANC

D28355: Introduce function ecm_install_configured_file

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

D28355: Introduce function ecm_install_configured_file

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 resol

D28355: Introduce function ecm_install_configured_file

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 c

D28355: Introduce function ecm_install_configured_file

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" > ECMConfi

D28355: Introduce function ecm_install_configured_file

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 http

D28355: Introduce function ecm_install_configured_file

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 t

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

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 neat

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 sometim

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 #

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&id=78693 BRANCH master REVISION DETAIL https://phabricator.kde.org/D28355 AF

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,

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