Patchwork [v4,05/21] qt5base: add glib support

login
register
mail settings
Submitter Thomas Petazzoni
Date March 19, 2013, 7:29 p.m.
Message ID <1363721394-14973-6-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/229198/
State Accepted
Headers show

Comments

Thomas Petazzoni - March 19, 2013, 7:29 p.m.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/qt5/qt5base/qt5base.mk |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Peter Korsgaard - March 19, 2013, 9:14 p.m.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 Thomas> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Committed, thanks.

 Thomas> ---
 Thomas>  package/qt5/qt5base/qt5base.mk |    4 +++-
 Thomas>  1 file changed, 3 insertions(+), 1 deletion(-)

 Thomas> diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk
 Thomas> index 62f0b9a..d5dcb7d 100644
 Thomas> --- a/package/qt5/qt5base/qt5base.mk
 Thomas> +++ b/package/qt5/qt5base/qt5base.mk
 Thomas> @@ -21,7 +21,6 @@ QT5BASE_INSTALL_STAGING = YES
 Thomas>  QT5BASE_CONFIGURE_OPTS += \
 Thomas>  	-optimized-qmake \
 Thomas>  	-no-kms \
 Thomas> -	-no-glib \
 Thomas>  	-no-cups \
 Thomas>  	-no-nis \
 Thomas>  	-no-libudev \
 Thomas> @@ -95,6 +94,9 @@ QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_PNG),libpng)
 Thomas>  QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_DBUS),-dbus,-no-dbus)
 Thomas>  QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_DBUS),dbus)
 
 Thomas> +QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_LIBGLIB2),-glib,-no-glib)
 Thomas> +QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2)

You're adding a mix of:

 - Explicit suboptions which selects the needed libraries
   (fontconfig/jpeg/png/dbus)
 - Automatic enabling/disabling support if package is enabled

Is there any specific reason for this?
Thomas Petazzoni - March 20, 2013, 8:35 a.m.
Dear Peter Korsgaard,

On Tue, 19 Mar 2013 22:14:59 +0100, Peter Korsgaard wrote:

>  Thomas> @@ -95,6 +94,9 @@ QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_PNG),libpng)
>  Thomas>  QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_DBUS),-dbus,-no-dbus)
>  Thomas>  QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_DBUS),dbus)
>  
>  Thomas> +QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_LIBGLIB2),-glib,-no-glib)
>  Thomas> +QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2)
> 
> You're adding a mix of:
> 
>  - Explicit suboptions which selects the needed libraries
>    (fontconfig/jpeg/png/dbus)
>  - Automatic enabling/disabling support if package is enabled
> 
> Is there any specific reason for this?

For the specific cases you're pointing at (glib and dbus), I modeled
things to be similar to what we have for Qt4.

Also it kind of makes sense because adding DBus support creates an
additional Qt5DBus.so library, while the libglib2 support doesn't.

But admittedly, the boundary between automatic enabling/disabling
support and explicit suboptions is a bit fuzzy. It's sometimes a bit
strange to have a suboption for something very small, and automatic
enabling/disabling for something rather "big". I don't really have a
good idea on how to clearly draw the line between the two solutions.
One radical solution is: whenever it can be automatic, it should be.
Don't know if it's reasonable, though. I'm open to suggestions on this,
and we should probably update the documentation with details on this
topic.

Best regards,

Thomas
Arnout Vandecappelle - March 27, 2013, 5:19 p.m.
On 20/03/13 09:35, Thomas Petazzoni wrote:
> But admittedly, the boundary between automatic enabling/disabling
> support and explicit suboptions is a bit fuzzy. It's sometimes a bit
> strange to have a suboption for something very small, and automatic
> enabling/disabling for something rather "big". I don't really have a
> good idea on how to clearly draw the line between the two solutions.
> One radical solution is: whenever it can be automatic, it should be.
> Don't know if it's reasonable, though. I'm open to suggestions on this,
> and we should probably update the documentation with details on this
> topic.

  For me, the default is to have automatic  enable/disable, with the 
following exceptions:

- the "wrapper library" is rather large, so you want to be able to build 
without the wrapper library. I can't actually find an example of this. 
Even the QtDbus example: the QtDbus+QtXml libraries total 650K 
(stripped), which is not that much compared to 3M for QtCore.

- it's not obvious to the user that the dependency should be selected. 
BR2_PACKAGE_DIRECTFB_MULTI is a good example: you wouldn't have the idea 
that you have to select linux-fusion in order to get multi-application 
support on DirectFB.


  That said, I don't think it every hurts to have a Config.in option when 
it is not strictly required. It increases our code size, but there's no 
important maintenance effort (at least if it's a simple bool option). So 
I would say it's mostly up to the contributors.

  Regards,
  Arnout

Patch

diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk
index 62f0b9a..d5dcb7d 100644
--- a/package/qt5/qt5base/qt5base.mk
+++ b/package/qt5/qt5base/qt5base.mk
@@ -21,7 +21,6 @@  QT5BASE_INSTALL_STAGING = YES
 QT5BASE_CONFIGURE_OPTS += \
 	-optimized-qmake \
 	-no-kms \
-	-no-glib \
 	-no-cups \
 	-no-nis \
 	-no-libudev \
@@ -95,6 +94,9 @@  QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_PNG),libpng)
 QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_DBUS),-dbus,-no-dbus)
 QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_QT5BASE_DBUS),dbus)
 
+QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_LIBGLIB2),-glib,-no-glib)
+QT5BASE_DEPENDENCIES   += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2)
+
 # Build the list of libraries to be installed on the target
 QT5BASE_INSTALL_LIBS_y                                 += Qt5Core
 QT5BASE_INSTALL_LIBS_$(BR2_PACKAGE_QT5BASE_NETWORK)    += Qt5Network