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

nixos/dbus: support dbus-broker #112879

Closed
wants to merge 2 commits into from

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

The dbus-broker project tries to improve on what was dbus-daemon, and provide a better alternative.
This PR adds support for dbus-broker, which I hope could be the default dbus daemon for NixOS.
For more information about dbus-broker see https://dvdhrm.github.io/rethinking-the-dbus-message-bus/ or this short description from Fedora. They've used dbus-broker since F30 (was released on April 30, 2019,) which should help to make this an easy switch for us.

Things done
  • I've logged into a root console and checked if dbus-broker was running
  • Inspected /etc/systemd for the units being setup in the way I expected
  • Ran a Pantheon VM , launched a terminal, and checked the status of dbus-broker
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

* add me as maintainer
* add system-console-users
* fix-paths.patch
  This I'm not 100% on.
  We make it so the systemd service isn't configured to have
  /run/current-system/sw/share in XDG_DATA_DIRS, and dbus-broker falls
  back to /usr/share. This makes it so nothing will get launched
  from /run/current-system/sw/share, and everything is limited to the
  configs (and how I believe this should be working). However, I have
  suspicion that maybe dbus-daemon knows about /run/current-system/sw/share.
@worldofpeace worldofpeace requested a review from a team February 12, 2021 10:29
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 12, 2021
@worldofpeace
Copy link
Contributor Author

My position for this PR is to try to merge this without changing defaults. And in another PR change this and maybe we can get a jobset to see that none of our tests are acting up, make release notes, and just general more scrutiny. LMK if that sounds like the way to go.

@ofborg ofborg bot requested a review from peterhoeg February 12, 2021 10:38
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 12, 2021
@r-rmcgibbo

This comment has been minimized.

@worldofpeace
Copy link
Contributor Author

A concern with I'd have to be concerned with is that dbus-broker doesn't have apparmor support bus1/dbus-broker#169.
Also, gdm has secret dependencies on dbus-run-session https://gitlab.gnome.org/GNOME/gdm/-/merge_requests/90#note_776136 as there is no dbus-run-session equivalent bus1/dbus-broker#145.
So certain dbus utilities might need to be split from dbus and installed, as this change would make everything from dbus not it environment.systemPackages. It would be a good idea to maybe also hardcode those.

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Feb 12, 2021

@xaverdh
Copy link
Contributor

xaverdh commented Feb 12, 2021

When testing this I see a lot of Ignoring duplicate name messages in the system logs. I assume these were silently ignored by dbus and dbus-broker is just more verbose about them?

@worldofpeace
Copy link
Contributor Author

When testing this I see a lot of Ignoring duplicate name messages in the system logs. I assume these were silently ignored by dbus and dbus-broker is just more verbose about them?

Yep, I believe this is true also.

@worldofpeace worldofpeace requested review from a team and removed request for a team February 14, 2021 01:56
@xaverdh
Copy link
Contributor

xaverdh commented Feb 23, 2021

So we already hard code dbus-run-session path in gdm since bcf3872 right?
From a quick grep through nixpkgs, there are quite a few other occurrences of dbus-run-session though.. is the plan to hard code all of them?

@peterhoeg
Copy link
Member

peterhoeg commented Feb 23, 2021 via email

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-21-05/11559/7

@dvdhrm
Copy link

dvdhrm commented Mar 17, 2021

I don't use GNOME, but it should no longer launch dbus manually - instead it expects systemd to have set up the socket.

On Fedora, GNOME will rely on systemd --user to provide a D-Bus user bus. However, this means there can only be a single user-bus per user (per definition), unlike the old session buses where a single user could have multiple of those. This is usually not an issue. But GDM might run multiple times on a single machine (due to the way that GDM tracks sessions and allows multiple parallel logins). Therefore, the gdm developers use dbus-run-session to spawn their own session bus for each instance of GDM and work around this limitation. This is a hacky workaround and the intention is to switch to a dynamically allocated user instead. But nobody is working on this, so it might stay that way for some time.

Long story short: GDM completely ignores the user bus and spawns its own session bus via dbus-run-session.

@xaverdh
Copy link
Contributor

xaverdh commented Apr 10, 2021

So I heard that sadly worldofpeace will no longer be actively working on nixpkgs. Could we merge this as is (maybe with a warning that its experimental and some things might not work yet)? I would very much like to use it even in the current state and could also take over the pr if this is desired.

@SuperSandro2000
Copy link
Member

So I heard that sadly worldofpeace will no longer be actively working on nixpkgs. Could we merge this as is (maybe with a warning that its experimental and some things might not work yet)? I would very much like to use it even in the current state and could also take over the pr if this is desired.

If this needs more work just create a new PR based on this one.

@ilya-fedin
Copy link
Contributor

Can someone just update dbus-broker package? 🤔
Personally I'm using dbus-broker for a long time on NixOS with a home-made module, but that old version has bugs with service activation and that's really annoying :(

@peterhoeg
Copy link
Member

@ilya-fedin - maybe just pull out the commit with the update and stick it into it's own PR? Considering there are no dependencies on it right now, that should get merged quickly.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented May 4, 2021

#121664
I picked only the system-console-users thing from here

@xaverdh xaverdh mentioned this pull request May 11, 2021
10 tasks
@xaverdh
Copy link
Contributor

xaverdh commented May 11, 2021

I rebased this on recent master (with #121664 applied) here

@xaverdh xaverdh closed this May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants