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: don't search for units in current-system #22354

Merged
merged 1 commit into from
Feb 6, 2017

Conversation

abbradar
Copy link
Member

@abbradar abbradar commented Feb 1, 2017

Motivation for this change

Continuation of #22343. With this dbus always follows configuration files properly regardless of environment: upstream configuration -> NixOS's -local.conf -> other services and cofngiurations, including system path.

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

Tested in a VM together with #22343 and with strace to confirm that dbus reads all configuration files like before. cc @layus

@copumpkin
Copy link
Member

Is this really a mass rebuild? I guess it'll mostly affect a bunch of linux-specific stuff

@abbradar
Copy link
Member Author

abbradar commented Feb 1, 2017

Hm, I thought that systemd depends on dbus (don't judge me, I wouldn't be surprised if it depends on gtk :D) but it seems to be false. Still, it affects GTK and Qt at least -- I usually classified this as a mass rebuild.

@layus
Copy link
Member

layus commented Feb 2, 2017

Hi abbradar,
have you understood the deleted comment ?

# this package installs nothing into those dirs and they create a dependency

Wat was the purpose of the removed --datadir option ?

@abbradar
Copy link
Member Author

abbradar commented Feb 2, 2017

@layus If I interpreted it properly it meant that without this flag an extra dependency (presumably between multiple outputs as it was added as part of that work) was created. I assumed that it was from dbus.lib to dbus.out but I don't see such a dependency with this patch -- and nothing else of interest. @vcunat -- if that interpretation seems wrong please correct me!

datadir in Makefile was removed because it's not needed now -- it was because otherwise all data files would be installed to /run/current-system/sw/share, not to $out/share.

@vcunat
Copy link
Member

vcunat commented Feb 2, 2017

I think the same; the dependency isn't created now. Still, I wonder if some code in nixpkgs is relying on /run/current-system/sw/share being searched for dbus units.

@abbradar
Copy link
Member Author

abbradar commented Feb 2, 2017

@vcunat There is I think, and dbus.nix handles that by adding config.system.path to dbus paths.

@layus
Copy link
Member

layus commented Feb 2, 2017

Then what is the point of the services.dbus.packages option ?

@abbradar
Copy link
Member Author

abbradar commented Feb 2, 2017

@layus To avoid installing to environment everything that installs a dbus service. IMHO we want most if not all system services to be installed like that, but this directory was added historically.

@layus
Copy link
Member

layus commented Feb 2, 2017

At the end of the day, It adds confusion to search both services.dbus.packages and config.system.path. If we have services.dbus.packages, why even look into config.system.path ? You may want a package in the system path and another in dbus config.

@abbradar
Copy link
Member Author

abbradar commented Feb 2, 2017

I agree but it may be very difficult to get rid of this (unknown number of modules depend on this) and this will break users configuration (off the top of my head -- blueman installed to environment.systemPackages will break). Also one may argue that this is convenient for users (not sure about this one).

@layus
Copy link
Member

layus commented Feb 2, 2017

agreed, indeed. Until someone complains :-).
Curious you should mention blueman. I was trying to install it and could not make it work with dbus...

@abbradar
Copy link
Member Author

abbradar commented Feb 2, 2017

See #22353 -- with this OBEX and even networking finally works for me out of the box.

@abbradar
Copy link
Member Author

abbradar commented Feb 4, 2017

This worked okay in a VM so let's merge this in few days.

@abbradar abbradar merged commit 23d47ac into NixOS:staging Feb 6, 2017
@abbradar
Copy link
Member Author

It seems that this is not the end of the story, but at least it helped to uncover an old bug :D

Currently it manifests itself as inability to start services as root. After investigation in the dbus source code this is my prelimitary understanding: the reason is that dbus-daemon-launch-helper uses hardcoded service.conf path and a trimmed version of configuration parser which doesn't support <include>. Therefore we don't have system-local.conf included for purposes of this helper. The problem was masked before because datadir was set to /run/current-system/sw/share, so dbus services that were installed into environment worked fine. It was still broken though because that didn't work for services.dbus.packages.

The first strategy that comes to mind is to set datadir to /etc and place there specially modified system.conf which contains all <include>s by itself. I'll try to implement this -- sadly this will cause another dbus rebuild. I'll also make sure to comment this so we remember this problem later. Sorry for the trouble!

@abbradar abbradar mentioned this pull request Feb 16, 2017
7 tasks
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

4 participants