Introduce function ecm_install_configured_file
ClosedPublic

Authored by davidedmundson on Mar 27 2020, 5:00 PM.

Details

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

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Mar 27 2020, 5:00 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptMar 27 2020, 5:00 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
davidedmundson requested review of this revision.Mar 27 2020, 5:00 PM
pino added a subscriber: pino.Mar 27 2020, 5:18 PM

Can you please adapt it so _template can be an absolute path?

Good idea. Done

pino added a comment.Mar 27 2020, 6:42 PM

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(<FILE> <INSTALL_DIRECTORY>)
#
# 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 <davidedmundson@kde.org>

Also, not sure whether you need a .rst file in the docs/module/ directory.

modules/ECMConfiguredInstall.cmake
47–49

considering we are documenting the input file as .cmake.in, should we enforce this here and ignore any file not ending like that?

modules/ECMConfiguredInstall.cmake
47–49

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.

pino added inline comments.Mar 27 2020, 11:58 PM
modules/ECMConfiguredInstall.cmake
47–49

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.

kossebau added a subscriber: kossebau.EditedMar 28 2020, 5:55 AM

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_files(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, @ONLYon 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) :)

modules/ECMConfiguredInstall.cmake
13

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.

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

All of the things!

Only strips suffix ".in".

Quick review while I had some spare minutes, to keep things going.

modules/ECMConfiguredInstall.cmake
6

Perhaps "install" -> "install the result"

10

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 <file> [<file2> [...]]
#        DESTINATION <INSTALL_DIRECTORY>
#        [COPYONLY]
#        [ESCAPE_QUOTES]
#        [@ONLY]
#        [COMPONENT <component>]
#    )
19

# Since 5.69/70.0.

57

Being a regexp, the "." in ".in$" might need escaping.

63

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.

tests/ECMConfiguredInstallTest/check_tree.cmake.in
5–12

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).

kossebau added inline comments.Mar 30 2020, 6:37 PM
modules/ECMConfiguredInstall.cmake
63

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).

davidedmundson marked 3 inline comments as done.Apr 2 2020, 9:38 AM
davidedmundson added inline comments.
modules/ECMConfiguredInstall.cmake
63

COPY_ONLY I think is mutually exclusive anyway.

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

tests/ECMConfiguredInstallTest/check_tree.cmake.in
5–12

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.

davidedmundson marked an inline comment as done.

update

extra escape

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

apol added a subscriber: apol.Apr 17 2020, 1:27 PM

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

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.

modules/ECMConfiguredInstall.cmake
6

"Takes". And: configured "files", given "list of files"?

tests/ECMConfiguredInstallTest/CMakeLists.txt
3

cmake_minimum_required should be first line always, for consistent pattern.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2020, 2:30 PM
This revision was automatically updated to reflect the committed changes.

Moved to invent.