diff mbox series

[1/1] package/bluez5_utils: needs readline

Message ID 20180113224532.2715-1-bernd.kuhls@t-online.de
State Superseded
Headers show
Series [1/1] package/bluez5_utils: needs readline | expand

Commit Message

Bernd Kuhls Jan. 13, 2018, 10:45 p.m. UTC 
Upstream commit
https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e

moved code depending on readline from the client tool to the shared
library, readline is therefore not an optional dependency anymore.

Fixes
http://autobuild.buildroot.net/results/019/0197ee0036e129b736c1dc0e83722236fb656618/

Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
---
 package/bluez5_utils/Config.in       | 1 +
 package/bluez5_utils/bluez5_utils.mk | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Baruch Siach Jan. 14, 2018, 5:32 a.m. UTC | #1 
Hi Bernd,

On Sat, Jan 13, 2018 at 11:45:32PM +0100, Bernd Kuhls wrote:
> Upstream commit
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e
> 
> moved code depending on readline from the client tool to the shared
> library, readline is therefore not an optional dependency anymore.
> 
> Fixes
> http://autobuild.buildroot.net/results/019/0197ee0036e129b736c1dc0e83722236fb656618/
> 
> Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
> ---
>  package/bluez5_utils/Config.in       | 1 +
>  package/bluez5_utils/bluez5_utils.mk | 3 +--
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/bluez5_utils/Config.in b/package/bluez5_utils/Config.in
> index 55831e9b61..bb0a2905c5 100644
> --- a/package/bluez5_utils/Config.in
> +++ b/package/bluez5_utils/Config.in
> @@ -9,6 +9,7 @@ config BR2_PACKAGE_BLUEZ5_UTILS
>  	depends on BR2_TOOLCHAIN_HAS_SYNC_4
>  	select BR2_PACKAGE_DBUS
>  	select BR2_PACKAGE_LIBGLIB2
> +	select BR2_PACKAGE_READLINE

I don't think that this is the correct fix. The bluez README file says:

        --disable-client

                Disable support for the command line client

                By default the command line client is enabled and uses the
                readline library. For specific systems where BlueZ is
                configured by other means, the command line client can be
                disabled and the dependency on readline is removed.

So this does not look like a decision to make readline a mandatory dependency, 
but a bug that needs to be fixed upstream.

>  	help
>  	  bluez utils version 5.x
>  
> diff --git a/package/bluez5_utils/bluez5_utils.mk b/package/bluez5_utils/bluez5_utils.mk
> index 0634ec9f6f..055d44de78 100644
> --- a/package/bluez5_utils/bluez5_utils.mk
> +++ b/package/bluez5_utils/bluez5_utils.mk
> @@ -8,7 +8,7 @@ BLUEZ5_UTILS_VERSION = 5.48
>  BLUEZ5_UTILS_SOURCE = bluez-$(BLUEZ5_UTILS_VERSION).tar.xz
>  BLUEZ5_UTILS_SITE = $(BR2_KERNEL_MIRROR)/linux/bluetooth
>  BLUEZ5_UTILS_INSTALL_STAGING = YES
> -BLUEZ5_UTILS_DEPENDENCIES = dbus libglib2
> +BLUEZ5_UTILS_DEPENDENCIES = dbus libglib2 readline
>  BLUEZ5_UTILS_LICENSE = GPL-2.0+, LGPL-2.1+
>  BLUEZ5_UTILS_LICENSE_FILES = COPYING COPYING.LIB
>  
> @@ -26,7 +26,6 @@ endif
>  
>  ifeq ($(BR2_PACKAGE_BLUEZ5_UTILS_CLIENT),y)
>  BLUEZ5_UTILS_CONF_OPTS += --enable-client
> -BLUEZ5_UTILS_DEPENDENCIES += readline
>  else
>  BLUEZ5_UTILS_CONF_OPTS += --disable-client
>  endif

baruch
Thomas Petazzoni Jan. 14, 2018, 1:53 p.m. UTC | #2 
Hello,

On Sun, 14 Jan 2018 07:32:04 +0200, Baruch Siach wrote:
> Hi Bernd,
> 
> On Sat, Jan 13, 2018 at 11:45:32PM +0100, Bernd Kuhls wrote:
> > Upstream commit
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e
> > 
> > moved code depending on readline from the client tool to the shared
> > library, readline is therefore not an optional dependency anymore.
> > 
> > Fixes
> > http://autobuild.buildroot.net/results/019/0197ee0036e129b736c1dc0e83722236fb656618/
> > 
> > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de>
> > ---
> >  package/bluez5_utils/Config.in       | 1 +
> >  package/bluez5_utils/bluez5_utils.mk | 3 +--
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/package/bluez5_utils/Config.in b/package/bluez5_utils/Config.in
> > index 55831e9b61..bb0a2905c5 100644
> > --- a/package/bluez5_utils/Config.in
> > +++ b/package/bluez5_utils/Config.in
> > @@ -9,6 +9,7 @@ config BR2_PACKAGE_BLUEZ5_UTILS
> >  	depends on BR2_TOOLCHAIN_HAS_SYNC_4
> >  	select BR2_PACKAGE_DBUS
> >  	select BR2_PACKAGE_LIBGLIB2
> > +	select BR2_PACKAGE_READLINE  
> 
> I don't think that this is the correct fix. The bluez README file says:
> 
>         --disable-client
> 
>                 Disable support for the command line client
> 
>                 By default the command line client is enabled and uses the
>                 readline library. For specific systems where BlueZ is
>                 configured by other means, the command line client can be
>                 disabled and the dependency on readline is removed.
> 
> So this does not look like a decision to make readline a mandatory dependency, 
> but a bug that needs to be fixed upstream.

I agree it would be nice to check with upstream. They seem to have
intentionally added shell.c to the shared library, and this file
clearly unconditionally uses readline.

Perhaps it's their README file that is now incorrect.

Bernd, could you check with the upstream developers what they say about
this?

Thanks!

Thomas
Thomas Petazzoni Jan. 17, 2018, 8:43 a.m. UTC | #3 
Bernd,

On Sun, 14 Jan 2018 14:53:24 +0100, Thomas Petazzoni wrote:

> I agree it would be nice to check with upstream. They seem to have
> intentionally added shell.c to the shared library, and this file
> clearly unconditionally uses readline.
> 
> Perhaps it's their README file that is now incorrect.
> 
> Bernd, could you check with the upstream developers what they say about
> this?

Have you had the chance to bring this up to the upstream developers?

Best regards,

Thomas
Bernd Kuhls Jan. 17, 2018, 9:23 p.m. UTC | #4 
Am Sun, 14 Jan 2018 07:32:04 +0200 schrieb Baruch Siach:

> Hi Bernd,
> 
> On Sat, Jan 13, 2018 at 11:45:32PM +0100, Bernd Kuhls wrote:
[...]
>> +	select BR2_PACKAGE_READLINE
> 
> I don't think that this is the correct fix. The bluez README file says:
> 
>         --disable-client
> 
>                 Disable support for the command line client
> 
>                 By default the command line client is enabled and uses
>                 the readline library. For specific systems where BlueZ
>                 is configured by other means, the command line client
>                 can be disabled and the dependency on readline is
>                 removed.
> 
> So this does not look like a decision to make readline a mandatory
> dependency,
> but a bug that needs to be fixed upstream.

Hi Baruch,

this commit https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?
id=70b8b754f8e6f9abe9211c686b279dbef16bf666

"client: Make use of bt_shell.
Use bt_shell instead of readline directly."

removed readline code from client code and replaced it with calls to 
bt_shell, bt_shell itself is not used exclusively by client code anymore, 
see the commit I mentioned in my patch ( https://git.kernel.org/pub/scm/
bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e )
and the many source code files which include "src/shared/shell.h".

Afaics this does look like an outdated README file and not a bug in the 
source code, what do you think? Before writing to the bluez devs I would 
like to reach a consensus about what to ask them ;)

Regards, Bernd
Baruch Siach Jan. 18, 2018, 4:40 a.m. UTC | #5 
Hi Bernd,

On Wed, Jan 17, 2018 at 10:23:11PM +0100, Bernd Kuhls wrote:
> Am Sun, 14 Jan 2018 07:32:04 +0200 schrieb Baruch Siach:
> > On Sat, Jan 13, 2018 at 11:45:32PM +0100, Bernd Kuhls wrote:
> [...]
> >> +	select BR2_PACKAGE_READLINE
> > 
> > I don't think that this is the correct fix. The bluez README file says:
> > 
> >         --disable-client
> > 
> >                 Disable support for the command line client
> > 
> >                 By default the command line client is enabled and uses
> >                 the readline library. For specific systems where BlueZ
> >                 is configured by other means, the command line client
> >                 can be disabled and the dependency on readline is
> >                 removed.
> > 
> > So this does not look like a decision to make readline a mandatory
> > dependency,
> > but a bug that needs to be fixed upstream.
> 
> this commit https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?
> id=70b8b754f8e6f9abe9211c686b279dbef16bf666
> 
> "client: Make use of bt_shell.
> Use bt_shell instead of readline directly."
> 
> removed readline code from client code and replaced it with calls to 
> bt_shell, bt_shell itself is not used exclusively by client code anymore, 
> see the commit I mentioned in my patch ( https://git.kernel.org/pub/scm/
> bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e )
> and the many source code files which include "src/shared/shell.h".
> 
> Afaics this does look like an outdated README file and not a bug in the 
> source code, what do you think? Before writing to the bluez devs I would 
> like to reach a consensus about what to ask them ;)

IMO you can simply ask them whether readline is now a mandatory dependency. If 
it is, then an update patch for the README file would be nice.

baruch
Jörg Krause March 2, 2018, 8:59 p.m. UTC | #6 
Hi Baruch, Bernd,

On Thu, 2018-01-18 at 06:40 +0200, Baruch Siach wrote:
> Hi Bernd,
> 
> On Wed, Jan 17, 2018 at 10:23:11PM +0100, Bernd Kuhls wrote:
> > Am Sun, 14 Jan 2018 07:32:04 +0200 schrieb Baruch Siach:
> > > On Sat, Jan 13, 2018 at 11:45:32PM +0100, Bernd Kuhls wrote:
> > 
> > [...]
> > > > +	select BR2_PACKAGE_READLINE
> > > 
> > > I don't think that this is the correct fix. The bluez README file says:
> > > 
> > >         --disable-client
> > > 
> > >                 Disable support for the command line client
> > > 
> > >                 By default the command line client is enabled and uses
> > >                 the readline library. For specific systems where BlueZ
> > >                 is configured by other means, the command line client
> > >                 can be disabled and the dependency on readline is
> > >                 removed.
> > > 
> > > So this does not look like a decision to make readline a mandatory
> > > dependency,
> > > but a bug that needs to be fixed upstream.
> > 
> > this commit https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?
> > id=70b8b754f8e6f9abe9211c686b279dbef16bf666
> > 
> > "client: Make use of bt_shell.
> > Use bt_shell instead of readline directly."
> > 
> > removed readline code from client code and replaced it with calls to 
> > bt_shell, bt_shell itself is not used exclusively by client code anymore, 
> > see the commit I mentioned in my patch ( https://git.kernel.org/pub/scm/
> > bluetooth/bluez.git/commit/?id=bee3796113196da1b3f56da42fcae4d9bae6695e )
> > and the many source code files which include "src/shared/shell.h".
> > 
> > Afaics this does look like an outdated README file and not a bug in the 
> > source code, what do you think? Before writing to the bluez devs I would 
> > like to reach a consensus about what to ask them ;)
> 
> IMO you can simply ask them whether readline is now a mandatory dependency. If 
> it is, then an update patch for the README file would be nice.
> 

Any news on this issue? Would be great to have this fixed before the
next release.

I had a quick look and I would say that the README is indeed outdated
and readline is necessary for all parts depending on libshared-
{glib,mainloop}, which is true for most (all?) tools and bluetoothd.

Best regards,
Jörg Krause
Thomas Petazzoni April 20, 2018, 10:20 a.m. UTC | #7 
Hello,

On Fri, 02 Mar 2018 21:59:58 +0100, Jörg Krause wrote:

> Any news on this issue? Would be great to have this fixed before the
> next release.

I've sent an e-mail to upstream about this:

  https://marc.info/?l=linux-bluetooth&m=152421846903323&w=2

Let's see if they answer, and what they answer.

Best regards,

Thomas
Thomas Petazzoni April 20, 2018, 2:08 p.m. UTC | #8 
Hello,

On Fri, 20 Apr 2018 12:20:13 +0200, Thomas Petazzoni wrote:

> > Any news on this issue? Would be great to have this fixed before the
> > next release.  
> 
> I've sent an e-mail to upstream about this:
> 
>   https://marc.info/?l=linux-bluetooth&m=152421846903323&w=2
> 
> Let's see if they answer, and what they answer.

They answered, and proposed a patch to fix it, which works. I've
submitted a Buildroot patch to integrate it:

  https://patchwork.ozlabs.org/patch/901944/

And therefore I'll mark the older patch proposed by Bernd as Superseded.

Best regards,

Thomas Petazzoni
diff mbox series

Patch

diff --git a/package/bluez5_utils/Config.in b/package/bluez5_utils/Config.in
index 55831e9b61..bb0a2905c5 100644
--- a/package/bluez5_utils/Config.in
+++ b/package/bluez5_utils/Config.in
@@ -9,6 +9,7 @@  config BR2_PACKAGE_BLUEZ5_UTILS
 	depends on BR2_TOOLCHAIN_HAS_SYNC_4
 	select BR2_PACKAGE_DBUS
 	select BR2_PACKAGE_LIBGLIB2
+	select BR2_PACKAGE_READLINE
 	help
 	  bluez utils version 5.x
 
diff --git a/package/bluez5_utils/bluez5_utils.mk b/package/bluez5_utils/bluez5_utils.mk
index 0634ec9f6f..055d44de78 100644
--- a/package/bluez5_utils/bluez5_utils.mk
+++ b/package/bluez5_utils/bluez5_utils.mk
@@ -8,7 +8,7 @@  BLUEZ5_UTILS_VERSION = 5.48
 BLUEZ5_UTILS_SOURCE = bluez-$(BLUEZ5_UTILS_VERSION).tar.xz
 BLUEZ5_UTILS_SITE = $(BR2_KERNEL_MIRROR)/linux/bluetooth
 BLUEZ5_UTILS_INSTALL_STAGING = YES
-BLUEZ5_UTILS_DEPENDENCIES = dbus libglib2
+BLUEZ5_UTILS_DEPENDENCIES = dbus libglib2 readline
 BLUEZ5_UTILS_LICENSE = GPL-2.0+, LGPL-2.1+
 BLUEZ5_UTILS_LICENSE_FILES = COPYING COPYING.LIB
 
@@ -26,7 +26,6 @@  endif
 
 ifeq ($(BR2_PACKAGE_BLUEZ5_UTILS_CLIENT),y)
 BLUEZ5_UTILS_CONF_OPTS += --enable-client
-BLUEZ5_UTILS_DEPENDENCIES += readline
 else
 BLUEZ5_UTILS_CONF_OPTS += --disable-client
 endif