Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbus: [WIP] new version and socket activation #18382

Closed
wants to merge 2 commits into from

Conversation

peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Sep 7, 2016

Motivation for this change

This is WIP - as it requires significant recompilation, my poor laptop is currently adding to global warming. I will be back with an update once I actually manage to run this.

This PR supersedes #18222.

We now socket activate dbus, which means it will be launched on demand.

There is no longer any reason to launch it manually from the various startup scripts and the services.xserver.startDbusSession has thus been deprecated.

This also removes the need for the configurable option in #18222.

This is upstream's recommended way to run dbus.

cc: @ttuegel @lethalman @edolstra

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

We now socket activate dbus, which means it will be launched on demand.

There is no longer any reason to launch it manually from the various
startup scripts and the services.xserver.startDbusSession has thus been
deprecated.

This is upstream's recommended way to run dbus.
@mention-bot
Copy link

@peterhoeg, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ttuegel, @lethalman and @cruegge to be potential reviewers

@domenkozar
Copy link
Member

New version is already in staging

@peterhoeg
Copy link
Member Author

Thanks, but we still need the additional configure flags to get the unit files. If this PR has a chance to make it for 16.09, maybe we should hold off merging the new dbus version from staging to master until everything is OK here?

@peterhoeg
Copy link
Member Author

I can confirm that this PR works properly with KDE in a VM. I'm currently trying with XFCE but that will be another few hours before it's ready...

@domenkozar
Copy link
Member

Let's also test what happens if you restart the dbus while desktop manager is running

@peterhoeg
Copy link
Member Author

I've just tried the following, which works perfectly fine:

systemctl --user reload dbus

But, this isn't quite as good:

systemctl --user restart dbus

While the dbus service restarts correctly, everything that was activated using dbus activation is no longer running (kuiserver5, kglobalaccel, kscreen_backend_launcher). The desktop otherwise runs but of course the services provided by those 3 daemons are no longer available.

I think there are 2 points here:

a) In general the user dbus service shouldn't be restarted, which is why we set reloadIfChanged = true.

b) In the long run, dbus will not spawn services itself, but instead use systemd for spawning (the system level dbus instance does this). I have a few services here running like that (kwalletd5, kdeconnect) and they do survive the dbus restart.

@peterhoeg
Copy link
Member Author

I've just tried with xfce and restarting dbus restarts the session.

But without socket activation (and thus not using systemd to keep track of the service), killing dbus kills the xfce session, but as it isn't being auto-spawned, I'm left with just the desktop image and nothing else.

So we're actually better off with socket activation.

@peterhoeg
Copy link
Member Author

Any concerns about this? And are we too late for 16.09?

@ttuegel
Copy link
Member

ttuegel commented Sep 8, 2016

Any concerns about this? And are we too late for 16.09?

If this is ready, I can merge it into staging. It's probably too late for 16.09 because it won't even be in master for some time, but I leave the final determination there to @domenkozar.

@domenkozar
Copy link
Member

It depends on the outcome of #16514

If we decide to restart dbus on upgrade, I'd like to have this included since it improves the impact of restart.

@peterhoeg
Copy link
Member Author

@domenkozar - that's unrelated.

This PR relates to socket activation of the user dbus instance, whereas #16514 has to do with restarting the system dbus instance.

@domenkozar
Copy link
Member

@peterhoeg I thought user dbus instance is also restarted if dbus is upgraded/restarted. Is that not right?

@peterhoeg
Copy link
Member Author

They should be unrelated. I'll do some digging.

@peterhoeg
Copy link
Member Author

I've just tried in a VM. You can go wild on the system instance - it doesn't touch the user instance when restarted.

peterhoeg pushed a commit to peterhoeg/nixpkgs that referenced this pull request Sep 20, 2016
The following changes are included:

1) install user unit files from upstream dbus
2) use absolute paths to config for --system and --session instances
3) make socket activation of user units configurable

There has been a number of PRs to address this, so this one does the
bare minimum, which is to make the functionality available and
configurable but defaults to off.

Related PRs:
 - NixOS#18382
 - NixOS#18222
peterhoeg pushed a commit to peterhoeg/nixpkgs that referenced this pull request Sep 20, 2016
The following changes are included:

1) install user unit files from upstream dbus
2) use absolute paths to config for --system and --session instances
3) make socket activation of user units configurable

There has been a number of PRs to address this, so this one does the
bare minimum, which is to make the functionality available and
configurable but defaults to off.

Related PRs:
 - NixOS#18382
 - NixOS#18222
domenkozar pushed a commit that referenced this pull request Sep 30, 2016
The following changes are included:

1) install user unit files from upstream dbus
2) use absolute paths to config for --system and --session instances
3) make socket activation of user units configurable

There has been a number of PRs to address this, so this one does the
bare minimum, which is to make the functionality available and
configurable but defaults to off.

Related PRs:
 - #18382
 - #18222
domenkozar pushed a commit that referenced this pull request Sep 30, 2016
The following changes are included:

1) install user unit files from upstream dbus
2) use absolute paths to config for --system and --session instances
3) make socket activation of user units configurable

There has been a number of PRs to address this, so this one does the
bare minimum, which is to make the functionality available and
configurable but defaults to off.

Related PRs:
 - #18382
 - #18222

(cherry picked from commit f7215c9)
Signed-off-by: Domen Kožar <domen@dev.si>
@Mic92
Copy link
Member

Mic92 commented Oct 22, 2016

what is the state of this pull request?

@peterhoeg
Copy link
Member Author

Most of this has already been merged in a separate PR. I'll clean this up and either close or resubmit.

@mmahut
Copy link
Member

mmahut commented Aug 1, 2019

@peterhoeg Any update on this pull request?

@peterhoeg
Copy link
Member Author

We can close this.

@peterhoeg peterhoeg closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants