Differential D25324

[GTK Config] Add XSettingsd as a runtime dependency
ClosedPublic
Actions

Authored by gikari on Nov 15 2019, 11:02 AM.

Details

Reviewers
cblack
apol
jgrulich
ngraham
Group Reviewers
Plasma
Summary

GTK Config Module uses XSettingsd to apply settings to GTK applications
on the fly, thus making it an optional runtime dependency. To provide
this information to distributions this patch is made.

BUG: 418263
FIXED-IN: 5.18.3

Test Plan
  1. Remove xsettingsd from the system.
  2. Clear CMake Cache
  3. Run CMake
  4. Similar warning should be displayed:
-- The following RUNTIME packages have not been found:

 * XSettingsd, XSettingsd daemon
   Allows GTK Config kded module to apply settings to GTK applications on the fly
  1. Install xsettingsd
  2. Warning should not be displayed anymore
  3. Repeat

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
Branch
warn-about-xsettingsd-dependency (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18822
Build 18840: arc lint + arc unit
gikari created this revision.Nov 15 2019, 11:02 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 15 2019, 11:02 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gikari requested review of this revision.Nov 15 2019, 11:02 AM
broulik added a subscriber: broulik.Nov 15 2019, 11:30 AM

I think you would want to make that a RUNTIME dependency. This stuff is mostly for packagers, telling them to install something isn't what we want.

gikari added a comment.Nov 15 2019, 12:37 PM
In D25324#562909, @broulik wrote:

I think you would want to make that a RUNTIME dependency.

This is probably a stupid question, but how can I do that?

eszlari added a subscriber: eszlari.Nov 16 2019, 5:13 PM

I guess he refers to:

https://cmake.org/cmake/help/latest/module/FeatureSummary.html#command:set_package_properties

But seems, this only applies to dependencies checked by find_package().

gikari added a comment.Nov 16 2019, 5:38 PM
In D25324#563347, @eszlari wrote:

But seems, this only applies to dependencies checked by find_package().

I'm confused.

Does it mean, I should create custom FindXsettingsd.cmake module for xsettingsd, then write something like this:

find_package(Xsettingsd)
set_package_properties(Xsettingsd PROPERTIES
    TYPE RUNTIME
    DESCRIPTION "Provides settings to X11 applications via the XSETTINGS specification"
    URL "https://github.com/derat/xsettingsd"
)

?

eszlari added a comment.Feb 27 2020, 6:49 PM
In D25324#563373, @gikari wrote:
In D25324#563347, @eszlari wrote:

But seems, this only applies to dependencies checked by find_package().

I'm confused.

Does it mean, I should create custom FindXsettingsd.cmake module for xsettingsd, then write something like this:

find_package(Xsettingsd)
set_package_properties(Xsettingsd PROPERTIES
    TYPE RUNTIME
    DESCRIPTION "Provides settings to X11 applications via the XSETTINGS specification"
    URL "https://github.com/derat/xsettingsd"
)

?

Yes. Maybe you can take inspiration from this commit:
https://cgit.kde.org/kinfocenter.git/commit/?id=35b19106c5cb64c4705a3682269826d4be6510f2

gikari updated this revision to Diff 76627.Feb 28 2020, 12:58 PM

Add XSettingsd find module

gikari retitled this revision from Display a warning, if xsettingsd is not found to [GTK Config] Add XSettingsd as a runtime dependency.Feb 28 2020, 1:03 PM
gikari edited the summary of this revision. (Show Details)
gikari edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Mar 8 2020, 4:09 PM
ngraham added a subscriber: ngraham.

Very nice.

This revision is now accepted and ready to land.Mar 8 2020, 4:09 PM
eszlari added a comment.Mar 8 2020, 4:37 PM

BUG: 418263

This bug is not fixed by this patch. xsettingsd needs to be started in plasma-workspace/startkde/startplasma.cpp (or by systemd in the future).

FIXED-IN: 5.19

I would be nice, if this could make it to 5.18, so that xsettingsd would end up on the Kubuntu 20.04 ISO image (https://phabricator.kde.org/T12753).

gikari edited the summary of this revision. (Show Details)Mar 8 2020, 6:03 PM
gikari added a comment.EditedMar 8 2020, 6:18 PM
In D25324#624229, @eszlari wrote:

This bug is not fixed by this patch. xsettingsd needs to be started in plasma-workspace/startkde/startplasma.cpp (or by systemd in the future).

xsettingsd is started by the daemon itself, if it's not instantiated already. The daemon itself launches at startup, because it is a kded module.

eszlari added a comment.Mar 8 2020, 10:28 PM
In D25324#624299, @gikari wrote:
In D25324#624229, @eszlari wrote:

This bug is not fixed by this patch. xsettingsd needs to be started in plasma-workspace/startkde/startplasma.cpp (or by systemd in the future).

xsettingsd is started by the daemon itself, if it's not instantiated already. The daemon itself launches at startup, because it is a kded module.

You are right, sorry for the noise! I must have mixed up someting while testing it on the Kubuntu 20.04 preview.

gikari edited the summary of this revision. (Show Details)Mar 8 2020, 11:05 PM
gikari added a comment.Mar 8 2020, 11:08 PM

Ok. So, can I land it or should I wait for other reviewers? Also, should I make the dependency REQUIRED or leave it optional?

ngraham added subscribers: jriddell, cfeck.Mar 8 2020, 11:20 PM

We can't add a new required dependency since dependencies are frozen in the stable branches. Not sure what the policy is regarding adding optional ones though. @cfeck/@jriddell?

gikari edited the summary of this revision. (Show Details)Mar 10 2020, 4:25 PM
ngraham edited the summary of this revision. (Show Details)Mar 10 2020, 4:25 PM
jriddell added a comment.Mar 10 2020, 4:30 PM

I don't think this is adding a new dependency just highlighting that there is one, so it's good to add to stable branches

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (10 lines)

Diff 69795

View Options

CMakeLists.txt

Loading...