[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Joshua Henderson]

This fixes a build issue where the qt5location plugins are not built, but are
attempted to be installed.

The qt5location plugins have dependencies that are automatically resolved
causing plugins to conditionaly be built. This change checks for the existance
of the plugins prior to trying to install them.

Fixes:

    http://autobuild.buildroot.net/results/bc1/bc13abf3bb2fe1c991aec2334ee658c9641d1fd5/build-end.log

Cc: Julien Corjon <corjon.j@ecagroup.com>
Cc: Peter Seiderer <ps.report@gmx.net>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
---
 package/qt5/qt5location/qt5location.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/package/qt5/qt5location/qt5location.mk b/package/qt5/qt5location/qt5location.mk
index e9f1e4d..fcd9fc8 100644
--- a/package/qt5/qt5location/qt5location.mk
+++ b/package/qt5/qt5location/qt5location.mk
@@ -48,7 +48,9 @@ endif
 
 define QT5LOCATION_INSTALL_TARGET_POSITION
 	cp -dpf $(STAGING_DIR)/usr/lib/libQt5Positioning.so.* $(TARGET_DIR)/usr/lib
-	cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/
+	if [ -d $(STAGING_DIR)/usr/lib/qt/plugins/position ] ; then \
+		cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/ ; \
+	fi
 endef
 
 define QT5LOCATION_INSTALL_TARGET_CMDS
-- 
2.7.4

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Thomas Petazzoni]

Hello,

On Thu, 20 Jul 2017 16:33:29 -0700, Joshua Henderson wrote:
> This fixes a build issue where the qt5location plugins are not built, but are
> attempted to be installed.
> 
> The qt5location plugins have dependencies that are automatically resolved
> causing plugins to conditionaly be built. This change checks for the existance
> of the plugins prior to trying to install them.

Thanks for the fix. Do you know under which condition the plugins will
be built vs. not built ?

If it's related to one or several dependencies being available, then I
would prefer something like:

ifeq ($(BR2...dependencies needed for plugins),y)
define QT5LOCATION_INSTALL_PLUGINS
	cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/
endef
endif

define QT5LOCATION_INSTALL_TARGET_POSITION
	cp -dpf $(STAGING_DIR)/usr/lib/libQt5Positioning.so.* $(TARGET_DIR)/usr/lib
	$(QT5LOCATION_INSTALL_PLUGINS)
endef

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Joshua Henderson]

Thomas,

On 07/20/2017 11:26 PM, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 20 Jul 2017 16:33:29 -0700, Joshua Henderson wrote:
>> This fixes a build issue where the qt5location plugins are not built, but are
>> attempted to be installed.
>>
>> The qt5location plugins have dependencies that are automatically resolved
>> causing plugins to conditionaly be built. This change checks for the existance
>> of the plugins prior to trying to install them.
> 
> Thanks for the fix. Do you know under which condition the plugins will
> be built vs. not built ?
> 

See my reply to Peter on this issue and v2 of this patch.  This is not the right fix.
It appears that qtlocation improperly has a dependency on the quick module in order to
establish build dependencies for the plugin in question.  It is probably better to hit
this issue head on instead of just making buildroot install whatever is built.

> If it's related to one or several dependencies being available, then I
> would prefer something like:
> 
> ifeq ($(BR2...dependencies needed for plugins),y)
> define QT5LOCATION_INSTALL_PLUGINS
> 	cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/
> endef
> endif
> 
> define QT5LOCATION_INSTALL_TARGET_POSITION
> 	cp -dpf $(STAGING_DIR)/usr/lib/libQt5Positioning.so.* $(TARGET_DIR)/usr/lib
> 	$(QT5LOCATION_INSTALL_PLUGINS)
> endef
> 

Good to know.  I actually looked into doing this, but then I started questioning having
dependency tracking of this sort in both buildroot and the application itself.

Food for thought on this topic of copying installed files from staging to target.  Why
not abstract it out and simply provide a list of files and directories to a single
package variable that would handle this automatically?  It won't work for every case,
but it seems like it would work most of the time and be a bit simpler to create and
clearer to read what is being installed.

Josh

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Peter Seiderer]

Hello Joshua,

> Gesendet: Freitag, 21. Juli 2017 um 01:33 Uhr
> Von: "Joshua Henderson" <joshua.henderson@microchip.com>
> An: buildroot at buildroot.org
> Cc: "Peter Seiderer" <ps.report@gmx.net>, "Julien Corjon" <corjon.j@ecagroup.com>, "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com>
> Betreff: [PATCH] package/qt5location: handle plugins being conditionally built
>
> This fixes a build issue where the qt5location plugins are not built, but are
> attempted to be installed.
> 
> The qt5location plugins have dependencies that are automatically resolved
> causing plugins to conditionaly be built. This change checks for the existance
> of the plugins prior to trying to install them.
> 

No, the dependencies of the position plugins are only on module positioning (one purpose of the qt5location
package):

    $  cat src/plugins/plugins.pro 
TEMPLATE = subdirs
qtHaveModule(positioning): SUBDIRS +=  position
qtHaveModule(location): SUBDIRS += geoservices


The problem is a build-ordering, the right fix is to enable ordering in src/src.pro:

diff --git a/src/src.pro b/src/src.pro
index d0a1ee4..d45ddb8 100644
--- a/src/src.pro
+++ b/src/src.pro
@@ -1,5 +1,6 @@
 TEMPLATE = subdirs
-
+CONFIG += ordered
+ 
 SUBDIRS += 3rdparty/clip2tri 3rdparty/clipper 3rdparty/poly2tri
 3rdparty/clip2tri.depends = 3rdparty/clipper 3rdparty/poly2tri

Buildroot patch follows...

Regards,
Peter

> Fixes:
> 
>     http://autobuild.buildroot.net/results/bc1/bc13abf3bb2fe1c991aec2334ee658c9641d1fd5/build-end.log
> 
> Cc: Julien Corjon <corjon.j@ecagroup.com>
> Cc: Peter Seiderer <ps.report@gmx.net>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
> ---
>  package/qt5/qt5location/qt5location.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/package/qt5/qt5location/qt5location.mk b/package/qt5/qt5location/qt5location.mk
> index e9f1e4d..fcd9fc8 100644
> --- a/package/qt5/qt5location/qt5location.mk
> +++ b/package/qt5/qt5location/qt5location.mk
> @@ -48,7 +48,9 @@ endif
>  
>  define QT5LOCATION_INSTALL_TARGET_POSITION
>  	cp -dpf $(STAGING_DIR)/usr/lib/libQt5Positioning.so.* $(TARGET_DIR)/usr/lib
> -	cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/
> +	if [ -d $(STAGING_DIR)/usr/lib/qt/plugins/position ] ; then \
> +		cp -dpfr $(STAGING_DIR)/usr/lib/qt/plugins/position $(TARGET_DIR)/usr/lib/qt/plugins/ ; \
> +	fi
>  endef
>  
>  define QT5LOCATION_INSTALL_TARGET_CMDS
> -- 
> 2.7.4
> 
> 

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Joshua Henderson]

Peter, Thomas,

On 07/20/2017 11:56 PM, Peter Seiderer wrote:
> Hello Joshua,
> 
>> Gesendet: Freitag, 21. Juli 2017 um 01:33 Uhr
>> Von: "Joshua Henderson" <joshua.henderson@microchip.com>
>> An: buildroot at buildroot.org
>> Cc: "Peter Seiderer" <ps.report@gmx.net>, "Julien Corjon" <corjon.j@ecagroup.com>, "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com>
>> Betreff: [PATCH] package/qt5location: handle plugins being conditionally built
>>
>> This fixes a build issue where the qt5location plugins are not built, but are
>> attempted to be installed.
>>
>> The qt5location plugins have dependencies that are automatically resolved
>> causing plugins to conditionaly be built. This change checks for the existance
>> of the plugins prior to trying to install them.
>>
> 
> No, the dependencies of the position plugins are only on module positioning (one purpose of the qt5location
> package):

Ahhh.  You are correct and that statement means I should see some dependency on position for
positioning. Something like this covers it:

    plugins.depends += positioning

And I do see that.  The problem is that the above line only happens in src/src.pro if we have the 
quick module with qtHaveModule(quick).  This is a qt5location change [1] that happened between qt
5.8.0 and qt 5.9.1, which moved the above line as a dependency on quick,  which is why this issue
did not show up in 5.8.0.

This is also why I see the root problem come and go only with a different buildroot config
(availability of quick or not).  I was led down this erroneous path when I took the src.pro for
its word and assumed a dependency on quick in v1 of this patch.

[1] https://code.qt.io/cgit/qt/qtlocation.git/commit/?id=c54ee74acdb9757989004005baf79e99be4c9417

> 
>     $  cat src/plugins/plugins.pro 
> TEMPLATE = subdirs
> qtHaveModule(positioning): SUBDIRS +=  position
> qtHaveModule(location): SUBDIRS += geoservices
> 
> 
> The problem is a build-ordering, the right fix is to enable ordering in src/src.pro:
> 
> diff --git a/src/src.pro b/src/src.pro
> index d0a1ee4..d45ddb8 100644
> --- a/src/src.pro
> +++ b/src/src.pro
> @@ -1,5 +1,6 @@
>  TEMPLATE = subdirs
> -
> +CONFIG += ordered
> + 
>  SUBDIRS += 3rdparty/clip2tri 3rdparty/clipper 3rdparty/poly2tri
>  3rdparty/clip2tri.depends = 3rdparty/clipper 3rdparty/poly2tri
> 
> Buildroot patch follows...

Isn't there some saying about every time "ordered" is used a kitten dies?  Wouldn't the proper fix be to
address the dependency instead? I did not see your patch.  So, I have submitted a v2 to this patch and
also submitted the patch to the Qt bug tracker going about fixing the dependency instead.

Josh

[Buildroot] [PATCH] package/qt5location: handle plugins being conditionally built

[Peter Seiderer]

Hello Joshua,

On Fri, 21 Jul 2017 13:44:56 -0700, Joshua Henderson <joshua.henderson@microchip.com> wrote:

> Peter, Thomas,
> 
> On 07/20/2017 11:56 PM, Peter Seiderer wrote:
> > Hello Joshua,
> > 
> >> Gesendet: Freitag, 21. Juli 2017 um 01:33 Uhr
> >> Von: "Joshua Henderson" <joshua.henderson@microchip.com>
> >> An: buildroot at buildroot.org
> >> Cc: "Peter Seiderer" <ps.report@gmx.net>, "Julien Corjon" <corjon.j@ecagroup.com>, "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com>
> >> Betreff: [PATCH] package/qt5location: handle plugins being conditionally built
> >>
> >> This fixes a build issue where the qt5location plugins are not built, but are
> >> attempted to be installed.
> >>
> >> The qt5location plugins have dependencies that are automatically resolved
> >> causing plugins to conditionaly be built. This change checks for the existance
> >> of the plugins prior to trying to install them.
> >>
> > 
> > No, the dependencies of the position plugins are only on module positioning (one purpose of the qt5location
> > package):
> 
> Ahhh.  You are correct and that statement means I should see some dependency on position for
> positioning. Something like this covers it:
> 
>     plugins.depends += positioning
> 
> And I do see that.  The problem is that the above line only happens in src/src.pro if we have the 
> quick module with qtHaveModule(quick).  This is a qt5location change [1] that happened between qt
> 5.8.0 and qt 5.9.1, which moved the above line as a dependency on quick,  which is why this issue
> did not show up in 5.8.0.
> 
> This is also why I see the root problem come and go only with a different buildroot config
> (availability of quick or not).  I was led down this erroneous path when I took the src.pro for
> its word and assumed a dependency on quick in v1 of this patch.
> 
> [1] https://code.qt.io/cgit/qt/qtlocation.git/commit/?id=c54ee74acdb9757989004005baf79e99be4c9417
> 
> > 
> >     $  cat src/plugins/plugins.pro 
> > TEMPLATE = subdirs
> > qtHaveModule(positioning): SUBDIRS +=  position
> > qtHaveModule(location): SUBDIRS += geoservices
> > 
> > 
> > The problem is a build-ordering, the right fix is to enable ordering in src/src.pro:
> > 
> > diff --git a/src/src.pro b/src/src.pro
> > index d0a1ee4..d45ddb8 100644
> > --- a/src/src.pro
> > +++ b/src/src.pro
> > @@ -1,5 +1,6 @@
> >  TEMPLATE = subdirs
> > -
> > +CONFIG += ordered
> > + 
> >  SUBDIRS += 3rdparty/clip2tri 3rdparty/clipper 3rdparty/poly2tri
> >  3rdparty/clip2tri.depends = 3rdparty/clipper 3rdparty/poly2tri
> > 
> > Buildroot patch follows...
> 
> Isn't there some saying about every time "ordered" is used a kitten dies?  Wouldn't the proper fix be to
> address the dependency instead? I did not see your patch.  So, I have submitted a v2 to this patch and
> also submitted the patch to the Qt bug tracker going about fixing the dependency instead.

Did not yet send the patch (lack of time for a qt bug report (thanks to you for doing it) and
proper gerrit patch submission)...

Fixing it by a dependency is a nice solution (if it works, see review of your v2 patch),
'ordered' is used by some other qt submodules...

Regards,
Peter

> 
> Josh