redshift/geoclue/localtime: progress in fixing agent confusion #44820

michaelpj · 2018-08-09T15:09:45Z

Motivation for this change

Partially fixes #44725.

geoclue recently started requiring an authorization agent in order for clients to get data. For example, they provide a demo agent that asks the user via a notification if they want to allow the application access to their location.

DEs are supposed to provide their own agent, but only GNOME does so. However, we can use the demo agent provided by geoclue. AFAICT, this needs to run as a user service, because it is associated with the user who makes the requests.

This makes redshift work again (albeit with an annoying notification prompt), after a bit more wrangling because the agent also expects a valid .desktop file and appears to check that the executable exists.

However, I couldn't make localtime work, because it runs as a system service, and there isn't an agent for it's UID. I think the thing to do would be to make it run as a user service, but my preliminary hacking was not enough to make this work.

cc @garbas @yegortimoshenko as maintainers of relevant packages.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

michaelpj · 2018-08-09T15:11:34Z

Oh, and FWIW I think the breaking commit in geoclue is this one, which appears in 4.9. We recently upgraded from 4.8 -> 4.10, which is presumably when this started.

jtojnar · 2018-08-09T15:52:25Z

pkgs/development/libraries/geoclue/default.nix

@@ -1,24 +1,29 @@
 { fetchurl, stdenv, intltool, pkgconfig, gtk-doc, docbook_xsl, docbook_xml_dtd_412, glib, json-glib, libsoup, libnotify, gdk_pixbuf
 , modemmanager, avahi, glib-networking, wrapGAppsHook, gobjectIntrospection
-, withDemoAgent ? false
+# doc build currently seems to be broken


Could you provide more details? The docs build for me.

If I set this to true I get the following:

> nix build -f . pkgs.geoclue builder for '/nix/store/mgyaj5vnzg1fy2yvx16xqk11r6kiqq9v-geoclue-2.4.11.drv' failed with exit code 2; last 10 log lines: ../libgeoclue-docs.xml:77: element include: XInclude error : could not load ../xml/gclue-client-proxy.xml, and no fallback was found warning: failed to load external entity "../xml/gclue-location-proxy.xml" ../libgeoclue-docs.xml:83: element include: XInclude error : could not load ../xml/gclue-location-proxy.xml, and no fallback was found make[3]: *** [Makefile:750: html-build.stamp] Error 6 make[3]: Leaving directory '/build/geoclue-2.4.11/docs/lib' make[2]: *** [Makefile:577: all-recursive] Error 1 make[2]: Leaving directory '/build/geoclue-2.4.11/docs' make[1]: *** [Makefile:517: all-recursive] Error 1 make[1]: Leaving directory '/build/geoclue-2.4.11' make: *** [Makefile:449: all] Error 2 [0 built (1 failed), 0.0 MiB DL] error: build of '/nix/store/mgyaj5vnzg1fy2yvx16xqk11r6kiqq9v-geoclue-2.4.11.drv' failed

This is with sandboxing on. I don't get this in the previous version, but I couldn't figure out what the problem.

Recent master seems to have a bunch of build-related commits, so I suspect they may still be fixing things up.

jtojnar · 2018-08-09T15:55:30Z

pkgs/development/libraries/geoclue/default.nix

-, withDemoAgent ? false
+# doc build currently seems to be broken
+, enableDocs ? false
+, withDemoAgent ? true


Demo agent should not be build by default, since the default use is for libraries. Override the value in the module when needed.

I can do that, but then we'll build it twice. As is presumably libraries can just depend on geoclue2.dev, so I don' think this should affect closure size?

geoclue2.dev is only used for build, the library is still in out. Maybe we could split lib to a separate output. Though building such a small program twice should not really matter.

Okay, fine with me.

jtojnar · 2018-08-09T16:20:48Z

nixos/modules/services/desktops/geoclue2.nix

@@ -4,6 +4,10 @@

 with lib;

+let
+  # the demo agent isn't built by default, but we need it here
+  package = pkgs.geoclue2.override { withDemoAgent = true; };


Can it be set to config.services.geoclue2.enableDemoAgent?

michaelpj · 2018-08-09T16:30:34Z

If you can work out how to fix the docs I'm up for it, but I'd really like to get redshift working again.

jtojnar · 2018-08-09T16:40:04Z

Will try to bisect it later.

michaelpj · 2018-08-09T16:43:02Z

You have more patience than me :D

jtojnar · 2018-08-09T16:54:13Z

Builds fine from git:

{ fetchFromGitLab, stdenv, autoconf, automake, libtool, intltool, pkgconfig, gtk-doc, docbook_xsl, docbook_xml_dtd_412, glib, json-glib, libsoup, libnotify, gdk_pixbuf
, modemmanager, avahi, glib-networking, wrapGAppsHook, gobjectIntrospection
, withDemoAgent ? false
}:

with stdenv.lib;

stdenv.mkDerivation rec {
  name = "geoclue-${version}";
  version = "2.4.11";

  src = fetchFromGitLab {
    domain = "gitlab.freedesktop.org";
    owner = "geoclue";
    repo = "geoclue";
    rev = version;
    sha256 = "11pvw7cif1jp305qag85br9pwda5h9in9377cwzy82wfb0inyrf2";
  };

  outputs = [ "out" "dev" "devdoc" ];

  nativeBuildInputs = [
    autoconf automake libtool pkgconfig intltool gtk-doc docbook_xsl docbook_xml_dtd_412 wrapGAppsHook gobjectIntrospection
  ];

  buildInputs = [
    glib json-glib libsoup avahi
  ] ++ optionals withDemoAgent [
    libnotify gdk_pixbuf
  ] ++ optionals (!stdenv.isDarwin) [ modemmanager ];

  propagatedBuildInputs = [ glib glib-networking ];

  configureFlags = [
    "--with-systemdsystemunitdir=$(out)/etc/systemd/system"
    "--enable-introspection"
    "--enable-gtk-doc"
    "--enable-demo-agent=${if withDemoAgent then "yes" else "no"}"
  ] ++ optionals stdenv.isDarwin [
    "--disable-silent-rules"
    "--disable-3g-source"
    "--disable-cdma-source"
    "--disable-modem-gps-source"
    "--disable-nmea-source"
  ];

  postPatch = ''
    sed -i '/git submodule/d' autogen.sh
  '';

  preConfigure = "NOCONFIGURE=1 ./autogen.sh";

  # https://gitlab.freedesktop.org/geoclue/geoclue/issues/73
  postInstall = ''
    sed -i $dev/lib/pkgconfig/libgeoclue-2.0.pc -e "s|includedir=.*|includedir=$dev/include|"
  '';

  meta = with stdenv.lib; {
    description = "Geolocation framework and some data providers";
    homepage = https://gitlab.freedesktop.org/geoclue/geoclue/wikis/home;
    maintainers = with maintainers; [ raskin garbas ];
    platforms = with platforms; linux ++ darwin;
    license = licenses.lgpl2;
  };
}

Could you maybe report a bug upstream?

michaelpj · 2018-08-09T17:09:58Z

I'm not sure I know what the issue is - do you think there's something missing in the release tarball?

jtojnar · 2018-08-09T17:50:50Z

No idea. But maybe upstream will be wiser.

michaelpj · 2018-08-09T21:54:29Z

I diffed the release tarball and the source checkout - it looks like the tarball has had some generation steps done, and is generally a bit different. I think perhaps we're not meant to build the docs from the release tarball - I think we should probably just switch to building this from git, but that can be in a different PR.

jtojnar · 2018-08-09T23:12:12Z

I do not see why would not devdoc be supposed to be built from tarball. Anyway, opened https://gitlab.freedesktop.org/geoclue/geoclue/issues/77

michaelpj · 2018-08-12T21:33:51Z

I think this is ready to go, any other comments?

jtojnar · 2018-08-12T21:35:25Z

I would still prefer building from git to disabling the docs.

michaelpj · 2018-08-12T22:12:48Z

Updated to build from git following your patch.

michaelpj · 2018-08-12T22:13:01Z

(This will probably be a bit nicer once they're done moving to meson)

jtojnar · 2018-08-12T22:15:14Z

pkgs/development/libraries/geoclue/default.nix

  };

-  outputs = [ "out" "dev" "devdoc" ];
+  outputs = [ "out" "dev" "devdoc"];


The spacing should be consistent.

jtojnar · 2018-08-12T22:15:23Z

pkgs/development/libraries/geoclue/default.nix

+    sed -i '/git submodule/d' autogen.sh
+  '';
+
+  # configure will happen durng the configure phase anyway


jtojnar · 2018-08-12T22:16:08Z

nixos/modules/services/desktops/geoclue2.nix

+    # user who is making the requests
+    systemd.user.services = mkIf config.services.geoclue2.enableDemoAgent { 
+      "geoclue-agent" = {
+        description =  "Geoclue agent";


Extra space

michaelpj · 2018-08-12T22:19:11Z

Nits fixed.

michaelpj · 2018-08-14T09:00:41Z

gentle prod

jtojnar · 2018-08-14T13:41:57Z

2.4.12 was released and it fixes the devdoc. Could you update to it?

jtojnar · 2018-08-14T13:46:53Z

pkgs/development/libraries/geoclue/default.nix

+
+  # configure will happen during the configure phase anyway
+  preConfigure = "NOCONFIGURE=1 ./autogen.sh";
+
  # https://gitlab.freedesktop.org/geoclue/geoclue/issues/73
  postInstall = ''
    sed -i $dev/lib/pkgconfig/libgeoclue-2.0.pc -e "s|includedir=.*|includedir=$dev/include|"


This can be removed in 2.4.12

michaelpj · 2018-08-14T14:33:14Z

Thanks for chasing that up - I've removed the commit changing it to build from git, and changed the version upgrade commit to go to 2.4.12.

jtojnar

Few more final nits. Also could you squash bae4fa2130bba6f1020c31790ab583c31fe9e388 and 38914154ced3e2f5ba4b74901d406dd54befd7f4 into ca8bda4252214ec6d0cc0bf3ab8bd8ed7c4bc9ec:

jtojnar · 2018-08-14T14:35:34Z

pkgs/development/libraries/geoclue/default.nix

@@ -31,7 +32,7 @@ stdenv.mkDerivation rec {
  configureFlags = [
    "--with-systemdsystemunitdir=$(out)/etc/systemd/system"
    "--enable-introspection"
-    "--enable-gtk-doc"
+    "--enable-gtk-doc=yes"


Can you drop this change.

jtojnar · 2018-08-14T14:36:37Z

pkgs/development/libraries/geoclue/default.nix

-  ];
+    pkgconfig intltool wrapGAppsHook gobjectIntrospection
+    # devdoc
+    gtk-doc docbook_xsl docbook_xml_dtd_412 ];


Can you move the closing paren to the next line (like fetchurl does)?

…oid confusing geoclue agent

michaelpj · 2018-08-14T14:59:27Z

Last nits done.

jtojnar · 2018-08-14T15:04:02Z

Actually, services.geoclue2.enableDemoAgent = false should probably also be added to noxlibs module.

michaelpj · 2018-08-14T15:05:38Z

Is it not enough that it's false by default?

jtojnar · 2018-08-14T15:09:36Z

Yeah, that will probably be enough.

jtojnar · 2018-08-14T15:09:58Z

@GrahamcOfBorg build geoclue2

GrahamcOfBorg · 2018-08-14T15:10:52Z

Success on x86_64-linux (full log)

Attempted: geoclue2

Partial log (click to expand)

checking for references to /build in /nix/store/lpgimzzvf0xkn2pancp4pgpr7lgw3xyq-geoclue-2.4.12...
shrinking RPATHs of ELF executables and libraries in /nix/store/hvl2z9j3q7kjnf0z8pl44q36y9vlc0q4-geoclue-2.4.12-dev
strip is /nix/store/gpc2wld1s0c6qzx9326cwn1wcx29xzsj-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/hvl2z9j3q7kjnf0z8pl44q36y9vlc0q4-geoclue-2.4.12-dev/lib
patching script interpreter paths in /nix/store/hvl2z9j3q7kjnf0z8pl44q36y9vlc0q4-geoclue-2.4.12-dev
checking for references to /build in /nix/store/hvl2z9j3q7kjnf0z8pl44q36y9vlc0q4-geoclue-2.4.12-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/8h8dfd5pxwp6avjfn0y9gbj3m472afjj-geoclue-2.4.12-devdoc
strip is /nix/store/gpc2wld1s0c6qzx9326cwn1wcx29xzsj-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/8h8dfd5pxwp6avjfn0y9gbj3m472afjj-geoclue-2.4.12-devdoc
checking for references to /build in /nix/store/8h8dfd5pxwp6avjfn0y9gbj3m472afjj-geoclue-2.4.12-devdoc...

GrahamcOfBorg · 2018-08-14T15:12:29Z

Success on x86_64-darwin (full log)

Attempted: geoclue2

Partial log (click to expand)

Wrapping program /nix/store/r2459i9fdj1l1f91fjhlygrgi0bzdabq-geoclue-2.4.12/libexec/geoclue-2.0/demos/where-am-i
strip is /nix/store/251fvx4mx9q4zarbca5cciasmn66p668-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/r2459i9fdj1l1f91fjhlygrgi0bzdabq-geoclue-2.4.12/lib  /nix/store/r2459i9fdj1l1f91fjhlygrgi0bzdabq-geoclue-2.4.12/libexec
patching script interpreter paths in /nix/store/r2459i9fdj1l1f91fjhlygrgi0bzdabq-geoclue-2.4.12
strip is /nix/store/251fvx4mx9q4zarbca5cciasmn66p668-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/qy0yy4hdiqgl30ffs1z5cia3hgxqn4b5-geoclue-2.4.12-dev/lib
patching script interpreter paths in /nix/store/qy0yy4hdiqgl30ffs1z5cia3hgxqn4b5-geoclue-2.4.12-dev
strip is /nix/store/251fvx4mx9q4zarbca5cciasmn66p668-cctools-binutils-darwin/bin/strip
patching script interpreter paths in /nix/store/gqm8prlc21jfk5pvawzqywpgal582sld-geoclue-2.4.12-devdoc
/nix/store/r2459i9fdj1l1f91fjhlygrgi0bzdabq-geoclue-2.4.12

GrahamcOfBorg · 2018-08-14T15:12:45Z

Success on aarch64-linux (full log)

Attempted: geoclue2

Partial log (click to expand)

shrinking RPATHs of ELF executables and libraries in /nix/store/bl389i8232wd9z242xjjgpnpv2bh3j6j-geoclue-2.4.12-dev
strip is /nix/store/ah0va6j4cnwj9nx4j6rwcfc8nh785jwm-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/bl389i8232wd9z242xjjgpnpv2bh3j6j-geoclue-2.4.12-dev/lib
patching script interpreter paths in /nix/store/bl389i8232wd9z242xjjgpnpv2bh3j6j-geoclue-2.4.12-dev
checking for references to /build in /nix/store/bl389i8232wd9z242xjjgpnpv2bh3j6j-geoclue-2.4.12-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/vn3khi9sf1i20zbwgwi5gq3v38dgmz6r-geoclue-2.4.12-devdoc
strip is /nix/store/ah0va6j4cnwj9nx4j6rwcfc8nh785jwm-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/vn3khi9sf1i20zbwgwi5gq3v38dgmz6r-geoclue-2.4.12-devdoc
checking for references to /build in /nix/store/vn3khi9sf1i20zbwgwi5gq3v38dgmz6r-geoclue-2.4.12-devdoc...
/nix/store/qa8jd3dm8qci5p4yjgn36ndm4z8yk63p-geoclue-2.4.12

michaelpj mentioned this pull request Aug 9, 2018

Redshift, localtime can't talk to geoclue #44725

Closed

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Aug 9, 2018

michaelpj force-pushed the fix/redshift-geoclue-agents branch from 3a413b2 to 380fbad Compare August 9, 2018 15:15

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 labels Aug 9, 2018

jtojnar reviewed Aug 9, 2018

View reviewed changes

jtojnar requested changes Aug 9, 2018

View reviewed changes

michaelpj force-pushed the fix/redshift-geoclue-agents branch from 380fbad to 96adcce Compare August 9, 2018 16:16

jtojnar reviewed Aug 9, 2018

View reviewed changes

jtojnar requested changes Aug 12, 2018

View reviewed changes

jtojnar reviewed Aug 14, 2018

View reviewed changes

michaelpj force-pushed the fix/redshift-geoclue-agents branch 2 times, most recently from 130f6a3 to 3891415 Compare August 14, 2018 14:30

jtojnar reviewed Aug 14, 2018

View reviewed changes

michaelpj added 4 commits August 14, 2018 15:53

redshift: install desktop files, and use absolute paths in them to av…

a176206

…oid confusing geoclue agent

geoclue: 2.4.10 -> 2.4.12

284fc9b

geoclue2 service: add option to run demo agent

3166693

localtime: simplify module a little

80d4fa7

michaelpj force-pushed the fix/redshift-geoclue-agents branch from 3891415 to 80d4fa7 Compare August 14, 2018 14:59

jtojnar approved these changes Aug 14, 2018

View reviewed changes

jtojnar merged commit fe51bf3 into NixOS:master Aug 14, 2018

michaelpj mentioned this pull request Oct 14, 2018

redshift doesn't work with geoclue location provider #45994

Closed

redshift/geoclue/localtime: progress in fixing agent confusion #44820

redshift/geoclue/localtime: progress in fixing agent confusion #44820

Conversation

michaelpj commented Aug 9, 2018

Motivation for this change

Things done

michaelpj commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Aug 9, 2018

jtojnar commented Aug 9, 2018

michaelpj commented Aug 9, 2018

jtojnar commented Aug 9, 2018

michaelpj commented Aug 9, 2018

jtojnar commented Aug 9, 2018

michaelpj commented Aug 9, 2018

jtojnar commented Aug 9, 2018

michaelpj commented Aug 12, 2018

jtojnar commented Aug 12, 2018

michaelpj commented Aug 12, 2018

michaelpj commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Aug 12, 2018

michaelpj commented Aug 14, 2018

jtojnar commented Aug 14, 2018

Choose a reason for hiding this comment

michaelpj commented Aug 14, 2018

jtojnar left a comment

Choose a reason for hiding this comment

jtojnar Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Aug 14, 2018

jtojnar commented Aug 14, 2018

michaelpj commented Aug 14, 2018

jtojnar commented Aug 14, 2018

jtojnar commented Aug 14, 2018

GrahamcOfBorg commented Aug 14, 2018

GrahamcOfBorg commented Aug 14, 2018

GrahamcOfBorg commented Aug 14, 2018

jtojnar Aug 14, 2018 •

edited