| Message ID | 1438855575-4767-1-git-send-email-corjon.j@ecagroup.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi Julien, On Thu, Aug 06, 2015 at 12:06:14PM +0200, Julien Corjon wrote: > +ifeq ($(BR2_PACKAGE_GSTREAMER),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 0.10 > +QT5BASE_DEPENDENCIES += gstreamer > +else ifeq ($(BR2_PACKAGE_GSTREAMER1),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 1.0 > +QT5BASE_DEPENDENCIES += gstreamer1 > +else > +QT5BASE_CONFIGURE_OPTS += -no-gstreamer > +endif Shouldn't we default to gstreamer 1.0 when both are enabled? baruch
Dear Julien Corjon, On Thu, 6 Aug 2015 12:06:14 +0200, Julien Corjon wrote: > Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ > > Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> > --- > package/qt5/qt5base/qt5base.mk | 10 ++++++++++ > 1 file changed, 10 insertions(+) I'm sorry, but I don't think this is really the source of the problem. I did reproduce this build failure, and even if GStreamer is built before Qt5 (which is the case in the build failure you're linking in the commit log), Qt5 concludes that GStreamer is not available. And this is because for some reason, Qt5 now seems to ignore the PKG_CONFIG environment variable that we pass to it. I've added some debugging messages to the packagesExist() function, and it uses just "pkg-config" (i.e the system pkg-config, not the one built and installed by Buildroot in $(HOST_DIR)/usr/bin). While I think that your patch should be applied (it's good to make optional dependencies explicit), I also think the pkg-config problem should be investigated and fixed. Best regards, Thomas
On 08/08/2015 14:16, Thomas Petazzoni wrote: > Dear Julien Corjon, > > On Thu, 6 Aug 2015 12:06:14 +0200, Julien Corjon wrote: >> Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ >> >> Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> >> --- >> package/qt5/qt5base/qt5base.mk | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > > I'm sorry, but I don't think this is really the source of the problem. > I did reproduce this build failure, and even if GStreamer is built > before Qt5 (which is the case in the build failure you're linking in > the commit log), Qt5 concludes that GStreamer is not available. > > And this is because for some reason, Qt5 now seems to ignore the > PKG_CONFIG environment variable that we pass to it. I've added some > debugging messages to the packagesExist() function, and it uses just > "pkg-config" (i.e the system pkg-config, not the one built and > installed by Buildroot in $(HOST_DIR)/usr/bin). > > While I think that your patch should be applied (it's good to make > optional dependencies explicit), I also think the pkg-config problem > should be investigated and fixed. > I have just been testing 2015.08 and have immediately hit exactly the same issue. I have done some digging into qt5 and I think I see the issue. The problem is that qmake never looks at environment variables, only things passed on the command line. The patch below fixes the issue for me by explicitly passing PKG_CONFIG on the command line. What is unclear is why I am suddenly seeing the issue in 5.5.0 but not 5.4.1 as that bit of code seems the same in both versions. I'll report the issue upstream and see what they say. Should I spin this into a patch? --- qt5base-5.5.0/config.tests/unix/compile.test.orig 2015-09-15 08:53:45.202320408 +0100 +++ qt5base-5.5.0/config.tests/unix/compile.test 2015-09-15 08:54:02.720770491 +0100 @@ -67,7 +67,7 @@ test -r Makefile && $MAKE distclean >/de # Make sure output from possible previous tests is gone rm -f "$EXE" "${EXE}.exe" -set -- "$OUTDIR/bin/qmake" -nocache -spec "$QMKSPEC" "CONFIG+=$QMAKE_CONFIG" "CONFIG+=android_app" "CONFIG-=debug_and_release app_bundle lib_bundle" "LIBS*=$LFLAGS" "LIBS+=$MAC_ARCH_LFLAGS" "INCLUDEPATH*=$INCLUDEPATH" "QMAKE_CXXFLAGS*=$CXXFLAGS" "QMAKE_CXXFLAGS+=$MAC_ARCH_CXXFLAGS" "$SRCDIR/$TEST/$EXE.pro" -o "$OUTDIR/$TEST/Makefile" +set -- "$OUTDIR/bin/qmake" "PKG_CONFIG=$PKG_CONFIG" -nocache -spec "$QMKSPEC" "CONFIG+=$QMAKE_CONFIG" "CONFIG+=android_app" "CONFIG-=debug_and_release app_bundle lib_bundle" "LIBS*=$LFLAGS" "LIBS+=$MAC_ARCH_LFLAGS" "INCLUDEPATH*=$INCLUDEPATH" "QMAKE_CXXFLAGS*=$CXXFLAGS" "QMAKE_CXXFLAGS+=$MAC_ARCH_CXXFLAGS" "$SRCDIR/$TEST/$EXE.pro" -o "$OUTDIR/$TEST/Makefile" if [ "$VERBOSE" = "yes" ]; then OUTDIR=$OUTDIR "$@" && $MAKE && SUCCESS=yes else Regards Will
Dear Will Wagner, Le 15/09/2015 10:37, Will Wagner a écrit : > On 08/08/2015 14:16, Thomas Petazzoni wrote: >> Dear Julien Corjon, >> >> On Thu, 6 Aug 2015 12:06:14 +0200, Julien Corjon wrote: >>> Fixes: >>> http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ >>> >>> >>> Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> >>> --- >>> package/qt5/qt5base/qt5base.mk | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >> >> I'm sorry, but I don't think this is really the source of the problem. >> I did reproduce this build failure, and even if GStreamer is built >> before Qt5 (which is the case in the build failure you're linking in >> the commit log), Qt5 concludes that GStreamer is not available. >> >> And this is because for some reason, Qt5 now seems to ignore the >> PKG_CONFIG environment variable that we pass to it. I've added some >> debugging messages to the packagesExist() function, and it uses just >> "pkg-config" (i.e the system pkg-config, not the one built and >> installed by Buildroot in $(HOST_DIR)/usr/bin). >> >> While I think that your patch should be applied (it's good to make >> optional dependencies explicit), I also think the pkg-config problem >> should be investigated and fixed. >> > > I have just been testing 2015.08 and have immediately hit exactly the > same issue. I have done some digging into qt5 and I think I see the > issue. The problem is that qmake never looks at environment variables, > only things passed on the command line. The patch below fixes the issue > for me by explicitly passing PKG_CONFIG on the command line. What is > unclear is why I am suddenly seeing the issue in 5.5.0 but not 5.4.1 as > that bit of code seems the same in both versions. I also do not understand why this bug has shows up suddenly... > > I'll report the issue upstream and see what they say. I already report a bug on Qt bug-tracking system[1]. Feel free to reply to it with your investigation > > Should I spin this into a patch? Sure you should spin thin into a patch for Buildroot and for Qt mainline. Regards, Julien [1] https://bugreports.qt.io/browse/QTBUG-48046 > > --- qt5base-5.5.0/config.tests/unix/compile.test.orig 2015-09-15 > 08:53:45.202320408 +0100 > +++ qt5base-5.5.0/config.tests/unix/compile.test 2015-09-15 > 08:54:02.720770491 +0100 > @@ -67,7 +67,7 @@ test -r Makefile && $MAKE distclean >/de > # Make sure output from possible previous tests is gone > rm -f "$EXE" "${EXE}.exe" > > -set -- "$OUTDIR/bin/qmake" -nocache -spec "$QMKSPEC" > "CONFIG+=$QMAKE_CONFIG" "CONFIG+=android_app" "CONFIG-=debug_and_release > app_bundle lib_bundle" "LIBS*=$LFLAGS" "LIBS+=$MAC_ARCH_LFLAGS" > "INCLUDEPATH*=$INCLUDEPATH" "QMAKE_CXXFLAGS*=$CXXFLAGS" > "QMAKE_CXXFLAGS+=$MAC_ARCH_CXXFLAGS" "$SRCDIR/$TEST/$EXE.pro" -o > "$OUTDIR/$TEST/Makefile" > +set -- "$OUTDIR/bin/qmake" "PKG_CONFIG=$PKG_CONFIG" -nocache -spec > "$QMKSPEC" "CONFIG+=$QMAKE_CONFIG" "CONFIG+=android_app" > "CONFIG-=debug_and_release app_bundle lib_bundle" "LIBS*=$LFLAGS" > "LIBS+=$MAC_ARCH_LFLAGS" "INCLUDEPATH*=$INCLUDEPATH" > "QMAKE_CXXFLAGS*=$CXXFLAGS" "QMAKE_CXXFLAGS+=$MAC_ARCH_CXXFLAGS" > "$SRCDIR/$TEST/$EXE.pro" -o "$OUTDIR/$TEST/Makefile" > if [ "$VERBOSE" = "yes" ]; then > OUTDIR=$OUTDIR "$@" && $MAKE && SUCCESS=yes > else > > > Regards > Will > >
Julien, All, On 2015-08-06 12:06 +0200, Julien Corjon spake thusly: > Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ As Thomas said, this does not really fix that issue. However, still following Thomas comment, I think this is a valid patch, and it should be applied with a tweaked commit log; package/qt5base: add support for optional gstreamer qt5base can optionally support gstreamer-0.10 or gstreamer-1.0. Upstreamer still considers gstreamer-0.10 to be preferred over 1.0. Ensure that gstreamer, when enabled, is built before qt5base; tell qt5base to use the proper gstreamer variant, or none if gstreamer is not enabled. Signed-off-by: You With that fixed, you can add my: Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Regards, Yann E. MORIN. > Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> > --- > package/qt5/qt5base/qt5base.mk | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk > index 753b069..f04545b 100644 > --- a/package/qt5/qt5base/qt5base.mk > +++ b/package/qt5/qt5base/qt5base.mk > @@ -152,6 +152,16 @@ QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2) > QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_ICU),-icu,-no-icu) > QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_QT5BASE_ICU),icu) > > +ifeq ($(BR2_PACKAGE_GSTREAMER),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 0.10 > +QT5BASE_DEPENDENCIES += gstreamer > +else ifeq ($(BR2_PACKAGE_GSTREAMER1),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 1.0 > +QT5BASE_DEPENDENCIES += gstreamer1 > +else > +QT5BASE_CONFIGURE_OPTS += -no-gstreamer > +endif > + > QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_EXAMPLES),-make,-nomake) examples > > # Build the list of libraries to be installed on the target > -- > 2.1.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Yann, Peter, All Le 10/01/2016 17:19, Yann E. MORIN a écrit : > Julien, All, > > On 2015-08-06 12:06 +0200, Julien Corjon spake thusly: >> Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ > > As Thomas said, this does not really fix that issue. > > However, still following Thomas comment, I think this is a valid patch, Patch was valid back then. > and it should be applied with a tweaked commit log; > > package/qt5base: add support for optional gstreamer > > qt5base can optionally support gstreamer-0.10 or gstreamer-1.0. > Upstreamer still considers gstreamer-0.10 to be preferred over 1.0. > > Ensure that gstreamer, when enabled, is built before qt5base; tell > qt5base to use the proper gstreamer variant, or none if gstreamer > is not enabled. > > Signed-off-by: You > Peter already patch gstreamer for qt5base and he have removed gstreamer0.1 support[1]. I'm not sure this was the good solution because gstreamer0.1 is still the default choise for Qt[2] but no buildroot user seems to complain about that. > With that fixed, you can add my: > > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Regards, > Yann E. MORIN. Regards, Julien Corjon [1] https://git.busybox.net/buildroot/commit/?id=da9f034d5d1df10ad6c3e249a9a488dc7ef1c42a [2] https://wiki.qt.io/New_Features_in_Qt_5.5
>>>>> "Julien" == Julien CORJON <corjon.j@ecagroup.com> writes: Hi, > Peter already patch gstreamer for qt5base and he have removed > gstreamer0.1 support[1]. I'm not sure this was the good solution because > gstreamer0.1 is still the default choise for Qt[2] but no buildroot user > seems to complain about that. As I explained in the commit message, the reason for disabling the gstreamer 0.10.x support was that it was broken in qt5multimedia. If this is (or gets in the future) fixed, then I have no problem adding it again. With that said, we should use prefer gstreamer 1.x support over 0.10.x if both are enabled.
Julien, All, On 2015-08-06 12:06 +0200, Julien Corjon spake thusly: > Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ Given the feedback in this thread, I'm marking this patch as rejected in Patchwork: - we already have support for gstreamer-1.x - gstreamer-0.10.x seems to be broken. Thanks for spawning the discussion with your patch! :-) Regards, Yann E. MORIN. > Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> > --- > package/qt5/qt5base/qt5base.mk | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk > index 753b069..f04545b 100644 > --- a/package/qt5/qt5base/qt5base.mk > +++ b/package/qt5/qt5base/qt5base.mk > @@ -152,6 +152,16 @@ QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2) > QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_ICU),-icu,-no-icu) > QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_QT5BASE_ICU),icu) > > +ifeq ($(BR2_PACKAGE_GSTREAMER),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 0.10 > +QT5BASE_DEPENDENCIES += gstreamer > +else ifeq ($(BR2_PACKAGE_GSTREAMER1),y) > +QT5BASE_CONFIGURE_OPTS += -gstreamer 1.0 > +QT5BASE_DEPENDENCIES += gstreamer1 > +else > +QT5BASE_CONFIGURE_OPTS += -no-gstreamer > +endif > + > QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_EXAMPLES),-make,-nomake) examples > > # Build the list of libraries to be installed on the target > -- > 2.1.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk index 753b069..f04545b 100644 --- a/package/qt5/qt5base/qt5base.mk +++ b/package/qt5/qt5base/qt5base.mk @@ -152,6 +152,16 @@ QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_LIBGLIB2),libglib2) QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_ICU),-icu,-no-icu) QT5BASE_DEPENDENCIES += $(if $(BR2_PACKAGE_QT5BASE_ICU),icu) +ifeq ($(BR2_PACKAGE_GSTREAMER),y) +QT5BASE_CONFIGURE_OPTS += -gstreamer 0.10 +QT5BASE_DEPENDENCIES += gstreamer +else ifeq ($(BR2_PACKAGE_GSTREAMER1),y) +QT5BASE_CONFIGURE_OPTS += -gstreamer 1.0 +QT5BASE_DEPENDENCIES += gstreamer1 +else +QT5BASE_CONFIGURE_OPTS += -no-gstreamer +endif + QT5BASE_CONFIGURE_OPTS += $(if $(BR2_PACKAGE_QT5BASE_EXAMPLES),-make,-nomake) examples # Build the list of libraries to be installed on the target
Fixes: http://autobuild.buildroot.net/results/0ee0f879e8563954c64b3940cdec39d2e6de937a/ Signed-off-by: Julien Corjon <corjon.j@ecagroup.com> --- package/qt5/qt5base/qt5base.mk | 10 ++++++++++ 1 file changed, 10 insertions(+)