Move kcminit_startup and kded to plasma-session
ClosedPublic

Authored by apol on Feb 17 2020, 1:54 PM.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22617
Build 22635: arc lint + arc unit
apol created this revision.Feb 17 2020, 1:54 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 17 2020, 1:54 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Feb 17 2020, 1:54 PM
apol updated this revision to Diff 75827.Feb 17 2020, 1:54 PM

Removed unused code

davidedmundson accepted this revision.Feb 17 2020, 2:04 PM
This revision is now accepted and ready to land.Feb 17 2020, 2:04 PM
This revision was automatically updated to reflect the committed changes.
lbeltrame added a subscriber: lbeltrame.EditedFeb 18 2020, 6:41 AM

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.

At the same time startplasma-x11 hangs forever (but I did not have time to attach gdb to look at what it was doing).

fvogt reopened this revision.Feb 18 2020, 8:37 AM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
startkde/plasma-session/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.

This revision is now accepted and ready to land.Feb 18 2020, 8:37 AM
fvogt requested changes to this revision.Feb 18 2020, 8:37 AM
This revision now requires changes to proceed.Feb 18 2020, 8:37 AM
davidedmundson added inline comments.Feb 18 2020, 8:49 AM
startkde/plasma-session/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);

fvogt added inline comments.Feb 18 2020, 8:51 AM
startkde/plasma-session/startup.cpp
211

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.

Fix process env
Correctly wait for kcminit phase 0 to finish

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.

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.

rebase and squash changes

apol added a comment.Feb 18 2020, 2:56 PM

Both the isServiceRegistered and the env changes make sense to me.

startkde/plasma-session/startup.cpp
231–233

Unrelated change.

422

Maybe it would make sense to do the merging of envs here? This way we only set when it's necessary.

only pass in adjusted env

apol added a comment.Feb 20 2020, 2:41 PM

LGTM otherwise

startkde/plasma-session/startup.cpp
422

only do it if (!additionalEnv.isEmpty())

457

only do it if (!additionalEnv.isEmpty())

davidedmundson accepted this revision.Feb 26 2020, 11:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 26 2020, 11:53 AM
This revision was automatically updated to reflect the committed changes.