Bug 73961 - add support for disabling the wifi backend
Summary: add support for disabling the wifi backend
Status: RESOLVED FIXED
Alias: None
Product: GeoClue
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Geoclue Bugs
QA Contact: Geoclue Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-23 07:44 UTC by Allison Lortie (desrt)
Modified: 2014-02-03 17:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Add --disable-wifi ./configure option (3.83 KB, patch)
2014-01-23 08:15 UTC, Allison Lortie (desrt)
Details | Splinter Review
Add support for disabling some sources (8.58 KB, patch)
2014-02-01 10:08 UTC, Allison Lortie (desrt)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Allison Lortie (desrt) 2014-01-23 07:44:12 UTC
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.
Comment 1 Allison Lortie (desrt) 2014-01-23 08:15:24 UTC
Created attachment 92644 [details] [review]
Add --disable-wifi ./configure option
Comment 2 Zeeshan Ali 2014-01-23 12:48:21 UTC
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 3 Zeeshan Ali 2014-01-23 14:35:50 UTC
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.
Comment 4 Allison Lortie (desrt) 2014-01-25 05:19:55 UTC
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.
Comment 5 Zeeshan Ali 2014-01-25 19:54:34 UTC
(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.
Comment 6 Bastien Nocera 2014-01-25 23:23:25 UTC
(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.
Comment 7 Zeeshan Ali 2014-01-26 13:36:44 UTC
(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).
Comment 8 Allison Lortie (desrt) 2014-02-01 10:08:24 UTC
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.
Comment 9 Zeeshan Ali 2014-02-02 14:03:24 UTC
(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.
Comment 10 Ting-Wei Lan 2014-02-02 17:09:26 UTC
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.
Comment 11 Zeeshan Ali 2014-02-03 16:59:53 UTC
(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.
Comment 12 Zeeshan Ali 2014-02-03 17:29:14 UTC
(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.