D28305: WIP: Systemd Startup

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> hchain wrote in plasma-core@.target:3
> can we make this `plasma-kwin@%i.service` ?

can do, the _ is just to match the binary name

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D28305: WIP: Systemd Startup

[Henri Chain]

hchain added inline comments.

INLINE COMMENTS

> plasma-core@.target:3
> +Description=KDE Plasma Workspace Core
> +Wants=plasma-plasmashell.service plasma-kwin_%i.service 
> plasma-kcminit.service plasma-kded.service plasma-kcminit-phase1.service
> +Requires=plasma-ksmserver.service

can we make this `plasma-kwin@%i.service` ?

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D28305: WIP: Systemd Startup

[David Edmundson]

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


  support x11 and wayland ...ish
  
  we now call target plasma-workspace@x11 or plasma-workspace@wayland   it then 
can call the right services
  but kwin_wayland is still currently spawned before the systemd stuff

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28305?vs=79106&id=80412

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  gmenu-dbusmenu-proxy/gmenudbusmenuproxy.desktop
  gmenu-dbusmenu-proxy/plasma-gmenudbusproxy.service.in
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/plasma-krunner.service.in
  ksmserver/CMakeLists.txt
  ksmserver/plasma-ksmserver.service.in
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasma-plasmashell.service.in
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/plasma-kcminit-phase1.service.in
  startkde/kcminit/plasma-kcminit.service.in
  startkde/plasma-core@.target
  startkde/plasma-ksplash-ready.service.in
  startkde/plasma-workspace@.target
  startkde/startplasma.cpp
  xembed-sni-proxy/CMakeLists.txt
  xembed-sni-proxy/plasma-xembedsniproxy.service.in
  xembed-sni-proxy/xembedsniproxy.desktop

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D28305: WIP: Systemd Startup

[David Edmundson]

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


  Add xembedsniproxy and gdbusmenuproxy

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28305?vs=78922&id=79106

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  gmenu-dbusmenu-proxy/gmenudbusmenuproxy.desktop
  gmenu-dbusmenu-proxy/plasma-gmenudbusproxy.service.in
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/plasma-krunner.service.in
  ksmserver/CMakeLists.txt
  ksmserver/plasma-ksmserver.service.in
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasma-plasmashell.service.in
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/plasma-kcminit-phase1.service.in
  startkde/kcminit/plasma-kcminit.service.in
  startkde/plasma-core.target
  startkde/plasma-ksplash-ready.service.in
  startkde/plasma-workspace.target
  startkde/startplasma.cpp
  xembed-sni-proxy/CMakeLists.txt
  xembed-sni-proxy/plasma-xembedsniproxy.service.in
  xembed-sni-proxy/xembedsniproxy.desktop

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[David Edmundson]

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


  update

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28305?vs=78917&id=78922

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/plasma-krunner.service.in
  ksmserver/CMakeLists.txt
  ksmserver/plasma-ksmserver.service.in
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasma-plasmashell.service.in
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/plasma-kcminit-phase1.service.in
  startkde/kcminit/plasma-kcminit.service.in
  startkde/plasma-core.target
  startkde/plasma-ksplash-ready.service.in
  startkde/plasma-workspace.target
  startkde/startplasma.cpp

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[David Edmundson]

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


  update

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28305?vs=78541&id=78917

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/plasma-krunner.service.in
  ksmserver/CMakeLists.txt
  ksmserver/plasma-ksmserver.service.in
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasma-plasmashell.service.in
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/plasma-kcminit-phase1.service.in
  startkde/kcminit/plasma-kcminit.service.in
  startkde/plasma-core.target
  startkde/plasma-ksplash-ready.service.in
  startkde/plasma-workspace.target
  startkde/startplasma.cpp

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[Henri Chain]

hchain added inline comments.

INLINE COMMENTS

> startplasma.cpp:404
> +startPlasmaSession.start(QStringLiteral(CMAKE_INSTALL_FULL_BINDIR 
> "/plasma_session"), plasmaSessionOptions);
> +rc = startPlasmaSession.waitForStarted();
> +#else

unrelated change ?

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[Henri Chain]

hchain added inline comments.

INLINE COMMENTS

> hchain wrote in plasmawayland-dev.desktop.cmake:2
> does that still work in legacy (non-systemd) mode ?

original patch https://phabricator.kde.org/D19004

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[Henri Chain]

hchain added inline comments.

INLINE COMMENTS

> apol wrote in plasmawayland-dev.desktop.cmake:2
> Is this change related?

does that still work in legacy (non-systemd) mode ?

> apol wrote in org.kde.plasmashell.desktop.cmake:55
> We probably want it always hidden?

maybe add a comment that this file is superseded in systemd-mode by a custom 
service and we don't want the systemd generator to make another one for us

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: hchain, apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> ahiemstra wrote in CMakeLists.txt:44
> This pattern is repeated multiple times, maybe we should make a macro out of 
> it? Like `install_systemd_service_file(source_file)`

that's a good idea

> apol wrote in ksmserver.service.cmake:8
> Why is there a - here after the =?

there's a comment above that line

> apol wrote in plasmawayland-dev.desktop.cmake:2
> Is this change related?

Related, yes.
Is this the right commit for it, probably not.

If we nest another dbus-session inside the system one everything gets confused.

Start dbus-manually shouldn't be our repsonsibility but apparently it's needed 
on some setups that don't have the combined systemd+dbus setups. See 404335

> apol wrote in org.kde.plasmashell.desktop.cmake:55
> We probably want it always hidden?

Right, this line currently does nothing.

It's meant to tell the systemd generator to skip this as we have a unit 
already, but we're going to use a common prefix at some point.

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[Aleix Pol Gonzalez]

apol added a comment.


  Change looks good to me overall +1

INLINE COMMENTS

> ksmserver.service.cmake:8
> +# This magic minus sign means don't fail if exit code is non-zero...
> +ExecStartPost=-@QtBinariesDir@/qdbus org.kde.KSplash /KSplash 
> org.kde.KSplash.setStage ksmserver
> +BusName=org.kde.ksmserver

Why is there a - here after the =?

> plasmawayland-dev.desktop.cmake:2
>  [Desktop Entry]
> -Exec=dbus-run-session @CMAKE_INSTALL_FULL_LIBEXECDIR@/startplasma-dev.sh 
> -wayland
> +Exec=@CMAKE_INSTALL_FULL_LIBEXECDIR@/startplasma-dev.sh -wayland
>  DesktopNames=KDE

Is this change related?

> org.kde.plasmashell.desktop.cmake:55
>  X-KDE-StartupNotify=false
> +X-KDE-HiddenUnderSystemd=true
>  X-DBUS-ServiceName=org.kde.plasmashell

We probably want it always hidden?

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: apol, ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D28305: WIP: Systemd Startup

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> CMakeLists.txt:44
>  
> +configure_file(krunner.service.cmake 
> ${CMAKE_CURRENT_BINARY_DIR}/krunner.service @ONLY)
> +install( FILES  ${CMAKE_CURRENT_BINARY_DIR}/krunner.service DESTINATION

This pattern is repeated multiple times, maybe we should make a macro out of 
it? Like `install_systemd_service_file(source_file)`

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: ahiemstra, benjaminberg, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D28305: WIP: Systemd Startup

[David Edmundson]

davidedmundson added a comment.


  In terms of testing, you will need:
  
  - this patch
  - to rerun ./login-sessions/install-sessions.sh
  - cherry-picked the relevant repos in  kded, kwin, kactivitymanagerd
  
  This will give you a basic session, for everything , you'll also want
  
  optionally for full autostart for 3rd party things like next cloud and stuff:
  
  - https://github.com/systemd/systemd/pull/15226 and 
https://github.com/systemd/systemd/pull/15230

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson
Cc: benjaminberg, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28305: WIP: Systemd Startup

[David Edmundson]

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


  add comments explaining some code

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28305?vs=78539&id=78541

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/krunner.service.cmake
  ksmserver/CMakeLists.txt
  ksmserver/ksmserver.service.cmake
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasmashell.service.cmake
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/kcminit-phase1.service.cmake
  startkde/kcminit/kcminit.service.cmake
  startkde/ksplash-ready.service.cmake
  startkde/plasma-core.target
  startkde/plasma-workspace.target
  startkde/startplasma.cpp

To: davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28305: WIP: Systemd Startup

[David Edmundson]

davidedmundson created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
davidedmundson requested review of this revision.

REVISION SUMMARY
  This brings numerous advantages such as:
  
  - easier admin configuration with drop-ins, overrides a
  
  nd multiple hooks throughout (and only having to learn one tool)
  
  - session cleanup on exit, avoiding that occasional part where shutdown
  
  hangs
  
  - startup that actually knows when things are up
  - race free autostart and DBus activation at once
  - logs that rotate are split by service and usable
  - resource management through slices and cgroups (the part I want)
  
  Over the past 2 years I've been trying to tidy up and encapsulate the
  relevant parts of startup into the binary plasma-session so that we can
  just runtime swap out that one part and supporting both paths will be
  easy.
  
  Support is toggleable via cmake flag, as it seems like it should be a
  distro decision, especially as we will require a specific systemd with
  the xdg-generator.
  
  KDED/kwin/other services are attached to the relevant repo.
  
  It is WIP as:
  
  - it requires unmerged generator for regular autostart files
  - There are outstanding tasks, see below
  
  Task T11914 
  
  Uploaded to get more people on board and also because merging master
  into a feature branch is becoming really annoying.
  
  Given it's cmake guarded, I'm also up for merging before completion if
  others are happy?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  config-workspace.h.cmake
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/krunner.service.cmake
  ksmserver/CMakeLists.txt
  ksmserver/ksmserver.service.cmake
  ksmserver/server.cpp
  login-sessions/plasmawayland-dev.desktop.cmake
  login-sessions/plasmawayland.desktop.cmake
  login-sessions/startplasma-dev.sh.cmake
  shell/CMakeLists.txt
  shell/org.kde.plasmashell.desktop.cmake
  shell/plasmashell.service.cmake
  startkde/CMakeLists.txt
  startkde/kcminit/CMakeLists.txt
  startkde/kcminit/kcminit-phase1.service.cmake
  startkde/kcminit/kcminit.service.cmake
  startkde/ksplash-ready.service.cmake
  startkde/plasma-core.target
  startkde/plasma-workspace.target
  startkde/startplasma.cpp

To: davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart