[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Matt Weber]

The ncurses package installs a full version of clear and reset(tset)
tools.  Preserve these by disabling the options in the busybox config
file.  This removes the need for ncurses to depend on busybox for solely
ordering of target install.

This commit resolves the following python circular dependency with python.
busybox -> libselinux -> python3 -> ncurses -> busybox

Fixes:
http://autobuild.buildroot.net/results/db1/db1e6f3054092fc5576ccab8e04a3b9d74ca9a8c/

Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
---
 package/busybox/busybox.mk | 12 ++++++++++++
 package/ncurses/ncurses.mk |  5 -----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index f9dacc7..b159b44 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -148,6 +148,17 @@ define BUSYBOX_MUSL_TWEAKS
 endef
 endif
 
+ifeq ($(BR2_PACKAGE_NCURSES_TARGET_PROGS),y)
+# Ncurses package overlaps:
+#     /usr/bin/clear
+#     /usr/bin/reset -> /usr/bin/tset (symlink)
+#
+define BUSYBOX_NCURSE_PROGS_TWEAKS
+	$(call KCONFIG_DISABLE_OPT,CONFIG_CLEAR,$(BUSYBOX_BUILD_CONFIG))
+	$(call KCONFIG_DISABLE_OPT,CONFIG_RESET,$(BUSYBOX_BUILD_CONFIG))
+endef
+endif
+
 define BUSYBOX_INSTALL_UDHCPC_SCRIPT
 	if grep -q CONFIG_UDHCPC=y $(@D)/.config; then \
 		$(INSTALL) -m 0755 -D package/busybox/udhcpc.script \
@@ -229,6 +240,7 @@ define BUSYBOX_KCONFIG_FIXUP_CMDS
 	$(BUSYBOX_SET_WATCHDOG)
 	$(BUSYBOX_SET_SELINUX)
 	$(BUSYBOX_MUSL_TWEAKS)
+	$(BUSYBOX_NCURSE_PROGS_TWEAKS)
 endef
 
 define BUSYBOX_CONFIGURE_CMDS
diff --git a/package/ncurses/ncurses.mk b/package/ncurses/ncurses.mk
index 68c77ac..2b99d3f 100644
--- a/package/ncurses/ncurses.mk
+++ b/package/ncurses/ncurses.mk
@@ -29,11 +29,6 @@ NCURSES_CONF_OPTS = \
 	$(if $(BR2_PACKAGE_NCURSES_TARGET_PROGS),,--without-progs) \
 	--without-manpages
 
-# Install after busybox for the full-blown versions
-ifeq ($(BR2_PACKAGE_BUSYBOX),y)
-NCURSES_DEPENDENCIES += busybox
-endif
-
 ifeq ($(BR2_STATIC_LIBS),y)
 NCURSES_CONF_OPTS += --without-shared --with-normal
 else ifeq ($(BR2_SHARED_LIBS),y)
-- 
1.9.1

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Thomas Petazzoni]

Hello,

On Thu, 13 Apr 2017 22:20:47 -0500, Matt Weber wrote:

> +define BUSYBOX_NCURSE_PROGS_TWEAKS

I've changed the name of this variable to:

	BUSYBOX_DISABLE_NCURSES_PROGS

and applied. Thanks a lot!

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

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Thu, 13 Apr 2017 22:20:47 -0500, Matt Weber wrote:

 >> +define BUSYBOX_NCURSE_PROGS_TWEAKS

 > I've changed the name of this variable to:

 > 	BUSYBOX_DISABLE_NCURSES_PROGS

 > and applied. Thanks a lot!

Committed to 2017.02.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Arnout Vandecappelle]

 This is the correct approach I think. I think we could also do this for all the
other cases where we have a -> busybox dependency just because they install the
same apps.

On 14-04-17 05:20, Matt Weber wrote:
> The ncurses package installs a full version of clear and reset(tset)
> tools.  Preserve these by disabling the options in the busybox config
> file.  This removes the need for ncurses to depend on busybox for solely
> ordering of target install.
> 
> This commit resolves the following python circular dependency with python.

 "python circular dependency with python" is a bit circular :-) You can actually
enter the circle from any of these packages. So just "This commit resolves the
following circular dependency".

 It would be nice to add a defconfig fragment to show how to trigger the
circular dependency.

> busybox -> libselinux -> python3 -> ncurses -> busybox
> 
> Fixes:
> http://autobuild.buildroot.net/results/db1/db1e6f3054092fc5576ccab8e04a3b9d74ca9a8c/
> 
> Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>

 Looks good to me, except for the variable naming...

> ---
>  package/busybox/busybox.mk | 12 ++++++++++++
>  package/ncurses/ncurses.mk |  5 -----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> index f9dacc7..b159b44 100644
> --- a/package/busybox/busybox.mk
> +++ b/package/busybox/busybox.mk
> @@ -148,6 +148,17 @@ define BUSYBOX_MUSL_TWEAKS
>  endef
>  endif
>  
> +ifeq ($(BR2_PACKAGE_NCURSES_TARGET_PROGS),y)
> +# Ncurses package overlaps:
> +#     /usr/bin/clear
> +#     /usr/bin/reset -> /usr/bin/tset (symlink)
> +#

 I think the #-line is redundant.

> +define BUSYBOX_NCURSE_PROGS_TWEAKS
                        ^S

 But perhaps a better name is BUSYBOX_UNSET_NCURSES_PROGS

> +	$(call KCONFIG_DISABLE_OPT,CONFIG_CLEAR,$(BUSYBOX_BUILD_CONFIG))
> +	$(call KCONFIG_DISABLE_OPT,CONFIG_RESET,$(BUSYBOX_BUILD_CONFIG))
> +endef
> +endif
> +
>  define BUSYBOX_INSTALL_UDHCPC_SCRIPT
>  	if grep -q CONFIG_UDHCPC=y $(@D)/.config; then \
>  		$(INSTALL) -m 0755 -D package/busybox/udhcpc.script \
> @@ -229,6 +240,7 @@ define BUSYBOX_KCONFIG_FIXUP_CMDS
>  	$(BUSYBOX_SET_WATCHDOG)
>  	$(BUSYBOX_SET_SELINUX)
>  	$(BUSYBOX_MUSL_TWEAKS)
> +	$(BUSYBOX_NCURSE_PROGS_TWEAKS)
                        ^S


 Regards,
 Arnout

>  endef
>  
>  define BUSYBOX_CONFIGURE_CMDS
> diff --git a/package/ncurses/ncurses.mk b/package/ncurses/ncurses.mk
> index 68c77ac..2b99d3f 100644
> --- a/package/ncurses/ncurses.mk
> +++ b/package/ncurses/ncurses.mk
> @@ -29,11 +29,6 @@ NCURSES_CONF_OPTS = \
>  	$(if $(BR2_PACKAGE_NCURSES_TARGET_PROGS),,--without-progs) \
>  	--without-manpages
>  
> -# Install after busybox for the full-blown versions
> -ifeq ($(BR2_PACKAGE_BUSYBOX),y)
> -NCURSES_DEPENDENCIES += busybox
> -endif
> -
>  ifeq ($(BR2_STATIC_LIBS),y)
>  NCURSES_CONF_OPTS += --without-shared --with-normal
>  else ifeq ($(BR2_SHARED_LIBS),y)
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Arnout Vandecappelle]

On 14-04-17 19:00, Arnout Vandecappelle wrote:
>  This is the correct approach I think. I think we could also do this for all the
> other cases where we have a -> busybox dependency just because they install the
> same apps.

 Sorry, I was writing this on the train and hadn't seen Thomas's "Applied,
thanks" mail.

 Please ignore.

 Regards,
 Arnout
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Thomas Petazzoni]

Hello,

On Fri, 14 Apr 2017 19:00:32 +0200, Arnout Vandecappelle wrote:

>  This is the correct approach I think. I think we could also do this for all the
> other cases where we have a -> busybox dependency just because they install the
> same apps.

Yes, I'm also wondering if it isn't better to disable the Busybox
applets (like Matt does in this patch for ncurses) rather then
overwriting the symlinks.

First of all, overwriting the symlinks means that if you "make
busybox-reinstall", you no longer get the right thing (for sure, we
never guarantee that partial builds/rebuilds give the correct output,
but it's nice when they do). And then of course, it means that there is
code in Busybox, consuming some space, that is never used.

The only annoyance that I can see is a potential blotification of
busybox.mk, with lots of conditional for various packages.

Best regards,

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

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Danomi Manchego]

Thomas,

On Thu, Apr 20, 2017 at 3:20 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Fri, 14 Apr 2017 19:00:32 +0200, Arnout Vandecappelle wrote:
>
>>  This is the correct approach I think. I think we could also do this for all the
>> other cases where we have a -> busybox dependency just because they install the
>> same apps.
>
> Yes, I'm also wondering if it isn't better to disable the Busybox
> applets (like Matt does in this patch for ncurses) rather then
> overwriting the symlinks.
>
> First of all, overwriting the symlinks means that if you "make
> busybox-reinstall", you no longer get the right thing (for sure, we
> never guarantee that partial builds/rebuilds give the correct output,
> but it's nice when they do).

Actually, I think this situation is prevented by "busybox: enable
noclobber option in install.sh",
https://git.buildroot.net/buildroot/commit/?id=802bff9c42376078f28aa49d3eab630120940eb0

But of course, the bloat factor is there regardless.

Danomi -

> And then of course, it means that there is
> code in Busybox, consuming some space, that is never used.
>
> The only annoyance that I can see is a potential blotification of
> busybox.mk, with lots of conditional for various packages.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Fri, 14 Apr 2017 19:00:32 +0200, Arnout Vandecappelle wrote:

 >> This is the correct approach I think. I think we could also do this for all the
 >> other cases where we have a -> busybox dependency just because they install the
 >> same apps.

 > Yes, I'm also wondering if it isn't better to disable the Busybox
 > applets (like Matt does in this patch for ncurses) rather then
 > overwriting the symlinks.

 > First of all, overwriting the symlinks means that if you "make
 > busybox-reinstall", you no longer get the right thing (for sure, we
 > never guarantee that partial builds/rebuilds give the correct output,
 > but it's nice when they do). And then of course, it means that there is
 > code in Busybox, consuming some space, that is never used.

 > The only annoyance that I can see is a potential blotification of
 > busybox.mk, with lots of conditional for various packages.

That and the fact that we're adding more "magic" to silently override
the configuration behind the users' back. I get it that people may be
less likely to tweak their busybox config than E.G. the kernel, but just
like for the kernel config I prefer to keep the config overrides to the
minimum.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Arnout Vandecappelle]

On 23-04-17 20:03, Peter Korsgaard wrote:
>>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:
> 
>  > Hello,
>  > On Fri, 14 Apr 2017 19:00:32 +0200, Arnout Vandecappelle wrote:
> 
>  >> This is the correct approach I think. I think we could also do this for all the
>  >> other cases where we have a -> busybox dependency just because they install the
>  >> same apps.
> 
>  > Yes, I'm also wondering if it isn't better to disable the Busybox
>  > applets (like Matt does in this patch for ncurses) rather then
>  > overwriting the symlinks.
> 
>  > First of all, overwriting the symlinks means that if you "make
>  > busybox-reinstall", you no longer get the right thing (for sure, we
>  > never guarantee that partial builds/rebuilds give the correct output,
>  > but it's nice when they do). And then of course, it means that there is
>  > code in Busybox, consuming some space, that is never used.
> 
>  > The only annoyance that I can see is a potential blotification of
>  > busybox.mk, with lots of conditional for various packages.
> 
> That and the fact that we're adding more "magic" to silently override
> the configuration behind the users' back. I get it that people may be
> less likely to tweak their busybox config than E.G. the kernel, but just
> like for the kernel config I prefer to keep the config overrides to the
> minimum.

 Well, we are "overriding" the user's busybox config, but since the alternative
is deleting the busybox-installed tool, whatever the user has configured is not
available anyway...

 Well, except that it still is available by calling it as "busybox clear".

 So, the choice comes down to:

1. Add a dependency on busybox like we do now, but it's actually a fake
dependency and can lead to circular dependencies. And it's anyway not failsafe
because a "busybox-rebuild" will again overwrite it. On addition, it has the
bloatification that Thomas mentions.

2. Make sure that busybox doesn't overwrite a tool that already exists. Needs to
be done for all the different ways that busybox can install things. Then the
order doesn't matter. Great solution, except for the bloatification. Also, it is
not great when you have FEATURE_INDIVIDUAL selected in busybox, because a
busybox-rebuild will not actually rebuild anything.

3. Like proposed in this patch, adapt busybox config. Has the advantage of no
bloatification, but it means that the user sees his own config overridden which
isn't nice. In addition, it's quite ugly for the busybox.mk to have all these
references to other packages (about 40!).


 Of course, we can also choose to apply solution 3 only to the situation where 1
doesn't work because of circular dependencies.


 I can't say which solution is the best. I therefore tend to say: stick to what
we have, and find a workaround if needed. In other words, apply this patch, but
don't do it for other packages until required.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Danomi Manchego]

Arnout,

On Sun, Apr 23, 2017 at 4:55 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> 2. Make sure that busybox doesn't overwrite a tool that already exists. Needs to
> be done for all the different ways that busybox can install things. Then the
> order doesn't matter. Great solution, except for the bloatification. Also, it is
> not great when you have FEATURE_INDIVIDUAL selected in busybox, because a
> busybox-rebuild will not actually rebuild anything.

Does https://git.buildroot.net/buildroot/commit/?id=802bff9c42376078f28aa49d3eab630120940eb0
not take care of overwriting tools that already exist?

Danomi -

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Arnout Vandecappelle]

On 24-04-17 17:36, Danomi Manchego wrote:
> Arnout,
> 
> On Sun, Apr 23, 2017 at 4:55 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> 2. Make sure that busybox doesn't overwrite a tool that already exists. Needs to
>> be done for all the different ways that busybox can install things. Then the
>> order doesn't matter. Great solution, except for the bloatification. Also, it is
>> not great when you have FEATURE_INDIVIDUAL selected in busybox, because a
>> busybox-rebuild will not actually rebuild anything.
> 
> Does https://git.buildroot.net/buildroot/commit/?id=802bff9c42376078f28aa49d3eab630120940eb0
> not take care of overwriting tools that already exist?

 Hey, I missed that one! So, that means that the dependencies on busybox are in
fact redundant, right? So we could actually revert the busybox.mk part of
33c72344a? Yep, tried that, I'll send a patch.

 So in fact, we can remove all dependencies on busybox? That's nice!

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  Of course, we can also choose to apply solution 3 only to the situation where 1
 > doesn't work because of circular dependencies.

 >  I can't say which solution is the best. I therefore tend to say: stick to what
 > we have, and find a workaround if needed. In other words, apply this patch, but
 > don't do it for other packages until required.

I'm in the same boat - So agreed!

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] busybox: preserve ncurses progs/tools

[Arnout Vandecappelle]

On 24-04-17 21:29, Peter Korsgaard wrote:
>>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
> 
> Hi,
> 
>  >  Of course, we can also choose to apply solution 3 only to the situation where 1
>  > doesn't work because of circular dependencies.
> 
>  >  I can't say which solution is the best. I therefore tend to say: stick to what
>  > we have, and find a workaround if needed. In other words, apply this patch, but
>  > don't do it for other packages until required.
> 
> I'm in the same boat - So agreed!

 Well, as Danomi indicated, the current situation is in fact choice 2 (busybox
doesn't overwrite the apps so the order doesn't matter) and not choice 1. So in
fact we can simply remove all the dependencies on busybox.

 Anyone feel up to it?

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF