Patchwork [1/1] gpsd: Add config options for USB, Bluetooth, Qt.

login
register
mail settings
Submitter Arn R
Date May 23, 2013, 2:43 p.m.
Message ID <1369320181-6069-1-git-send-email-arnerro@gmail.com>
Download mbox | patch
Permalink /patch/245966/
State Changes Requested
Headers show

Comments

Arn R - May 23, 2013, 2:43 p.m.
These options can now be disabled individually.
Previously, these options were simply enabled if the package they depended on was also enabled.
By default they will still be enabled if the packages they depend on are enabled.

Signed-off-by: Arn R <arnerro@gmail.com>
---
 package/gpsd/Config.in | 20 ++++++++++++++++++++
 package/gpsd/gpsd.mk   |  6 +++---
 2 files changed, 23 insertions(+), 3 deletions(-)
Thomas Petazzoni - May 23, 2013, 2:56 p.m.
Dear Arn R,

On Fri, 24 May 2013 00:43:01 +1000, Arn R wrote:
> These options can now be disabled individually.
> Previously, these options were simply enabled if the package they
> depended on was also enabled. By default they will still be enabled
> if the packages they depend on are enabled.

Well, we have a fuzzy boundary on how to handle this, we've discussed
that not long ago with Arnout. Which problem are you seeing with the
current situation (i.e, without your patch) ?

When possible and when it makes we generally try to auto-detect when we
can enable a particular feature, as you've seen in gpsd.mk.

Anyway, some comments below:

> +config BR2_PACKAGE_GPSD_BLUETOOTH_DEV
> +	bool "allow GPS devices via Bluetooth"
> +	default y

This should not be enabled by default.

> +	depends on BR2_PACKAGE_BLUEZ_UTILS

This should be a select.

And ditto for the other options.

I see what you wanted to achieve: something that's automatically
enabled when for example bluez-utils is enabled, so that the existing
behavior is preserved. But for package sub-options, they are generally
disabled by default and they select whichever dependency they need.

Let's wait the opinion of others to see how to handle this.

Best regards,

Thomas
Arn R - May 23, 2013, 3:30 p.m.
Hi Thomas,

On Fri, May 24, 2013 at 12:56 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Well, we have a fuzzy boundary on how to handle this, we've discussed
> that not long ago with Arnout.

Could you point me to the thread where you discussed it?

> Which problem are you seeing with the
> current situation (i.e, without your patch) ?

There is no serious problem, this is more a matter of micro-management :)
For example, I don't use the Qt binding (even though Qt is present in
my build) so I simply do not want it present in the filesystem. My gps
module is connected to a plain UART port (but I use BlueZ for another
bluetooth device), so I have no need for the Bluetooth or USB gps
support. Since GPSd provided explicit options to enable/disable these
very features I thought I may as well provide access to those options.


> When possible and when it makes we generally try to auto-detect when we
> can enable a particular feature, as you've seen in gpsd.mk.
>
> Anyway, some comments below:
>
> > +config BR2_PACKAGE_GPSD_BLUETOOTH_DEV
> > +     bool "allow GPS devices via Bluetooth"
> > +     default y
>
> This should not be enabled by default.
>
> > +     depends on BR2_PACKAGE_BLUEZ_UTILS
>
> This should be a select.
>
> And ditto for the other options.
>
> I see what you wanted to achieve: something that's automatically
> enabled when for example bluez-utils is enabled, so that the existing
> behavior is preserved. But for package sub-options, they are generally
> disabled by default and they select whichever dependency they need.
>
> Let's wait the opinion of others to see how to handle this.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
Thomas Petazzoni - May 23, 2013, 3:46 p.m.
Dear Arn R,

On Fri, 24 May 2013 01:30:14 +1000, Arn R wrote:
> Hi Thomas,
> 
> On Fri, May 24, 2013 at 12:56 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Well, we have a fuzzy boundary on how to handle this, we've
> > discussed that not long ago with Arnout.
> 
> Could you point me to the thread where you discussed it?

See http://lists.busybox.net/pipermail/buildroot/2013-May/072318.html
from Arnout, and my reply at
http://lists.busybox.net/pipermail/buildroot/2013-May/072335.html.

> > Which problem are you seeing with the
> > current situation (i.e, without your patch) ?
> 
> There is no serious problem, this is more a matter of
> micro-management :) For example, I don't use the Qt binding (even
> though Qt is present in my build) so I simply do not want it present
> in the filesystem. My gps module is connected to a plain UART port
> (but I use BlueZ for another bluetooth device), so I have no need for
> the Bluetooth or USB gps support. Since GPSd provided explicit
> options to enable/disable these very features I thought I may as well
> provide access to those options.

I see, thanks for your feedback on this use case.

We have two options here:

 1) Either declare that adding such options would add too many options
    to all the packages, and that such special modifications should be
    left to local modifications.

 2) Or add many more configuration options to packages.

Maybe (2) is what makes sense here. The cost of new options in terms of
maintenance is not so high, so maybe that's what we should do. I don't
know.

Peter, Arnout?

Best regards,

Thomas
Arnout Vandecappelle - May 23, 2013, 7:03 p.m.
On 23/05/13 17:46, Thomas Petazzoni wrote:
> Dear Arn R,
>
> On Fri, 24 May 2013 01:30:14 +1000, Arn R wrote:
>> Hi Thomas,
>>
>> On Fri, May 24, 2013 at 12:56 AM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>>> Well, we have a fuzzy boundary on how to handle this, we've
>>> discussed that not long ago with Arnout.
[snip]
>>> Which problem are you seeing with the
>>> current situation (i.e, without your patch) ?
>>
>> There is no serious problem, this is more a matter of
>> micro-management :) For example, I don't use the Qt binding (even
>> though Qt is present in my build) so I simply do not want it present
>> in the filesystem. My gps module is connected to a plain UART port
>> (but I use BlueZ for another bluetooth device), so I have no need for
>> the Bluetooth or USB gps support. Since GPSd provided explicit
>> options to enable/disable these very features I thought I may as well
>> provide access to those options.

  If enabling the option adds a significant amount of overhead (typically 
rootfs size overhead but it could also be performance overhead), then it 
is definitely worthwhile to make it an option. So for the qt bindings 
it's probably not worthwhile (qt itself is much larger). But for the 
Bluetooth or USB support, it could make a difference.

>
> I see, thanks for your feedback on this use case.
>
> We have two options here:
>
>   1) Either declare that adding such options would add too many options
>      to all the packages, and that such special modifications should be
>      left to local modifications.
>
>   2) Or add many more configuration options to packages.
>
> Maybe (2) is what makes sense here. The cost of new options in terms of
> maintenance is not so high,

  You forgot about updating the 'depends on' statements when a suboption 
wants to select e.g. glib...

> so maybe that's what we should do. I don't know.

  My dream is that the whole Config.in could be generated from the .mk 
file... but I'm not sure if that is really realistic. OpenWRT does it but 
it doesn't look that great either.

  So I think we'll stick to

    3) Decide on a case-by-case basis whether to go for suboptions or 
automatic enable/disable.


  Regards,
  Arnout

> Peter, Arnout?
>
> Best regards,
>
> Thomas
>
Thomas Petazzoni - May 23, 2013, 7:12 p.m.
Dear Arnout Vandecappelle,

On Thu, 23 May 2013 21:03:22 +0200, Arnout Vandecappelle wrote:

>   If enabling the option adds a significant amount of overhead (typically 
> rootfs size overhead but it could also be performance overhead), then it 
> is definitely worthwhile to make it an option. So for the qt bindings 
> it's probably not worthwhile (qt itself is much larger). But for the 
> Bluetooth or USB support, it could make a difference.

Regarding the Qt binding, if it installs additional files for example,
I wouldn't mind having a separate sub-option to disable it.

> > Maybe (2) is what makes sense here. The cost of new options in terms of
> > maintenance is not so high,
> 
>   You forgot about updating the 'depends on' statements when a suboption 
> wants to select e.g. glib...

Hum, not sure to understand what you meant here. Did you mean
"propagate the 'depends on'" from the selected package into the
selecting package, for dependencies on toolchain features and things
like that?

> > so maybe that's what we should do. I don't know.
> 
>   My dream is that the whole Config.in could be generated from the .mk 
> file... but I'm not sure if that is really realistic. OpenWRT does it but 
> it doesn't look that great either.
> 
>   So I think we'll stick to
> 
>     3) Decide on a case-by-case basis whether to go for suboptions or 
> automatic enable/disable.

I agree, as you don't see much other reasonable solutions.

Thomas
Arnout Vandecappelle - May 24, 2013, 5:16 a.m.
On 23/05/13 21:12, Thomas Petazzoni wrote:
>>> > >Maybe (2) is what makes sense here. The cost of new options in terms of
>>> > >maintenance is not so high,
>> >
>> >   You forgot about updating the 'depends on' statements when a suboption
>> >wants to select e.g. glib...
> Hum, not sure to understand what you meant here. Did you mean
> "propagate the 'depends on'" from the selected package into the
> selecting package, for dependencies on toolchain features and things
> like that?

  Yes indeed. And the maintenance effort is to keep that up-to-date when 
heavily used libraries grow new dependencies.

  Regards,
  Arnout

Patch

diff --git a/package/gpsd/Config.in b/package/gpsd/Config.in
index 337f8e7..c38d52b 100644
--- a/package/gpsd/Config.in
+++ b/package/gpsd/Config.in
@@ -90,6 +90,26 @@  config BR2_PACKAGE_GPSD_CONTROLSEND
 config BR2_PACKAGE_GPSD_SQUELCH
 	bool "squelch gpsd_report and gpsd_hexdump to save cpu"
 
+config BR2_PACKAGE_GPSD_BLUETOOTH_DEV
+	bool "allow GPS devices via Bluetooth"
+	default y
+	depends on BR2_PACKAGE_BLUEZ_UTILS
+	help
+	  Support Bluetooth devices with BlueZ.
+
+config BR2_PACKAGE_GPSD_USB_DEV
+	bool "allow GPS devices via USB"
+	default y
+	depends on BR2_PACKAGE_LIBUSB
+	help
+	  Support USB devices with libusb.
+
+config BR2_PACKAGE_GPSD_QTBINDING
+	bool "enable the Qt bindings"
+	default y
+	depends on BR2_PACKAGE_QT_NETWORK
+	help
+	  Builds the library libQgpsmm to provide Qt bindings.
 endmenu
 
 menu "Protocols"
diff --git a/package/gpsd/gpsd.mk b/package/gpsd/gpsd.mk
index 384726d..be4046f 100644
--- a/package/gpsd/gpsd.mk
+++ b/package/gpsd/gpsd.mk
@@ -44,7 +44,7 @@  else
 endif
 
 # Enable or disable Qt binding
-ifeq ($(BR2_PACKAGE_QT_NETWORK),y)
+ifeq ($(BR2_PACKAGE_GPSD_QTBINDING),y)
 	GPSD_SCONS_ENV += QMAKE="$(QT_QMAKE)"
 	GPSD_DEPENDENCIES += qt host-pkgconf
 else
@@ -52,14 +52,14 @@  else
 endif
 
 # If libusb is available build it before so the package can use it
-ifeq ($(BR2_PACKAGE_LIBUSB),y)
+ifeq ($(BR2_PACKAGE_GPSD_USB_DEV),y)
 	GPSD_DEPENDENCIES += libusb
 else
 	GPSD_SCONS_OPTS += usb=no
 endif
 
 # If bluetooth is available build it before so the package can use it
-ifeq ($(BR2_PACKAGE_BLUEZ_UTILS),y)
+ifeq ($(BR2_PACKAGE_GPSD_BLUETOOTH_DEV),y)
 	GPSD_DEPENDENCIES += bluez_utils
 else
 	GPSD_SCONS_OPTS += bluez=no