| Message ID | 20190616200919.4378-1-thomas.petazzoni@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] package/iputils: move binaries to the location also used by Busybox | expand |
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
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
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
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
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
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
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
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(+)