[RFC,1/2] busybox: avoid conflict with other packages

Message ID 20171213130131.15744-2-thomas.petazzoni@free-electrons.com
State Changes Requested
Headers show
Series
  • Handle conflicting files with Busybox
Related show

Commit Message

Thomas Petazzoni Dec. 13, 2017, 1:01 p.m.
One of the requirement to implement per-package SDK is that a package
should not overwrite files installed by another package. The busybox
package creates such a situation with numerous other packages, because
it provides applets that are also provided as full-featured programs
in other packages.

In order to avoid having other packages overwrite the Busybox applet
symbolic link with their own variant, this commit improves the logic
in busybox.mk to disable the applets when the corresponding
full-featured program will be installed by a different package.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Note: the list is not complete, this is just a RFC version to find out
if that's the direction we want to take.
---
 package/busybox/busybox.mk | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

Comments

Baruch Siach Dec. 13, 2017, 2:43 p.m. | #1
Hi Thomas,

On Wed, Dec 13, 2017 at 02:01:30PM +0100, Thomas Petazzoni wrote:
> One of the requirement to implement per-package SDK is that a package
> should not overwrite files installed by another package. The busybox
> package creates such a situation with numerous other packages, because
> it provides applets that are also provided as full-featured programs
> in other packages.
> 
> In order to avoid having other packages overwrite the Busybox applet
> symbolic link with their own variant, this commit improves the logic
> in busybox.mk to disable the applets when the corresponding
> full-featured program will be installed by a different package.

This means that a custom Busybox .config will automagically be transformed to 
something quite different. This is a significant change in behaviour. Can't we 
just move the symlink removal logic into busybox.mk instead? That would leave 
the busybox binary alone, and keep the magic .config transformations to 
minimum.

baruch

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> Note: the list is not complete, this is just a RFC version to find out
> if that's the direction we want to take.
> ---
>  package/busybox/busybox.mk | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> index 8b720b30b8..e51dbc4887 100644
> --- a/package/busybox/busybox.mk
> +++ b/package/busybox/busybox.mk
> @@ -250,6 +250,63 @@ endef
>  BUSYBOX_DEPENDENCIES += linux-pam
>  endif
>  
> +# Avoid collision with other packages
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_BC) += dc
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_BINUTILS) += ar strings
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_CPIO) += cpio
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DCRON) += crond crontab
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DEBIANUTILS) += run-parts which
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DIFFUTILS) += cmp diff
> +# test1 removes the [ applet
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_COREUTILS) += \
> +	chmod tail printf uname touch echo od realpath sha512sum rmdir readlink \
> +	sync mknod du true nlink ln ls fold who logname chown dirname chgrp \
> +	basename uptime sha256sum uniq env rm expr wc pwd tee cat test chroot \
> +	nohup head sha1sum df dd nl seq truncate id cp mkfifo tty whoami factor \
> +	tr mkdir paste cksum sort stty shred md5sum hostid nproc install date \
> +	mv sleep yes cut false unlink test1
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_FBSET) += fbset
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_KMOD_TOOLS) += lsmod rmmod modprobe insmod
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += blkid
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += dmesg
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += fdisk
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += flock
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += fstrim
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += hexdump
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += mkswap
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += readprofile
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += renice
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += setsid
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += swapoff
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += swapon
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_EJECT) += eject
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FALLOCATE) += fallocate
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FDFORMAT) += fdformat
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FSCK) += fsck
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_HWCLOCK) += hwclock
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_IPCRM) += ipcrm
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_IPCS) += ipcs
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_KILL) += kill
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LAST) += last
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOGGER) += logger
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOGIN) += login
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOSETUP) += losetup
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MESG) += mesg
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MORE) += more
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MOUNT) += mount umount
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MOUNTPOINT) += mountpoint
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_PIVOT_ROOT) += pivot_root
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SCHEDUTILS) += chrt
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SU) += su
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SULOGIN) += sulogin
> +BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SWITCH_ROOT) += switch_root
> +
> +define BUSYBOX_DROP_CONFLICTING_APPLETS
> +	$(foreach applet,$(BUSYBOX_DROP_APPLET_y),\
> +		$(call KCONFIG_DISABLE_OPT,CONFIG_$(call UPPERCASE,$(applet)),$(BUSYBOX_BUILD_CONFIG))
> +	)
> +endef
> +
>  # Telnet support
>  define BUSYBOX_INSTALL_TELNET_SCRIPT
>  	if grep -q CONFIG_FEATURE_TELNETD_STANDALONE=y $(@D)/.config; then \
> @@ -276,6 +333,7 @@ define BUSYBOX_KCONFIG_FIXUP_CMDS
>  	$(BUSYBOX_SET_SELINUX)
>  	$(BUSYBOX_SET_INDIVIDUAL_BINARIES)
>  	$(BUSYBOX_MUSL_TWEAKS)
> +	$(BUSYBOX_DROP_CONFLICTING_APPLETS)
>  endef
>  
>  define BUSYBOX_CONFIGURE_CMDS
Thomas Petazzoni Dec. 14, 2017, 5:18 a.m. | #2
Hello,

On Wed, 13 Dec 2017 16:43:05 +0200, Baruch Siach wrote:

> This means that a custom Busybox .config will automagically be transformed to 
> something quite different. This is a significant change in behaviour.

We already tweak the Busybox .config file quite significantly in
busybox.mk, so it's not really new. But I agree that the scale of the
change is very different.

> Can't we just move the symlink removal logic into busybox.mk instead?
> That would leave the busybox binary alone, and keep the magic .config
> transformations to minimum.

If I understand your proposal, we would not tweak the Busybox .config
file, but instead add a post install target hook that would remove the
symbolic links installed by Busybox "make install" when such symbolic
links are going to conflict with full-blown versions of the
corresponding applications, added by other packages. Is that correct ?

If so, then I see two drawbacks with this approach. They are not
unacceptable, but they are drawbacks:

 1. We have to keep the optional dependency on Busybox in util-linux,
    coreutils, fbset, and all those packages that conflict with Busybox.
    Indeed, removing the symbolic link installed by Busybox only works
    if Busybox is installed first. Unless Busybox "make install" is
    careful enough to not install a given symlink if a file of the
    same name already exists.

 2. The Busybox binary would contain lots of useless code, which simply
    can't be reached, except by calling explicitly "busybox cmd".

So neither are unacceptable drawbacks, but they are drawbacks. If the
general opinion is that we should do what you propose, I'm fine with
implementing that.

What do others think ?

Thomas
Baruch Siach Dec. 14, 2017, 6:58 a.m. | #3
Hi Thomas,

On Thu, Dec 14, 2017 at 06:18:50AM +0100, Thomas Petazzoni wrote:
> On Wed, 13 Dec 2017 16:43:05 +0200, Baruch Siach wrote:
> > This means that a custom Busybox .config will automagically be transformed 
> > to something quite different. This is a significant change in behaviour.
> 
> We already tweak the Busybox .config file quite significantly in
> busybox.mk, so it's not really new. But I agree that the scale of the
> change is very different.
> 
> > Can't we just move the symlink removal logic into busybox.mk instead?
> > That would leave the busybox binary alone, and keep the magic .config
> > transformations to minimum.
> 
> If I understand your proposal, we would not tweak the Busybox .config
> file, but instead add a post install target hook that would remove the
> symbolic links installed by Busybox "make install" when such symbolic
> links are going to conflict with full-blown versions of the
> corresponding applications, added by other packages. Is that correct ?
> 
> If so, then I see two drawbacks with this approach. They are not
> unacceptable, but they are drawbacks:
> 
>  1. We have to keep the optional dependency on Busybox in util-linux,
>     coreutils, fbset, and all those packages that conflict with Busybox.
>     Indeed, removing the symbolic link installed by Busybox only works
>     if Busybox is installed first. Unless Busybox "make install" is
>     careful enough to not install a given symlink if a file of the
>     same name already exists.

Quoting busybox.mk:

# Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
# full-blown versions of apps installed by other packages with sym/hard links.

Unfortunately, the noclobber implementation is racy. So the alternative is to 
remove apps from the generated busybox.links file that install.sh uses as 
input.

>  2. The Busybox binary would contain lots of useless code, which simply
>     can't be reached, except by calling explicitly "busybox cmd".

This is not different than the situation we have now. The added useless code 
size is in most cases negligible compared to any one of the full-blown 
version. In the end it's the responsibility of the user to fine tune the 
target image size.

> So neither are unacceptable drawbacks, but they are drawbacks. If the
> general opinion is that we should do what you propose, I'm fine with
> implementing that.
> 
> What do others think ?

baruch
Thomas Petazzoni Dec. 14, 2017, 7:17 a.m. | #4
Hello,

On Thu, 14 Dec 2017 08:58:07 +0200, Baruch Siach wrote:

> Quoting busybox.mk:
> 
> # Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
> # full-blown versions of apps installed by other packages with sym/hard links.
> 
> Unfortunately, the noclobber implementation is racy. So the alternative is to 
> remove apps from the generated busybox.links file that install.sh uses as 
> input.

The problem is that this busybox.links file gets generated as part of
the "install" target. So unless we explicitly call "make busybox.links"
in a pre-install hook, and then tweak it to remove the applets we don't
want, I don't see how it could work. Can be done, but it starts to be a
bit tricky :)

> >  2. The Busybox binary would contain lots of useless code, which simply
> >     can't be reached, except by calling explicitly "busybox cmd".  
> 
> This is not different than the situation we have now. The added useless code 
> size is in most cases negligible compared to any one of the full-blown 
> version. In the end it's the responsibility of the user to fine tune the 
> target image size.

I admit the size difference is minimal, and is clearly not the primary
motivation for this. To me, it was merely a nice side effect. I
measured a 70-80 KB reduction in the Busybox size when
BR2_PACKAGE_COREUTILS=y, so it's clearly not much compared to how much
disk space is consumed by the full-blown coreutils.

Still, I find it weird that we build support in the binary for
something that we don't expose in the target, while in terms of
complexity, disabling them in the configuration is as simple (perhaps
even more?) than preventing the symlinks from being created.

But again, if it's the general consensus, I'm fine with it. As long as
it helps things move forward towards per-package SDK and TLP support.

It would be nice to hear from Peter, Arnout or Yann for example.

Thanks for the feedback!

Thomas
Yann E. MORIN Dec. 28, 2017, 4:23 p.m. | #5
Baruch, All,

On 2017-12-14 08:58 +0200, Baruch Siach spake thusly:
> Hi Thomas,
> 
> On Thu, Dec 14, 2017 at 06:18:50AM +0100, Thomas Petazzoni wrote:
> > On Wed, 13 Dec 2017 16:43:05 +0200, Baruch Siach wrote:
> > > This means that a custom Busybox .config will automagically be transformed 
> > > to something quite different. This is a significant change in behaviour.
> > 
> > We already tweak the Busybox .config file quite significantly in
> > busybox.mk, so it's not really new. But I agree that the scale of the
> > change is very different.
> > 
> > > Can't we just move the symlink removal logic into busybox.mk instead?
> > > That would leave the busybox binary alone, and keep the magic .config
> > > transformations to minimum.
> > 
> > If I understand your proposal, we would not tweak the Busybox .config
> > file, but instead add a post install target hook that would remove the
> > symbolic links installed by Busybox "make install" when such symbolic
> > links are going to conflict with full-blown versions of the
> > corresponding applications, added by other packages. Is that correct ?
> > 
> > If so, then I see two drawbacks with this approach. They are not
> > unacceptable, but they are drawbacks:
> > 
> >  1. We have to keep the optional dependency on Busybox in util-linux,
> >     coreutils, fbset, and all those packages that conflict with Busybox.
> >     Indeed, removing the symbolic link installed by Busybox only works
> >     if Busybox is installed first. Unless Busybox "make install" is
> >     careful enough to not install a given symlink if a file of the
> >     same name already exists.
> 
> Quoting busybox.mk:
> 
> # Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
> # full-blown versions of apps installed by other packages with sym/hard links.
> 
> Unfortunately, the noclobber implementation is racy.

How is it racy?

At the moment we're gonna call it, no other package will be installing
at the same time (in the location seen by busybox at least), so there
should be no race.

Regards,
Yann E. MORIN.

> So the alternative is to 
> remove apps from the generated busybox.links file that install.sh uses as 
> input.
> 
> >  2. The Busybox binary would contain lots of useless code, which simply
> >     can't be reached, except by calling explicitly "busybox cmd".
> 
> This is not different than the situation we have now. The added useless code 
> size is in most cases negligible compared to any one of the full-blown 
> version. In the end it's the responsibility of the user to fine tune the 
> target image size.
> 
> > So neither are unacceptable drawbacks, but they are drawbacks. If the
> > general opinion is that we should do what you propose, I'm fine with
> > implementing that.
> > 
> > What do others think ?
> 
> baruch
> 
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Yann E. MORIN Dec. 28, 2017, 10:56 p.m. | #6
Baruch, Thomas, All,

On 2017-12-28 17:23 +0100, Yann E. MORIN spake thusly:
> On 2017-12-14 08:58 +0200, Baruch Siach spake thusly:
[--SNIP--]
> > Quoting busybox.mk:
> > 
> > # Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
> > # full-blown versions of apps installed by other packages with sym/hard links.
> > 
> > Unfortunately, the noclobber implementation is racy.
> 
> How is it racy?
> 
> At the moment we're gonna call it, no other package will be installing
> at the same time (in the location seen by busybox at least), so there
> should be no race.

Yet there were some issues with that part, so I've sent a small
patch-series to busybox to fix the noclobber stuff:
    http://lists.busybox.net/pipermail/busybox/2017-December/086039.html

So, with this, we should be able to:

  - invert the dependency chain, so thatr busybox now depends on all
    otehr packages that it may install applets of;

  - drop the big list of applets-to-disable in busybox, and rely on the
    install-noclobber install rule

If upstream accepts that series, of course...

Regards,
Yann E. MORIN.
Baruch Siach Dec. 29, 2017, 5:59 a.m. | #7
Hi Yann,

On Thu, Dec 28, 2017 at 11:56:39PM +0100, Yann E. MORIN wrote:
> On 2017-12-28 17:23 +0100, Yann E. MORIN spake thusly:
> > On 2017-12-14 08:58 +0200, Baruch Siach spake thusly:
> [--SNIP--]
> > > Quoting busybox.mk:
> > > 
> > > # Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
> > > # full-blown versions of apps installed by other packages with sym/hard links.
> > > 
> > > Unfortunately, the noclobber implementation is racy.
> > 
> > How is it racy?
> > 
> > At the moment we're gonna call it, no other package will be installing
> > at the same time (in the location seen by busybox at least), so there
> > should be no race.
> 
> Yet there were some issues with that part, so I've sent a small
> patch-series to busybox to fix the noclobber stuff:
>     http://lists.busybox.net/pipermail/busybox/2017-December/086039.html
> 
> So, with this, we should be able to:
> 
>   - invert the dependency chain, so thatr busybox now depends on all
>     otehr packages that it may install applets of;

Why would we need a dependency at all? Either busybox builds before that other 
package (like we do today) in which case the other package overwrites the 
busybox symlink, or that other package builds before busybox, and then the  
noclobber option does the right thing.

baruch

>   - drop the big list of applets-to-disable in busybox, and rely on the
>     install-noclobber install rule
> 
> If upstream accepts that series, of course...
Yann E. MORIN Dec. 29, 2017, 9:38 a.m. | #8
Baruch, All,

On 2017-12-29 07:59 +0200, Baruch Siach spake thusly:
> On Thu, Dec 28, 2017 at 11:56:39PM +0100, Yann E. MORIN wrote:
> > On 2017-12-28 17:23 +0100, Yann E. MORIN spake thusly:
> > > On 2017-12-14 08:58 +0200, Baruch Siach spake thusly:
> > [--SNIP--]
> > > > Quoting busybox.mk:
> > > > 
> > > > # Enable "noclobber" in install.sh, to prevent BusyBox from overwriting any
> > > > # full-blown versions of apps installed by other packages with sym/hard links.
> > > > 
> > > > Unfortunately, the noclobber implementation is racy.
> > > 
> > > How is it racy?
> > > 
> > > At the moment we're gonna call it, no other package will be installing
> > > at the same time (in the location seen by busybox at least), so there
> > > should be no race.
> > 
> > Yet there were some issues with that part, so I've sent a small
> > patch-series to busybox to fix the noclobber stuff:
> >     http://lists.busybox.net/pipermail/busybox/2017-December/086039.html
> > 
> > So, with this, we should be able to:
> > 
> >   - invert the dependency chain, so thatr busybox now depends on all
> >     otehr packages that it may install applets of;
> 
> Why would we need a dependency at all? Either busybox builds before that other 
> package (like we do today) in which case the other package overwrites the 
> busybox symlink, or that other package builds before busybox, and then the  
> noclobber option does the right thing.

As was discussed during the DevDays in Pragues about top-level parallel
build, we've concluded that we could not have two packages that touch
the same file.

When we are doing top-level parallel build, we no longer have any
guarantee of the ordering, especially the order packages install in
target/, so we loose the reproducibility that sequentiallity currently
provides.

So we have to ensure proper install ordering, so that we guarantee what
files are in target/.

So we either go with the current dependencies, added to all packages to
ensure they get installed after busybox, or we add them to busybox so it
gets installed after all the packages for which it may provide applets.

I prefer the second option, because it concentrates the dependency chain
in a single package, and it becomes very easy to write:

    BUSYBOX_DEPENDENCIES = \
        $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
        $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
    [...]

Rather than duplicate the optional dependency on Busybox in all the
packages (and as Thomas demonstrated, there are quite a few of them).

I may even go further: we could even make busybox depend on *all*
packages, irrespective on whether it provides applets for them or not,
since busybiox is pretty fast to build and does build in parallel quite
nicely anyway. That would nicely solve the issue.

Regards,
Yann E. MORIN.
Thomas Petazzoni Dec. 29, 2017, 9:42 a.m. | #9
Hello,

On Fri, 29 Dec 2017 10:38:18 +0100, Yann E. MORIN wrote:

> As was discussed during the DevDays in Pragues about top-level parallel
> build, we've concluded that we could not have two packages that touch
> the same file.
> 
> When we are doing top-level parallel build, we no longer have any
> guarantee of the ordering, especially the order packages install in
> target/, so we loose the reproducibility that sequentiallity currently
> provides.
> 
> So we have to ensure proper install ordering, so that we guarantee what
> files are in target/.
> 
> So we either go with the current dependencies, added to all packages to
> ensure they get installed after busybox, or we add them to busybox so it
> gets installed after all the packages for which it may provide applets.
> 
> I prefer the second option, because it concentrates the dependency chain
> in a single package, and it becomes very easy to write:
> 
>     BUSYBOX_DEPENDENCIES = \
>         $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
>         $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
>     [...]
> 
> Rather than duplicate the optional dependency on Busybox in all the
> packages (and as Thomas demonstrated, there are quite a few of them).

Seems like a good idea to me.

> I may even go further: we could even make busybox depend on *all*
> packages, irrespective on whether it provides applets for them or not,
> since busybiox is pretty fast to build and does build in parallel quite
> nicely anyway. That would nicely solve the issue.

I don't really like this approach, I find it too brutal.

Since we have the testing logic to validate that no package overwrites
files from another package, I think the approach of having explicit
dependencies in Busybox is good enough.

Best regards,

Thomas
Yann E. MORIN Dec. 29, 2017, 9:52 a.m. | #10
Thomas, All,

On 2017-12-29 10:42 +0100, Thomas Petazzoni spake thusly:
> On Fri, 29 Dec 2017 10:38:18 +0100, Yann E. MORIN wrote:
> > As was discussed during the DevDays in Pragues about top-level parallel
> > build, we've concluded that we could not have two packages that touch
> > the same file.
> > 
> > When we are doing top-level parallel build, we no longer have any
> > guarantee of the ordering, especially the order packages install in
> > target/, so we loose the reproducibility that sequentiallity currently
> > provides.
> > 
> > So we have to ensure proper install ordering, so that we guarantee what
> > files are in target/.
> > 
> > So we either go with the current dependencies, added to all packages to
> > ensure they get installed after busybox, or we add them to busybox so it
> > gets installed after all the packages for which it may provide applets.
> > 
> > I prefer the second option, because it concentrates the dependency chain
> > in a single package, and it becomes very easy to write:
> > 
> >     BUSYBOX_DEPENDENCIES = \
> >         $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
> >         $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
> >     [...]
> > 
> > Rather than duplicate the optional dependency on Busybox in all the
> > packages (and as Thomas demonstrated, there are quite a few of them).
> 
> Seems like a good idea to me.

For that, we'll need the three-patch series I submitted to Busybox:
    http://lists.busybox.net/pipermail/busybox/2017-December/086039.html

> > I may even go further: we could even make busybox depend on *all*
> > packages, irrespective on whether it provides applets for them or not,
> > since busybiox is pretty fast to build and does build in parallel quite
> > nicely anyway. That would nicely solve the issue.
> 
> I don't really like this approach, I find it too brutal.

Sorry, I was not advocating for this solution. What I meant was that, to
simplify things even further we could do that. But I don't think we need
that.

> Since we have the testing logic to validate that no package overwrites
> files from another package, I think the approach of having explicit
> dependencies in Busybox is good enough.

Once we turn the warniong into a hard error, yes. ;-)

Regards,
Yann E. MORIN.
Thomas Petazzoni Dec. 29, 2017, 9:55 a.m. | #11
Hello,

On Fri, 29 Dec 2017 10:52:42 +0100, Yann E. MORIN wrote:

> > > Rather than duplicate the optional dependency on Busybox in all the
> > > packages (and as Thomas demonstrated, there are quite a few of them).  
> > 
> > Seems like a good idea to me.  
> 
> For that, we'll need the three-patch series I submitted to Busybox:
>     http://lists.busybox.net/pipermail/busybox/2017-December/086039.html

Absolutely. Hopefully, upstream will give some feedback soon.

> > I don't really like this approach, I find it too brutal.  
> 
> Sorry, I was not advocating for this solution. What I meant was that, to
> simplify things even further we could do that. But I don't think we need
> that.

OK, then we agree :)

> > Since we have the testing logic to validate that no package overwrites
> > files from another package, I think the approach of having explicit
> > dependencies in Busybox is good enough.  
> 
> Once we turn the warniong into a hard error, yes. ;-)

In fact, I'd like to turn this into a hard error fairly soon. Or
perhaps I could do some research in the autobuilder logs to identify
the issues (other than Busybox) so that we try to fix them ahead of
time.

Best regards,

Thomas
Trent Piepho Dec. 29, 2017, 7:54 p.m. | #12
On Fri, 2017-12-29 at 10:38 +0100, Yann E. MORIN wrote:
> 
> I prefer the second option, because it concentrates the dependency chain
> in a single package, and it becomes very easy to write:
> 
>     BUSYBOX_DEPENDENCIES = \
>         $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
>         $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
>     [...]

Could one have a package variable specifically for install
dependencies, like:

BUSYBOX_INSTALL_DEPENDENCIES = coreutils util-linux ...

It seems like the base pkg infra could automagically turn that into
conditional dependencies, e.g. "coreutils" -> "(if
$(BR2_PACKAGE_COREUTILS), coreutils)".  And then append those to the
package dependencies.

So less boilerplate to write in the package file.

But it would also allow the possibility to take advantage of knowing
that coreutils is an install dependency rather than a build dependency.
 So it's allowed to build busybox and coreutils at the same time in
parallel.  The requirement is only that coreutils be installed to the
target dir first.  Usually installing is much faster than building, so
one gets more parallelism this way.

Of course this optimization is not necessary, but separating out the
install deps now allows the possibility in the future, while being less
boilerplate and more clear documentation now.
Yann E. MORIN Dec. 29, 2017, 8:18 p.m. | #13
Trent, All,

On 2017-12-29 19:54 +0000, Trent Piepho spake thusly:
> On Fri, 2017-12-29 at 10:38 +0100, Yann E. MORIN wrote:
> > 
> > I prefer the second option, because it concentrates the dependency chain
> > in a single package, and it becomes very easy to write:
> > 
> >     BUSYBOX_DEPENDENCIES = \
> >         $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
> >         $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
> >     [...]
> 
> Could one have a package variable specifically for install
> dependencies, like:

I think you already suggested so on IRC a while back, right?

In fact, I do see the reasoning behind this, but I don;t think there is
a big advantage for us, in Buildroot.

In any case, we will want those dependencies to be installed befiore the
package itself is installed, so we don't care that they were installed
before the package is built.

One case where that might be interesting is to test-build a package
without building its install dependencies. But except for busybox, it
would only be usefull for very few packages, because in the vast
majority of cases, dependencies are build dependencies.

So I don't think it warrants the extra complexity in the infra...

> BUSYBOX_INSTALL_DEPENDENCIES = coreutils util-linux ...

If we were to add support for install dependencies, we would want to
treat them as the other dependencies and not do magical thigns like list
all of them and defer to the infra to sort out those that are enabeld.

In fact, that would go contrary to what we currently do for the existing
dependencies: if a package lists a dependency but it is not enabled, we
explicitly fail.

So we'd want to have:

    BUSYBOX_INSTALL_DEPENDENCIES = \
        $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
        [...]

Adn then...

> It seems like the base pkg infra could automagically turn that into
> conditional dependencies, e.g. "coreutils" -> "(if
> $(BR2_PACKAGE_COREUTILS), coreutils)".  And then append those to the
> package dependencies.
> 
> So less boilerplate to write in the package file.

... that would not diminish the "boilerplate" in the package's .mk file.

But know that this is specifically just for busybox (and maybe a very
few other packages), so that really does not warrant extra support in
the infra, IMHO...

Regards,
Yann E. MORIN.

> But it would also allow the possibility to take advantage of knowing
> that coreutils is an install dependency rather than a build dependency.
>  So it's allowed to build busybox and coreutils at the same time in
> parallel.  The requirement is only that coreutils be installed to the
> target dir first.  Usually installing is much faster than building, so
> one gets more parallelism this way.
> 
> Of course this optimization is not necessary, but separating out the
> install deps now allows the possibility in the future, while being less
> boilerplate and more clear documentation now.
Trent Piepho Dec. 29, 2017, 9:50 p.m. | #14
On Fri, 2017-12-29 at 21:18 +0100, Yann E. MORIN wrote:
> On 2017-12-29 19:54 +0000, Trent Piepho spake thusly:
> > On Fri, 2017-12-29 at 10:38 +0100, Yann E. MORIN wrote:
> > > 
> > > I prefer the second option, because it concentrates the dependency chain
> > > in a single package, and it becomes very easy to write:
> > > 
> > >     BUSYBOX_DEPENDENCIES = \
> > >         $(if $(BR2_PACKAGE_COREUTILS),coreutils) \
> > >         $(if $(BR2_PACKAGE_UTIL_LINUX),util-linux) \
> > >     [...]
> > 
> > Could one have a package variable specifically for install
> > dependencies, like:
> 
> I think you already suggested so on IRC a while back, right?

Yes LTIB did this.  LTIB was fancier about automatically taking into
account changes in configuration to minimize rebuilding (disks were
slower then!) and this helped.  I don't think it's important w.r.t.
what buildroot could do here.

Buildroot is different software of course and I don't think LTIB is
important w.r.t. what buildroot could do here. Rather, consider that at
one time buildroot could not build packages in parallel and now it
will.  Just because something does not appear useful now does not mean
it never will be.

> If we were to add support for install dependencies, we would want to
> treat them as the other dependencies and not do magical thigns like list
> all of them and defer to the infra to sort out those that are enabeld.
> 
> In fact, that would go contrary to what we currently do for the existing
> dependencies: if a package lists a dependency but it is not enabled, we
> explicitly fail.

But it seems like this the exact opposite of what is being done?!  If
one uses:
BUSYBOX_DEPENDENCIES += $(if $(BR2_PACKAGE_COREUTILS),coreutils)

Then make busybox-show-depends does NOT list coreutils, because I have
not enabled coreutils.  Same with graph-depends, coreutils-show-
rdepends, and so on.

But if one does:
BUSYBOX_INSTALL_DEPENDENCIES += coreutils

Then it's possible to choose another policy.  One could make show-
depends only show the dependency if coreutils is enabled, just like
now, but add a show-all-install-depends target that lists all possible
dependencies, whether enabled or not.  Maybe someone will want
that?graph-depends could use a different color for the lines.

But if the distinction is not made, then all these possibilities go
away.

It's not that complex either, one line in pkg-generic before setting
FINAL_DEPENDENCIES

$(2)_DEPENDENCIES += $$(foreach p,$$($(2)_INSTALL_DEPENDENCIES),$$(if $$(BR2_PACKAGE_$$(call UPPERCASE,$$(p))),$$(p)))
Yann E. MORIN Jan. 4, 2018, 3:20 p.m. | #15
Thomas, All,

On 2017-12-29 10:55 +0100, Thomas Petazzoni spake thusly:
> On Fri, 29 Dec 2017 10:52:42 +0100, Yann E. MORIN wrote:
> > > > Rather than duplicate the optional dependency on Busybox in all the
> > > > packages (and as Thomas demonstrated, there are quite a few of them).  
> > > Seems like a good idea to me.  
> > For that, we'll need the three-patch series I submitted to Busybox:
> >     http://lists.busybox.net/pipermail/busybox/2017-December/086039.html
> Absolutely. Hopefully, upstream will give some feedback soon.

Upstream has merged those three patches now, so we can rely on a clean
way to install without clobbering existing utils; we just have to use:
    make install-noclobber

Regards,
Yann E. MORIN.
Thomas Petazzoni Jan. 4, 2018, 3:29 p.m. | #16
Hello,

On Thu, 4 Jan 2018 16:20:48 +0100, Yann E. MORIN wrote:

> Upstream has merged those three patches now, so we can rely on a clean
> way to install without clobbering existing utils; we just have to use:
>     make install-noclobber

Great! So the idea is to have Busybox install its stuff *after* all the
packages that install full-blown versions. Correct ?

Thomas
Yann E. MORIN Jan. 4, 2018, 3:39 p.m. | #17
Thomas, All,

On 2018-01-04 16:29 +0100, Thomas Petazzoni spake thusly:
> On Thu, 4 Jan 2018 16:20:48 +0100, Yann E. MORIN wrote:
> > Upstream has merged those three patches now, so we can rely on a clean
> > way to install without clobbering existing utils; we just have to use:
> >     make install-noclobber
> Great! So the idea is to have Busybox install its stuff *after* all the
> packages that install full-blown versions. Correct ?

Yes, that is what I understood from the discussion, we would just need to:

1. remove the conditional dependencies on busybox in all packages;

2. add a conditrional dependency in busybox for each package of which
   busybox may install an applet;

3. use the install-noclobber rule;

4. drop the BUSYBOX_NOCLOBBER_INSTALL hook.

Regards,
Yann E. MORIN.

Patch

diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index 8b720b30b8..e51dbc4887 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -250,6 +250,63 @@  endef
 BUSYBOX_DEPENDENCIES += linux-pam
 endif
 
+# Avoid collision with other packages
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_BC) += dc
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_BINUTILS) += ar strings
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_CPIO) += cpio
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DCRON) += crond crontab
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DEBIANUTILS) += run-parts which
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_DIFFUTILS) += cmp diff
+# test1 removes the [ applet
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_COREUTILS) += \
+	chmod tail printf uname touch echo od realpath sha512sum rmdir readlink \
+	sync mknod du true nlink ln ls fold who logname chown dirname chgrp \
+	basename uptime sha256sum uniq env rm expr wc pwd tee cat test chroot \
+	nohup head sha1sum df dd nl seq truncate id cp mkfifo tty whoami factor \
+	tr mkdir paste cksum sort stty shred md5sum hostid nproc install date \
+	mv sleep yes cut false unlink test1
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_FBSET) += fbset
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_KMOD_TOOLS) += lsmod rmmod modprobe insmod
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += blkid
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += dmesg
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += fdisk
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += flock
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += fstrim
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += hexdump
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += mkswap
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += readprofile
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += renice
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += setsid
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += swapoff
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_BINARIES) += swapon
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_EJECT) += eject
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FALLOCATE) += fallocate
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FDFORMAT) += fdformat
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_FSCK) += fsck
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_HWCLOCK) += hwclock
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_IPCRM) += ipcrm
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_IPCS) += ipcs
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_KILL) += kill
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LAST) += last
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOGGER) += logger
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOGIN) += login
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_LOSETUP) += losetup
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MESG) += mesg
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MORE) += more
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MOUNT) += mount umount
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_MOUNTPOINT) += mountpoint
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_PIVOT_ROOT) += pivot_root
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SCHEDUTILS) += chrt
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SU) += su
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SULOGIN) += sulogin
+BUSYBOX_DROP_APPLET_$(BR2_PACKAGE_UTIL_LINUX_SWITCH_ROOT) += switch_root
+
+define BUSYBOX_DROP_CONFLICTING_APPLETS
+	$(foreach applet,$(BUSYBOX_DROP_APPLET_y),\
+		$(call KCONFIG_DISABLE_OPT,CONFIG_$(call UPPERCASE,$(applet)),$(BUSYBOX_BUILD_CONFIG))
+	)
+endef
+
 # Telnet support
 define BUSYBOX_INSTALL_TELNET_SCRIPT
 	if grep -q CONFIG_FEATURE_TELNETD_STANDALONE=y $(@D)/.config; then \
@@ -276,6 +333,7 @@  define BUSYBOX_KCONFIG_FIXUP_CMDS
 	$(BUSYBOX_SET_SELINUX)
 	$(BUSYBOX_SET_INDIVIDUAL_BINARIES)
 	$(BUSYBOX_MUSL_TWEAKS)
+	$(BUSYBOX_DROP_CONFLICTING_APPLETS)
 endef
 
 define BUSYBOX_CONFIGURE_CMDS