NetworkManager is pretty much Linux-only so having a hard dependency on it kinda sucks. It would be pretty easy to compile the wifi backend only conditionally, so a --disable-wifi might be nice.
Created attachment 92644 [details] [review] Add --disable-wifi ./configure option
So why not get NetworkManager ported? :) I'm also going to add dep on ModemManager for 3G and GPS, would you like an option to disable that too? I really wouldn't want geoclue running anywhere with most of its features disabled. i-e I'd rather keep it linux-specific than to compromise on essential features.
Comment on attachment 92644 [details] [review] Add --disable-wifi ./configure option Review of attachment 92644 [details] [review]: ----------------------------------------------------------------- Too intrusive for my taste. How about a patch that makes wifi source not do anything if NetworkManager isn't available? An initialized source is not required to guarantee location.
After our discussion on IRC I'm not quite sure what you wanted added... did we decide on a generic --disable-network-manager or so? fwiw, gnome-settings-daemon now takes a hard dependency on geoclue so either we need to fix this problem here or there.
(In reply to comment #4) > After our discussion on IRC I'm not quite sure what you wanted added... did > we decide on a generic --disable-network-manager or so? There was no conclusion IIRC. I'm just wondering how you'd get around the hard dep on NetworkManager in shell etc? > fwiw, gnome-settings-daemon now takes a hard dependency on geoclue so either > we need to fix this problem here or there. Well I'd rather we keep geoclue platform specific for keeping it free of #ifdefs. Feel free to fix this in g-s-d instead.
(In reply to comment #5) > (In reply to comment #4) > > After our discussion on IRC I'm not quite sure what you wanted added... did > > we decide on a generic --disable-network-manager or so? > > There was no conclusion IIRC. I'm just wondering how you'd get around the > hard dep on NetworkManager in shell etc? > > > fwiw, gnome-settings-daemon now takes a hard dependency on geoclue so either > > we need to fix this problem here or there. > > Well I'd rather we keep geoclue platform specific Some bits will need to be kept platform specific in any case. Say somebody wants to get it working with conman instead of NM. It would also be bizarre if we needed non-Linux platforms or Linux platforms without NM to recreate the full geoclue D-Bus APIs. > for keeping it free of > #ifdefs. Feel free to fix this in g-s-d instead. I'd expect g-s-d's code to have nothing platform specific. IP-based location uses no platform specific APIs. IMO, the patch doesn't look so bad, but I would certainly make NM a hard dependency on Linux, and disable it on non-Linux. Less moving parts, less of a chance that some vendor will disable NM or MM support on Linux.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > After our discussion on IRC I'm not quite sure what you wanted added... did > > > we decide on a generic --disable-network-manager or so? > > > > There was no conclusion IIRC. I'm just wondering how you'd get around the > > hard dep on NetworkManager in shell etc? > > > > > fwiw, gnome-settings-daemon now takes a hard dependency on geoclue so either > > > we need to fix this problem here or there. > > > > Well I'd rather we keep geoclue platform specific > > Some bits will need to be kept platform specific in any case. Say somebody > wants to get it working with conman instead of NM. It would also be bizarre > if we needed non-Linux platforms or Linux platforms without NM to recreate > the full geoclue D-Bus APIs. > > > for keeping it free of > > #ifdefs. Feel free to fix this in g-s-d instead. > > I'd expect g-s-d's code to have nothing platform specific. IP-based location > uses no platform specific APIs. > > IMO, the patch doesn't look so bad, but I would certainly make NM a hard > dependency on Linux, and disable it on non-Linux. Less moving parts, less of > a chance that some vendor will disable NM or MM support on Linux. Yeah, I realized this after discussing with Ryan last night on IRC and thinking more on this. Now I just await for him to come over tonight and buy me that beer he promised before I push his patch. :P (It needs rebasing though but that should be simple).
Created attachment 93160 [details] [review] Add support for disabling some sources Introduce configure options --disable-wifi-source, --disable-3g-source and --disable-modem-gps-source to allow disabling of backends that depend on NetworkManager and ModemManager (since these components are not widely ported). There is no "auto detect" -- the options must be explicitly specified in order to disable the feature. This prevents a quiet reduction in functionality just because the right headers didn't happen to be installed at configure time, which should help prevent packaging mistakes.
(In reply to comment #8) > Created attachment 93160 [details] [review] [review] > Add support for disabling some sources > > Introduce configure options --disable-wifi-source, --disable-3g-source > and --disable-modem-gps-source to allow disabling of backends that > depend on NetworkManager and ModemManager (since these components are > not widely ported). > > There is no "auto detect" -- the options must be explicitly specified in > order to disable the feature. This prevents a quiet reduction in > functionality just because the right headers didn't happen to be > installed at configure time, which should help prevent packaging > mistakes. Pushed with compiler error fixed and a cosmetic change to configure output.
In the patch for configure.ac: # GPS source AC_ARG_ENABLE(modem-gps-source, AS_HELP_STRING([--disable-gps-source], [Disable GPS backend (requires ModemManager)]), The help string should be --disable-modem-gps-source.
(In reply to comment #10) > In the patch for configure.ac: > > # GPS source > > AC_ARG_ENABLE(modem-gps-source, > > AS_HELP_STRING([--disable-gps-source], [Disable GPS backend > (requires ModemManager)]), > > The help string should be --disable-modem-gps-source. Nice catch! Its a separate issue so it deserved a separate bug. No need to file it now though, just mentioning for future reference. Thanks.
(In reply to comment #10) > In the patch for configure.ac: > > # GPS source > > AC_ARG_ENABLE(modem-gps-source, > > AS_HELP_STRING([--disable-gps-source], [Disable GPS backend > (requires ModemManager)]), > > The help string should be --disable-modem-gps-source. This issue is now fixed in git master btw.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.