D27457: Move kcminit_startup and kded to plasma-session
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 R120:c673a8322630: Move kcminit_startup and kded to plasma-session (authored by davidedmundson). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=76045&id=76455 REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h startkde/startplasma-waylandsession.cpp startkde/startplasma-x11.cpp startkde/startplasma.cpp To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, 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
D27457: Move kcminit_startup and kded to plasma-session
apol added a comment. LGTM otherwise INLINE COMMENTS > startup.cpp:422 > +m_process->setArguments(args); > +auto env = QProcessEnvironment::systemEnvironment(); > +env.insert(additionalEnv); only do it `if (!additionalEnv.isEmpty())` > startup.cpp:450 > +m_process->setArguments(args); > +auto env = QProcessEnvironment::systemEnvironment(); > +env.insert(additionalEnv); only do it if (!additionalEnv.isEmpty()) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson updated this revision to Diff 76045. davidedmundson added a comment. fixup diff REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=76044&id=76045 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h startkde/startplasma-waylandsession.cpp startkde/startplasma-x11.cpp startkde/startplasma.cpp To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson updated this revision to Diff 76044. davidedmundson added a comment. only pass in adjusted env REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=75908&id=76044 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
apol added a comment. Both the isServiceRegistered and the env changes make sense to me. INLINE COMMENTS > startup.cpp:232 > connect(loginSound, &NotificationThread::finished, loginSound, > &NotificationThread::deleteLater); > loginSound->start();}); > Unrelated change. > startup.cpp:421 > +m_process->setArguments(args); > +m_process->setProcessEnvironment(env); > + Maybe it would make sense to do the merging of envs here? This way we only set when it's necessary. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson updated this revision to Diff 75908. davidedmundson added a comment. rebase and squash changes REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=75901&id=75908 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson added a comment. > As there's no reason to clear the environment, wouldn't it be more useful if StartServiceJob would only allow adding/overwriting variables? Possibly. My logic was that I didn't want to lose the possibility to unset something. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
fvogt added a comment. As there's no reason to clear the environment, wouldn't it be more useful if `StartServiceJob` would only allow adding/overwriting variables? `StartProcessJob` does not need an option to set the environment at all AFAICT. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson updated this revision to Diff 75901. davidedmundson added a comment. Fix process env Correctly wait for kcminit phase 0 to finish REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=75834&id=75901 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
fvogt added inline comments. INLINE COMMENTS > davidedmundson wrote in startup.cpp:211 > StartProcessJob I think is fine. It's not set, so it'll inherit. > > It's the extra arg to StartServiceJob that has potential to wipe the env. > > It defaults to empty > > and we do p->setEnvironment(m_env); > StartProcessJob I think is fine. It's not set, so it'll inherit. It is set in line 440, which defaults to empty (line 90 in the header) as well. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
davidedmundson added inline comments. INLINE COMMENTS > fvogt wrote in startup.cpp:211 > This (and below) are started with an empty environment, which means that > neither `DISPLAY`, `WAYLAND_DISPLAY` or `XAUTHORITY` are set. So everything > breaks horribly, as seen by @lbeltrame and openQA. StartProcessJob I think is fine. It's not set, so it'll inherit. It's the extra arg to StartServiceJob that has potential to wipe the env. It defaults to empty and we do p->setEnvironment(m_env); REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson, fvogt Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
fvogt reopened this revision.
fvogt added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> startup.cpp:211
> +const QVector sequence = {
> +new StartProcessJob(QStringLiteral("kcminit_startup"), {}),
> +new StartServiceJob(QStringLiteral("kded5"), {},
> QStringLiteral("org.kde.kded"), QProcess::systemEnvironment() << QStringList{
> QStringLiteral("KDED_STARTED_BY_KDEINIT=1") }),
This (and below) are started with an empty environment, which means that
neither `DISPLAY`, `WAYLAND_DISPLAY` or `XAUTHORITY` are set. So everything
breaks horribly, as seen by @lbeltrame and openQA.
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D27457
To: apol, #plasma, davidedmundson
Cc: fvogt, lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack,
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin,
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol,
ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
lbeltrame added a comment. This completely breaks startup in my system and in openQA: https://openqa.opensuse.org/tests/1177118#step/finish_desktop/6 (Notice that there is *no* desktop loaded). Basically many programs that start early complain that the Qt plugin can't be found because they can't connect to the display (started too early?): feb 17 23:38:40 leon kwalletd5[1661]: qt.qpa.xcb: could not connect to display [...] feb 17 23:38:40 leon kwalletd5[1661]: qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27457 To: apol, #plasma, davidedmundson Cc: lbeltrame, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
This revision was automatically updated to reflect the committed changes. Closed by commit R120:930e991b0e7e: Move kcminit_startup and kded to plasma-session (authored by apol). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=75827&id=75834 REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h startkde/startplasma-waylandsession.cpp startkde/startplasma-x11.cpp startkde/startplasma.cpp To: apol, #plasma, davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
apol created this revision. apol added a reviewer: Plasma. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. apol requested review of this revision. TEST PLAN Used it to start plasma REPOSITORY R120 Plasma Workspace BRANCH apol/kdedkcmtosession REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h startkde/startplasma-waylandsession.cpp startkde/startplasma-x11.cpp startkde/startplasma.cpp To: apol, #plasma Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D27457: Move kcminit_startup and kded to plasma-session
apol updated this revision to Diff 75827. apol added a comment. Removed unused code REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27457?vs=75826&id=75827 BRANCH apol/kdedkcmtosession REVISION DETAIL https://phabricator.kde.org/D27457 AFFECTED FILES startkde/plasma-session/startup.cpp startkde/plasma-session/startup.h startkde/startplasma-waylandsession.cpp startkde/startplasma-x11.cpp startkde/startplasma.cpp To: apol, #plasma Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
