Differential D29051

Add ecm_generate_dbus_service_file
Needs ReviewPublic
Actions

Authored by broulik on Apr 21 2020, 2:22 PM.

Details

Reviewers
davidedmundson
kossebau
kfunk
habacker
Group Reviewers
Frameworks
Summary

It serves as a replacement for kdbusaddons_generate_dbus_service_file.

An application can be a DBus-activated service just fine without using KDBusAddons.
Moreover, this new module uses named arguments for future-proofing, and adds support for specifying a SystemdService.
It also cleans up the confusion on what the "path" is about: Rather than requiring to specify executable and path separately, we just extract the executable file name on Windows, if necessary.

Usage:

ecm_generate_dbus_service_file(NAME org.kde.kded5
                               EXECUTABLE ${KDE_INSTALL_FULL_BINDIR}/kded5
                               SYSTEMD_SERVICE plasma-kded)
Test Plan
  • Was able to generate a kded service file
  • Was able to generate a kded service file with SystemdUnit
  • Verified that it moaned when executable wasn't an absolute path
  • Untested on Windwos

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Apr 21 2020, 2:22 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptApr 21 2020, 2:22 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.Apr 21 2020, 2:22 PM
broulik edited the test plan for this revision. (Show Details)Apr 21 2020, 2:26 PM
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
kossebau added a comment.Apr 21 2020, 2:43 PM

Some first quick comments, not yet looked at code itself.

Misses also link file from doc/.

modules/ECMGenerateDBusServiceFile.cmake
7

generate and install

23

Would be nice to have an example.
Also having each argument discussed in an own section makes getting the docs easier,

91

Creates a dependecy on KDEInstallDirs.

ECM/Modules are supposed to be usable in non-KDE-typical setups. So like other places in this subfolder this macro needs an argument to pass the installation folder.

broulik updated this revision to Diff 80775.Apr 21 2020, 2:56 PM
  • Improve docs
  • Add DESTINATION arg
broulik updated this revision to Diff 80776.Apr 21 2020, 2:58 PM

Add docs rst

kossebau added a comment.Apr 23 2020, 1:20 PM

Any chance for a simple unit test to check the generation does what is expected (or catches bad input)? :)

modules/ECMGenerateDBusServiceFile.cmake
23

This might be misunderstood that people on the caller side need to do an if/else switch for what to pass as EXECUTABLE.
Perhaps change to say this should be the absolute path (and give a hint that with KDEInstallDirs being used this should be then the _FULL variant of the variable.)
The actual special handling for the Windows case could be mentioned as a note only, for the curious user.

25

This leaves me puzzled what values are exactly accepted here, should get more details (and perhaps a separate example).

broulik marked 3 inline comments as done.May 22 2020, 8:08 AM
broulik updated this revision to Diff 83105.May 22 2020, 8:17 AM
  • Clarify docs
kossebau added a comment.May 29 2020, 10:22 AM

A unit test would be good to have. The test for ECMGeneratePkgConfigFile might be a sample for this.

modules/ECMGenerateDBusServiceFile.cmake
17

Computer says: .../extra-cmake-modules/modules/ECMGenerateDBusServiceFile.cmake:17: WARNING: Inline literal start-string without end-string.

20

I would propopse "to the installed service executable.", to avoid the misunderstanding this is about the copy in the build system.

21

Same issue: KDEInstallDirs misses double single quotes after it.

23

Perhaps phrase it like: "Note: On Windows, the macro will only use the file name part of <executable> since D-Bus service executables are to be installed in the same directory as the D-Bus daemon."
Current text still leaves chance to misunderstand that the user in "is used" is the macro caller, not the macro itself :)

Revision Contents
Changeset List

Diff 83105

View Options

docs/module/ECMGenerateDBusServiceFile.rst

Loading...
View Options

modules/ECMGenerateDBusServiceFile.cmake

Loading...