| Message ID | 20171213130131.15744-2-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Handle conflicting files with Busybox | expand |
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
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
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
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
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
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.
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...
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.
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
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.
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
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.
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.
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)))
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.
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
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.
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
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(+)