[1/1] package/bluez5_utils: needs readline

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

Commit Message

Bernd Kuhls Jan. 13, 2018, 10:45 p.m.
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. | #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. | #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. | #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. | #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. | #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

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