| Message ID | 1369320181-6069-1-git-send-email-arnerro@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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 >
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
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
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
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(-)