[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Thomas Petazzoni]

iputils installs several programs that are also implemented as applets
in Busybox. Two of these (arping and tftpd) are installed by iputils
in /bin, while Busybox installs them in /usr/sbin, causing both to be
present if both iputils and busybox are enabled.

This commit moves the binaries installed by iputils to /usr/sbin, so
that only these are installed (Busybox will be installed later thanks
to its optional dependency on iputils, but it will not override the
tools installed by iputils).

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/iputils/iputils.mk | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
index 8be54b4788..fbf586729b 100644
--- a/package/iputils/iputils.mk
+++ b/package/iputils/iputils.mk
@@ -52,6 +52,16 @@ endif
 # XSL Stylesheets for DocBook 5 not packaged for buildroot
 IPUTILS_CONF_OPTS += -DBUILD_MANS=false -DBUILD_HTML_MANS=false
 
+# move iputils binaries to the same location as where Busybox installs
+# the corresponding applets, so that we have a single version of the
+# tools (from iputils)
+define IPUTILS_MOVE_BINARIES
+	mv $(TARGET_DIR)/usr/bin/arping $(TARGET_DIR)/usr/sbin/arping
+	mv $(TARGET_DIR)/usr/bin/ping $(TARGET_DIR)/bin/ping
+	mv $(TARGET_DIR)/usr/bin/tftpd $(TARGET_DIR)/usr/sbin/tftpd
+endef
+IPUTILS_POST_INSTALL_TARGET_HOOKS += IPUTILS_MOVE_BINARIES
+
 # handle permissions ourselves
 IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
 define IPUTILS_PERMISSIONS
-- 
2.21.0

[Buildroot] [PATCH 2/2] package/iputils: fix IPUTILS_PERMISSIONS

[Thomas Petazzoni]

The IPUTILS_PERMISSIONS variable was wrong, because iputils installs
its programs in /usr/bin, not in /bin. It probably was only tested
with the merged /usr option enabled.

This commit fixes IPUTILS_PERMISSIONS, taking into account the change
done in "package/iputils: move binaries to the location also used by
Busybox":

 - arping is moved by the post-install target hook to /usr/sbin

 - clockdiff is installed by iputils in /usr/bin

 - ping is moved by the post-install target hook to /bin, which makes
   the previously incorrect path in IPUTILS_PERMISSIONS now correct

Fixes:

  http://autobuild.buildroot.net/results/fe378bca29c86a681ba9ad40386cb89248195c50/

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/iputils/iputils.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
index fbf586729b..394c19a943 100644
--- a/package/iputils/iputils.mk
+++ b/package/iputils/iputils.mk
@@ -65,9 +65,9 @@ IPUTILS_POST_INSTALL_TARGET_HOOKS += IPUTILS_MOVE_BINARIES
 # handle permissions ourselves
 IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
 define IPUTILS_PERMISSIONS
-	/bin/arping      f 4755 0 0 - - - - -
-	/bin/clockdiff   f 4755 0 0 - - - - -
-	/bin/ping        f 4755 0 0 - - - - -
+	/usr/sbin/arping      f 4755 0 0 - - - - -
+	/usr/bin/clockdiff    f 4755 0 0 - - - - -
+	/bin/ping             f 4755 0 0 - - - - -
 endef
 
 $(eval $(meson-package))
-- 
2.21.0

[Buildroot] [PATCH 2/2] package/iputils: fix IPUTILS_PERMISSIONS

[Petr Vorel]

Hi Thomas,

> The IPUTILS_PERMISSIONS variable was wrong, because iputils installs
> its programs in /usr/bin, not in /bin. It probably was only tested
> with the merged /usr option enabled.

> This commit fixes IPUTILS_PERMISSIONS, taking into account the change
> done in "package/iputils: move binaries to the location also used by
> Busybox":

>  - arping is moved by the post-install target hook to /usr/sbin

>  - clockdiff is installed by iputils in /usr/bin

>  - ping is moved by the post-install target hook to /bin, which makes
>    the previously incorrect path in IPUTILS_PERMISSIONS now correct

> Fixes:

>   http://autobuild.buildroot.net/results/fe378bca29c86a681ba9ad40386cb89248195c50/

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/iputils/iputils.mk | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

> diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
> index fbf586729b..394c19a943 100644
> --- a/package/iputils/iputils.mk
> +++ b/package/iputils/iputils.mk
> @@ -65,9 +65,9 @@ IPUTILS_POST_INSTALL_TARGET_HOOKS += IPUTILS_MOVE_BINARIES
>  # handle permissions ourselves
>  IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
>  define IPUTILS_PERMISSIONS
> -	/bin/arping      f 4755 0 0 - - - - -
> -	/bin/clockdiff   f 4755 0 0 - - - - -
> -	/bin/ping        f 4755 0 0 - - - - -
> +	/usr/sbin/arping      f 4755 0 0 - - - - -
> +	/usr/bin/clockdiff    f 4755 0 0 - - - - -
> +	/bin/ping             f 4755 0 0 - - - - -

again thanks for the fix and sorry for introducing bug.
Acked-by: Petr Vorel <petr.vorel@gmail.com>

The only thing to consider:
Upstream doesn't care much about systems without usr merge.
I wonder if systems without it (unset BR2_ROOTFS_MERGED_USR),
whether we want to have /bin/ping and symlinked it into /usr/bin/ping.
I see this is handled for coreutils (COREUTILS_BIN_PROGS).


Kind regards,
Petr

[Buildroot] [PATCH 2/2] package/iputils: fix IPUTILS_PERMISSIONS

[Thomas Petazzoni]

Hello,

On Mon, 17 Jun 2019 22:32:36 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:

> The only thing to consider:
> Upstream doesn't care much about systems without usr merge.
> I wonder if systems without it (unset BR2_ROOTFS_MERGED_USR),
> whether we want to have /bin/ping and symlinked it into /usr/bin/ping.
> I see this is handled for coreutils (COREUTILS_BIN_PROGS).

What COREUTILS_CLEANUP_BIN is doing is not to install a symlink
between /bin/foo and /usr/bin/foo. Indeed, such a symlink is not
necessary: both /bin and /usr/bin are in the PATH, so having /bin/foo
and /usr/bin/foo is totally useless.

What COREUTILS_CLEANUP_BIN does is to make sure that in a
non-BR2_ROOTFS_MERGED_USR case, the symlinks to coreutils are installed
in the same location as the corresponding Busybox applets, so that we
properly handle the conflict between Busybox applets and full-blown
variants, as I explained in my reply to your comments on PATCH 1/2.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/2] package/iputils: fix IPUTILS_PERMISSIONS

[Petr Vorel]

Hi Thomas,

> Hello,

> On Mon, 17 Jun 2019 22:32:36 +0200
> Petr Vorel <petr.vorel@gmail.com> wrote:

> > The only thing to consider:
> > Upstream doesn't care much about systems without usr merge.
> > I wonder if systems without it (unset BR2_ROOTFS_MERGED_USR),
> > whether we want to have /bin/ping and symlinked it into /usr/bin/ping.
> > I see this is handled for coreutils (COREUTILS_BIN_PROGS).

> What COREUTILS_CLEANUP_BIN is doing is not to install a symlink
> between /bin/foo and /usr/bin/foo. Indeed, such a symlink is not
> necessary: both /bin and /usr/bin are in the PATH, so having /bin/foo
> and /usr/bin/foo is totally useless.

> What COREUTILS_CLEANUP_BIN does is to make sure that in a
> non-BR2_ROOTFS_MERGED_USR case, the symlinks to coreutils are installed
> in the same location as the corresponding Busybox applets, so that we
> properly handle the conflict between Busybox applets and full-blown
> variants, as I explained in my reply to your comments on PATCH 1/2.

Thanks for an explanation.

Kind regards,
Petr

> Best regards,

> Thomas

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Yann E. MORIN]

Thomas, All,

On 2019-06-16 22:09 +0200, Thomas Petazzoni spake thusly:
> iputils installs several programs that are also implemented as applets
> in Busybox. Two of these (arping and tftpd) are installed by iputils
> in /bin, while Busybox installs them in /usr/sbin, causing both to be
> present if both iputils and busybox are enabled.
> 
> This commit moves the binaries installed by iputils to /usr/sbin, so
> that only these are installed (Busybox will be installed later thanks
> to its optional dependency on iputils, but it will not override the
> tools installed by iputils).
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/iputils/iputils.mk | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
> index 8be54b4788..fbf586729b 100644
> --- a/package/iputils/iputils.mk
> +++ b/package/iputils/iputils.mk
> @@ -52,6 +52,16 @@ endif
>  # XSL Stylesheets for DocBook 5 not packaged for buildroot
>  IPUTILS_CONF_OPTS += -DBUILD_MANS=false -DBUILD_HTML_MANS=false
>  
> +# move iputils binaries to the same location as where Busybox installs
> +# the corresponding applets, so that we have a single version of the
> +# tools (from iputils)
> +define IPUTILS_MOVE_BINARIES
> +	mv $(TARGET_DIR)/usr/bin/arping $(TARGET_DIR)/usr/sbin/arping
> +	mv $(TARGET_DIR)/usr/bin/ping $(TARGET_DIR)/bin/ping

This one will fail with a merged /usr, I guess...

Regards,
Yann E. MORIN.

> +	mv $(TARGET_DIR)/usr/bin/tftpd $(TARGET_DIR)/usr/sbin/tftpd
> +endef
> +IPUTILS_POST_INSTALL_TARGET_HOOKS += IPUTILS_MOVE_BINARIES
> +
>  # handle permissions ourselves
>  IPUTILS_CONF_OPTS += -DNO_SETCAP_OR_SUID=true
>  define IPUTILS_PERMISSIONS
> -- 
> 2.21.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Thomas Petazzoni]

On Sun, 16 Jun 2019 22:20:18 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > +	mv $(TARGET_DIR)/usr/bin/arping $(TARGET_DIR)/usr/sbin/arping
> > +	mv $(TARGET_DIR)/usr/bin/ping $(TARGET_DIR)/bin/ping  
> 
> This one will fail with a merged /usr, I guess...

Indeed, thanks for spotting. I sent a v2 that deals with this. I didn't
find a better than just do the move conditionally, i.e when merged /usr
is disabled.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Petr Vorel]

Hi Thomas,

> iputils installs several programs that are also implemented as applets
> in Busybox. Two of these (arping and tftpd) are installed by iputils
> in /bin, while Busybox installs them in /usr/sbin, causing both to be
     ^
I guess you mean /usr/bin

> present if both iputils and busybox are enabled.

> This commit moves the binaries installed by iputils to /usr/sbin, so
> that only these are installed (Busybox will be installed later thanks
> to its optional dependency on iputils, but it will not override the
> tools installed by iputils).
Thanks for reporting and fixing this bug.
Although I'm not keen on changing binary default location,
this is simple straightforward solution. Alternative would be to disable busybox
config CONFIG_ARPING (with support/kconfig/merge_config.sh), but that'd be too
complicated. Thus ack

Acked-by: Petr Vorel <petr.vorel@gmail.com>

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/iputils/iputils.mk | 10 ++++++++++
>  1 file changed, 10 insertions(+)

> diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk
> index 8be54b4788..fbf586729b 100644
> --- a/package/iputils/iputils.mk
> +++ b/package/iputils/iputils.mk
> @@ -52,6 +52,16 @@ endif
>  # XSL Stylesheets for DocBook 5 not packaged for buildroot
>  IPUTILS_CONF_OPTS += -DBUILD_MANS=false -DBUILD_HTML_MANS=false

> +# move iputils binaries to the same location as where Busybox installs
> +# the corresponding applets, so that we have a single version of the
> +# tools (from iputils)
> +define IPUTILS_MOVE_BINARIES
> +	mv $(TARGET_DIR)/usr/bin/arping $(TARGET_DIR)/usr/sbin/arping
> +	mv $(TARGET_DIR)/usr/bin/ping $(TARGET_DIR)/bin/ping
> +	mv $(TARGET_DIR)/usr/bin/tftpd $(TARGET_DIR)/usr/sbin/tftpd

Just small research about iputils install directories.
busybox keeps mixed dirs /usr/{s,}bin and /{s,}bin (locations which were before
usrmerge concept), ping is indeed in /bin [1].
Debian chose also mixing /usr/{s,}bin and /{s,}bin [2]

But I'm concern about /{usr/,}/bin vs. /{usr/,}/sbin.
iputils before starting to use  meson didn't have any default location.
There was iputils.spec file [3] (now deleted), which suggested some locations,
but was marked as for testing purposes only. It had different paths than busybox
and Debian use. I guess I have to accept that there was no consensus and
therefore are differences in paths across both upstreams and distros.

Kind regards,
Petr

[1] https://git.busybox.net/busybox/tree/networking/ping.c#n52
[2] https://salsa.debian.org/debian/iputils/blob/master/debian/rules
[3] https://github.com/iputils/iputils/blob/bffc0e957b98d626ab4cea218c89251201425442/iputils.spec#L45

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Thomas Petazzoni]

Hello Petr,

Thanks for the feedback.

On Mon, 17 Jun 2019 22:24:03 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:

> > iputils installs several programs that are also implemented as applets
> > in Busybox. Two of these (arping and tftpd) are installed by iputils
> > in /bin, while Busybox installs them in /usr/sbin, causing both to be  
>      ^
> I guess you mean /usr/bin

Gah, indeed. Unfortunately, the v2 of my patch has already been applied
by Arnout, and it has the same issue.

> Although I'm not keen on changing binary default location,
> this is simple straightforward solution. Alternative would be to disable busybox
> config CONFIG_ARPING (with support/kconfig/merge_config.sh), but that'd be too
> complicated. Thus ack

Adjusting the Busybox configuration is not the choice we have made to
solve this problem. Instead, the way we have chosen to solve the
conflict between Busybox applets and the "full-blown" variant of the
same tools is by:

 - Making "busybox" depend on all packages that provide the full-blown
   variants, so that those full-blown variants are built/installed
   before Busybox.

 - Ensure the Busybox installation process does not overwrite the
   full-blown variants when they are already installed.

This ensures that at the end of the build, if a full-blown variant is
installed, it takes precedence over the Busybox applet.

The drawback is while the Busybox symlink is not installed, the actual
code is present in the Busybox binary.

> Just small research about iputils install directories.
> busybox keeps mixed dirs /usr/{s,}bin and /{s,}bin (locations which were before
> usrmerge concept), ping is indeed in /bin [1].
> Debian chose also mixing /usr/{s,}bin and /{s,}bin [2]
> 
> But I'm concern about /{usr/,}/bin vs. /{usr/,}/sbin.
> iputils before starting to use  meson didn't have any default location.
> There was iputils.spec file [3] (now deleted), which suggested some locations,
> but was marked as for testing purposes only. It had different paths than busybox
> and Debian use. I guess I have to accept that there was no consensus and
> therefore are differences in paths across both upstreams and distros.

Due to the way we handle the Busybox applet vs. full-blown variant
conflict (detailed above), we really need the full-blown variant to be
installed at the same location as the corresponding Busybox applet.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Petr Vorel]

Hi Thomas,

> Hello Petr,

> Thanks for the feedback.

> On Mon, 17 Jun 2019 22:24:03 +0200
> Petr Vorel <petr.vorel@gmail.com> wrote:

> > > iputils installs several programs that are also implemented as applets
> > > in Busybox. Two of these (arping and tftpd) are installed by iputils
> > > in /bin, while Busybox installs them in /usr/sbin, causing both to be  
> >      ^
> > I guess you mean /usr/bin

> Gah, indeed. Unfortunately, the v2 of my patch has already been applied
> by Arnout, and it has the same issue.
Yes, I've noticed yesterday after sending review :).
But this is just a tiny detail.

> > Although I'm not keen on changing binary default location,
> > this is simple straightforward solution. Alternative would be to disable busybox
> > config CONFIG_ARPING (with support/kconfig/merge_config.sh), but that'd be too
> > complicated. Thus ack

> Adjusting the Busybox configuration is not the choice we have made to
> solve this problem. Instead, the way we have chosen to solve the
> conflict between Busybox applets and the "full-blown" variant of the
> same tools is by:

>  - Making "busybox" depend on all packages that provide the full-blown
>    variants, so that those full-blown variants are built/installed
>    before Busybox.

>  - Ensure the Busybox installation process does not overwrite the
>    full-blown variants when they are already installed.

> This ensures that at the end of the build, if a full-blown variant is
> installed, it takes precedence over the Busybox applet.

> The drawback is while the Busybox symlink is not installed, the actual
> code is present in the Busybox binary.

Thanks for an explanation. Having a bit bigger binary it's not a big deal.
If this is a problem for any reason, fortunately user can supply it's own
busybox config.
I guess you took this direction because it's much simpler than messing with
busybox config (the solution I suggested).

> > Just small research about iputils install directories.
> > busybox keeps mixed dirs /usr/{s,}bin and /{s,}bin (locations which were before
> > usrmerge concept), ping is indeed in /bin [1].
> > Debian chose also mixing /usr/{s,}bin and /{s,}bin [2]

> > But I'm concern about /{usr/,}/bin vs. /{usr/,}/sbin.
> > iputils before starting to use  meson didn't have any default location.
> > There was iputils.spec file [3] (now deleted), which suggested some locations,
> > but was marked as for testing purposes only. It had different paths than busybox
> > and Debian use. I guess I have to accept that there was no consensus and
> > therefore are differences in paths across both upstreams and distros.

> Due to the way we handle the Busybox applet vs. full-blown variant
> conflict (detailed above), we really need the full-blown variant to be
> installed at the same location as the corresponding Busybox applet.
Understand.

> Thomas

Kind regards,
Petr

[Buildroot] [PATCH 1/2] package/iputils: move binaries to the location also used by Busybox

[Thomas Petazzoni]

Hello,

On Tue, 18 Jun 2019 08:59:29 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:

> > Adjusting the Busybox configuration is not the choice we have made to
> > solve this problem. Instead, the way we have chosen to solve the
> > conflict between Busybox applets and the "full-blown" variant of the
> > same tools is by:  
> 
> >  - Making "busybox" depend on all packages that provide the full-blown
> >    variants, so that those full-blown variants are built/installed
> >    before Busybox.  
> 
> >  - Ensure the Busybox installation process does not overwrite the
> >    full-blown variants when they are already installed.  
> 
> > This ensures that at the end of the build, if a full-blown variant is
> > installed, it takes precedence over the Busybox applet.  
> 
> > The drawback is while the Busybox symlink is not installed, the actual
> > code is present in the Busybox binary.  
> 
> Thanks for an explanation. Having a bit bigger binary it's not a big deal.
> If this is a problem for any reason, fortunately user can supply it's own
> busybox config.
> I guess you took this direction because it's much simpler than messing with
> busybox config (the solution I suggested).

Yes, that's exactly the reason. It would require to keep track of which
Busybox options need to be disabled, depending on which other Buildroot
packages/options are enabled.

That being said, the current solution also requires that all packages
install their programs in the same location as the corresponding
Busybox applets, which also requires some maintenance effort.

None of the solutions is perfect, and a trade-off has to be made.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com