nixos/dbus: support dbus-broker

[xaverdh]

Motivation for this change

picking up #112879

Things done

• 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.

[xaverdh]

So I am running this on my system now. If people want to try it out, feel free :)

@ilya-fedin since you have written your own module, is something important missing from this pr / do you have suggestions?

[ilya-fedin]

since you have written your own module, is something important missing from this pr / do you have suggestions?

I don't think so, my module feels like a hack comparing to that one :)
https://github.com/ilya-fedin/nur-repository/blob/master/modules/dbus-broker/default.nix

[xaverdh]

Ok, so I ran a few tests on branch which defaults to the broker imp. (on top of this).
Xfce and Gnome seem to pass, Plasma currently hangs so probably needs some work.

[etircopyh]

@xaverdh how's it going?

[stale]

I marked this as stale due to inactivity. → More info

[06kellyjac]

I think this is still important

[06kellyjac]

is there a reason this is draft? After conflicts are resolved would this be good to go?

[xaverdh]

The plasma5-systemd-start test now succeeds on this branch on top of #172845. Maybe we can make this available as an experimental option soon.

[ashkitten]

this module doesn't add pkgs.dbus to environment.systemPackages like services.dbus.enable does, which makes dunstctl not work since it uses dbus-send

[ilya-fedin]

Maybe dunstctl should be fixed then? Personally I don't use dbus-send and I absolutely don't need anything other than libdbus for applications.

[ashkitten]

you're right, it should be added to dunst's dependencies. my intention was to bring awareness to the fact that this is a breaking change in behavior between services.dbus and services.dbus-broker.

[ashkitten]

while updating my system with this, it stopped dbus-broker.service which broke a whole bunch of things. i think even though dbus.service is set to be reloaded instead of restarted, dbus-broker.service is not (even though it's aliased to dbus.service)

[xaverdh]

while updating my system with this, it stopped dbus-broker.service which broke a whole bunch of things. i think even though dbus.service is set to be reloaded instead of restarted, dbus-broker.service is not (even though it's aliased to dbus.service)

Indeed. Not sure why reloadIfChanged is not respected here..

[xaverdh]

Even if I explicitly add X-ReloadIfChanged to the service config of both (dbus and dbus-broker service files) it is still restartet..

cc @flokli might know what is happening here?

[xaverdh]

Maybe we should just use reloadTriggers instead of restartTriggers + reloadIfChanged? seems like the proper thing to do..
But I do wonder why it works on master and what makes it no longer work here.

[xaverdh]

while updating my system with this, it stopped dbus-broker.service which broke a whole bunch of things. i think even though dbus.service is set to be reloaded instead of restarted, dbus-broker.service is not (even though it's aliased to dbus.service)

Can you check, if this is still the case?
I came back to this after a while and it worked on my system now..

[xaverdh]

But its not systemd that handles restarting / reloading in this case, but switch-to-configuration.pl. As I understand it, it looks for X-ReloadIfChanged and if that is set on the unit reloads it instead of restarting. Now if that is set on the alias unit instead of the unit proper, it will erroneously restart it.

[xaverdh]

Ok, maybe I misstated it above. Its not so much about X-Restart-Triggers, but rather about what happens when the switch-to-configuration logic decides that dbus-broker should be restarted.

[xaverdh]

This makes me think, we probably should set reloadIfChanged on the dbus unit, even when using the broker implementation..

[m-bdf]

Ok, maybe I misstated it above. Its not so much about X-Restart-Triggers, but rather about what happens when the switch-to-configuration logic decides that dbus-broker should be restarted.

ahh gotcha!

This makes me think, we probably should set reloadIfChanged on the dbus unit, even when using the broker implementation..

Agreed then, that sounds reasonable

[xaverdh]

Ok, so I set reloadIfChanged on the dbus alias as well, just to be sure.

[m-bdf]

After a bit of testing, LGTM

[lilyinstarlight]

I've been running this full-time on my laptop for a while now and after the reloadIfChanged issue was fixed, I've had no problems since

[AndersonTorres]

Anyone disagree about merging this?

[jtojnar]

I do not feel very good about mkDefault for a critical dependency like this. But I am not sure how else to implement this other than with services.dbus.implementation = lib.mkOption { type = lib.mkEnum [ "dbus" "dbus-broker" ]; } which is not particularly nice either. So do not take this as NAK.

[m-bdf]

Good remark
An alternative would be services.dbus.broker = true, or maybe even services.dbus.broker.enable = true to allow for potential broker-specific options under services.dbus.broker (which I'm not sure will ever be needed).
However these look like they enable something in the existing dbus daemon rather than replace it entirely, so services.dbus.implementation = "dbus-broker" as you suggested makes more sense to me.

[jtojnar]

The mkDefaults should be removed as well, then.

[m-bdf]

And mkIf cfg.enable can now be moved to wrap the entire config:

  config = mkIf cfg.enable (mkMerge [
    {
      …
    }

    (mkIf (cfg.implementation == "dbus") {
      …
    })

    (mkIf (cfg.implementation == "dbus-broker") {
      …
    })
  ]);

[m-bdf]

Looking at the D-Bus derivation's source code, it seems pkgs.dbus, pkgs.dbus.out and pkgs.dbus.daemon all refer to the exact same output:

nixpkgs/pkgs/development/libraries/dbus/default.nix

Line 111 in b457130

daemon = dbus.out;

Should we choose one and stick to it?

[jtojnar]

@m-bdf That is probably best done separately, as not to widen the scope of this PR and delay it even more.

Edit: opened #201647.

[SuperSandro2000]

Please rebase on master

[xaverdh]

Please rebase on master

done.

[peterhoeg]

I just tried this out. And not only is my laptop fine, but I also noticed a 10% increase in CPU power, 2 hours additional battery time, the sun shining significantly sharper and my long lost dog has finally returned.

Very nice work all. Merging this and if we need to clean anything else up and possibly change the default, we can always do that.

[lilyinstarlight]

Per @mweinelt in #dev:nixos.org (Matrix), this can be backported for inclusion in 22.11 if it does not change default behavior

If you are both comfortable with that @xaverdh and @peterhoeg, I would like for it to be backported for the upcoming release

[Mic92]

would be good to get this tested before so we can evaluate if we want this by default.

[github-actions]

Successfully created backport PR #202365 for release-22.11.

[xaverdh]

would be good to get this tested before so we can evaluate if we want this by default.

agreed!