| Message ID | 20221128190747.1917274-1-james.hilliard1@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] Kconfig: Add HAVE_LIBUBOOTENV | expand |
Hi James, On 28.11.22 20:07, James Hilliard wrote: > Allow passing through the environment if libubootenv is available > for the build target. > > Defaults to yes because we do not do any library checking for now. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Kconfig | 4 ++++ > Makefile.deps | 4 ++++ > bootloader/Config.in | 4 ++++ > 3 files changed, 12 insertions(+) > > diff --git a/Kconfig b/Kconfig > index 85fa5fd..bcea48b 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -53,6 +53,10 @@ config HAVE_LIBUBI > bool > option env="HAVE_LIBUBI" > > +config HAVE_LIBUBOOTENV > + bool > + option env="HAVE_LIBUBOOTENV" > + > config HAVE_LIBEXT2FS > bool > option env="HAVE_LIBEXT2FS" > diff --git a/Makefile.deps b/Makefile.deps > index 08df4e2..97ac96e 100644 > --- a/Makefile.deps > +++ b/Makefile.deps > @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > export HAVE_LIBUBI = y > endif > > +ifeq ($(HAVE_LIBUBOOTENV),) > +export HAVE_LIBUBOOTENV = y > +endif > + Your changes were already in SWUpdate but they were explicitely removed with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and the bootloader interface can be dinamically loaded (only at startup). You want to bring them back, so I see at least a conflict with Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? Regards, Stefano > ifeq ($(HAVE_LIBZEROMQ),) > export HAVE_LIBZEROMQ = y > endif > diff --git a/bootloader/Config.in b/bootloader/Config.in > index 1744b61..fc133f3 100644 > --- a/bootloader/Config.in > +++ b/bootloader/Config.in > @@ -20,10 +20,14 @@ config BOOTLOADER_EBG > > config UBOOT > bool "U-Boot" > + depends on HAVE_LIBUBOOTENV > help > Support for U-Boot > https://www.denx.de/wiki/U-Boot > > +comment "U-Boot needs libubootenv" > + depends on !HAVE_LIBUBOOTENV > + > config UBOOT_FWENV > string "U-Boot Environment Configuration file" > depends on UBOOT
On Tue, Nov 29, 2022 at 12:21 PM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, > > On 28.11.22 20:07, James Hilliard wrote: > > Allow passing through the environment if libubootenv is available > > for the build target. > > > > Defaults to yes because we do not do any library checking for now. > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > Kconfig | 4 ++++ > > Makefile.deps | 4 ++++ > > bootloader/Config.in | 4 ++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/Kconfig b/Kconfig > > index 85fa5fd..bcea48b 100644 > > --- a/Kconfig > > +++ b/Kconfig > > @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > bool > > option env="HAVE_LIBUBI" > > > > +config HAVE_LIBUBOOTENV > > + bool > > + option env="HAVE_LIBUBOOTENV" > > + > > config HAVE_LIBEXT2FS > > bool > > option env="HAVE_LIBEXT2FS" > > diff --git a/Makefile.deps b/Makefile.deps > > index 08df4e2..97ac96e 100644 > > --- a/Makefile.deps > > +++ b/Makefile.deps > > @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > export HAVE_LIBUBI = y > > endif > > > > +ifeq ($(HAVE_LIBUBOOTENV),) > > +export HAVE_LIBUBOOTENV = y > > +endif > > + > > Your changes were already in SWUpdate but they were explicitely removed > with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > the bootloader interface can be dinamically loaded (only at startup). > You want to bring them back, so I see at least a conflict with > Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb The purpose of this is to prevent a build failure due to that missing header. > > Regards, > Stefano > > > ifeq ($(HAVE_LIBZEROMQ),) > > export HAVE_LIBZEROMQ = y > > endif > > diff --git a/bootloader/Config.in b/bootloader/Config.in > > index 1744b61..fc133f3 100644 > > --- a/bootloader/Config.in > > +++ b/bootloader/Config.in > > @@ -20,10 +20,14 @@ config BOOTLOADER_EBG > > > > config UBOOT > > bool "U-Boot" > > + depends on HAVE_LIBUBOOTENV > > help > > Support for U-Boot > > https://www.denx.de/wiki/U-Boot > > > > +comment "U-Boot needs libubootenv" > > + depends on !HAVE_LIBUBOOTENV > > + > > config UBOOT_FWENV > > string "U-Boot Environment Configuration file" > > depends on UBOOT > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== >
Hi, > > > Allow passing through the environment if libubootenv is available > > > for the build target. > > > > > > Defaults to yes because we do not do any library checking for now. > > > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > --- > > > Kconfig | 4 ++++ > > > Makefile.deps | 4 ++++ > > > bootloader/Config.in | 4 ++++ > > > 3 files changed, 12 insertions(+) > > > > > > diff --git a/Kconfig b/Kconfig > > > index 85fa5fd..bcea48b 100644 > > > --- a/Kconfig > > > +++ b/Kconfig > > > @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > > bool > > > option env="HAVE_LIBUBI" > > > > > > +config HAVE_LIBUBOOTENV > > > + bool > > > + option env="HAVE_LIBUBOOTENV" > > > + > > > config HAVE_LIBEXT2FS > > > bool > > > option env="HAVE_LIBEXT2FS" > > > diff --git a/Makefile.deps b/Makefile.deps > > > index 08df4e2..97ac96e 100644 > > > --- a/Makefile.deps > > > +++ b/Makefile.deps > > > @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > > export HAVE_LIBUBI = y > > > endif > > > > > > +ifeq ($(HAVE_LIBUBOOTENV),) > > > +export HAVE_LIBUBOOTENV = y > > > +endif > > > + > > > > Your changes were already in SWUpdate but they were explicitely removed > > with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > > the bootloader interface can be dinamically loaded (only at startup). > > You want to bring them back, so I see at least a conflict with > > Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > > Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > > The purpose of this is to prevent a build failure due to that missing header. CONFIG_UBOOT depends on libuboot.h from libubootenv for proper struct libuboot definition w.r.t. the types used therein, see https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning there's just the dependency on the type definitions. The same is true for EFI Boot Guard as well by the way. So, I do see the following options: (1) Port those external type definition to the bootloader binding implementations in SWUpdate (need to be maintained!) or (2) wire SWUpdate compilation to detected libraries on the build system (using e.g. pkgconf) ― this may help other such dependencies as well. Currently, if you tick an option you have to make sure that the required dependencies are satisfied or compilation will fail. I do not really have a (strong) opinion on this.... Kind regards, Christian
Hi James, Christian, On 30.11.22 09:32, Christian Storm wrote: > Hi, > >>>> Allow passing through the environment if libubootenv is available >>>> for the build target. >>>> >>>> Defaults to yes because we do not do any library checking for now. >>>> >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>> --- >>>> Kconfig | 4 ++++ >>>> Makefile.deps | 4 ++++ >>>> bootloader/Config.in | 4 ++++ >>>> 3 files changed, 12 insertions(+) >>>> >>>> diff --git a/Kconfig b/Kconfig >>>> index 85fa5fd..bcea48b 100644 >>>> --- a/Kconfig >>>> +++ b/Kconfig >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI >>>> bool >>>> option env="HAVE_LIBUBI" >>>> >>>> +config HAVE_LIBUBOOTENV >>>> + bool >>>> + option env="HAVE_LIBUBOOTENV" >>>> + >>>> config HAVE_LIBEXT2FS >>>> bool >>>> option env="HAVE_LIBEXT2FS" >>>> diff --git a/Makefile.deps b/Makefile.deps >>>> index 08df4e2..97ac96e 100644 >>>> --- a/Makefile.deps >>>> +++ b/Makefile.deps >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) >>>> export HAVE_LIBUBI = y >>>> endif >>>> >>>> +ifeq ($(HAVE_LIBUBOOTENV),) >>>> +export HAVE_LIBUBOOTENV = y >>>> +endif >>>> + >>> >>> Your changes were already in SWUpdate but they were explicitely removed >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and >>> the bootloader interface can be dinamically loaded (only at startup). >>> You want to bring them back, so I see at least a conflict with >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? >> >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb >> Yes, but the same is for EBG: https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 and this is not treated in your patch. >> The purpose of this is to prevent a build failure due to that missing header. > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > struct libuboot definition w.r.t. the types used therein, see > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > there's just the dependency on the type definitions. > The same is true for EFI Boot Guard as well by the way. > > So, I do see the following options: I see several cases depending on the build systems. Probably, we should analyze separately. James is fixing inside Buildroot. 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If CONFIG_UBOOT is set, libubootenv is automatically added to the list of dependencies. 2 - for Debian / Linux disto, Christian's option 2 works, but dependencies are also set into the distro's package. In Debian, dependencies are listed in the control file (Build-Depends), and libubootenv is always set. 3 - Buildroot have always used the HAVE_*, so I guess this is the reason for this patch. In this sense, James' patch seems correct to me, but then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? > (1) Port those external type definition to the bootloader binding > implementations in SWUpdate (need to be maintained!) or We duplicate code and we go into trouble if the libraries are changing the API. > (2) wire SWUpdate compilation to detected libraries on the build system > (using e.g. pkgconf) ― this may help other such dependencies as well. This works on some build system, of course Linux Distros (but it is not used in current Debian Package). My point here (apart having the same behavior for all bootloaders, that is EBG, too) is if this patch has drawbacks that I do not understand / see, because these HAVE_ were explicitely removed with the commit above. > Currently, if you tick an option you have to make sure that the required > dependencies are satisfied or compilation will fail. > I do not really have a (strong) opinion on this.... > > Best regards, Stefano
On Wed, Nov 30, 2022 at 7:29 AM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, Christian, > > On 30.11.22 09:32, Christian Storm wrote: > > Hi, > > > >>>> Allow passing through the environment if libubootenv is available > >>>> for the build target. > >>>> > >>>> Defaults to yes because we do not do any library checking for now. > >>>> > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>>> --- > >>>> Kconfig | 4 ++++ > >>>> Makefile.deps | 4 ++++ > >>>> bootloader/Config.in | 4 ++++ > >>>> 3 files changed, 12 insertions(+) > >>>> > >>>> diff --git a/Kconfig b/Kconfig > >>>> index 85fa5fd..bcea48b 100644 > >>>> --- a/Kconfig > >>>> +++ b/Kconfig > >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > >>>> bool > >>>> option env="HAVE_LIBUBI" > >>>> > >>>> +config HAVE_LIBUBOOTENV > >>>> + bool > >>>> + option env="HAVE_LIBUBOOTENV" > >>>> + > >>>> config HAVE_LIBEXT2FS > >>>> bool > >>>> option env="HAVE_LIBEXT2FS" > >>>> diff --git a/Makefile.deps b/Makefile.deps > >>>> index 08df4e2..97ac96e 100644 > >>>> --- a/Makefile.deps > >>>> +++ b/Makefile.deps > >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > >>>> export HAVE_LIBUBI = y > >>>> endif > >>>> > >>>> +ifeq ($(HAVE_LIBUBOOTENV),) > >>>> +export HAVE_LIBUBOOTENV = y > >>>> +endif > >>>> + > >>> > >>> Your changes were already in SWUpdate but they were explicitely removed > >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > >>> the bootloader interface can be dinamically loaded (only at startup). > >>> You want to bring them back, so I see at least a conflict with > >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > >> > >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > >> > > Yes, but the same is for EBG: > > https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > > and this is not treated in your patch. > > >> The purpose of this is to prevent a build failure due to that missing header. > > > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > > struct libuboot definition w.r.t. the types used therein, see > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > > there's just the dependency on the type definitions. > > The same is true for EFI Boot Guard as well by the way. > > > > So, I do see the following options: > > I see several cases depending on the build systems. Probably, we should > analyze separately. James is fixing inside Buildroot. > > 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > CONFIG_UBOOT is set, libubootenv is automatically added to the list of > dependencies. > > 2 - for Debian / Linux disto, Christian's option 2 works, but > dependencies are also set into the distro's package. In Debian, > dependencies are listed in the control file (Build-Depends), and > libubootenv is always set. > > 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > for this patch. In this sense, James' patch seems correct to me, but > then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? Well we never really hit the EBG issue since buildroot hasn't ever packaged efibootguard at all. I guess we probably should reintroduce the HAVE_LIBEBGENV variable in case we add efibootguard support in the future. > > > (1) Port those external type definition to the bootloader binding > > implementations in SWUpdate (need to be maintained!) or > > We duplicate code and we go into trouble if the libraries are changing > the API. For buildroot runtime and compile time libraries should always be the same as we don't really support binary package installation. I think it may make sense to have an option to disable dlopen library loading entirely as this feature only seems useful for systems that support binary package installation. > > > (2) wire SWUpdate compilation to detected libraries on the build system > > (using e.g. pkgconf) ― this may help other such dependencies as well. > > This works on some build system, of course Linux Distros (but it is not > used in current Debian Package). I don't think this really works with buildroot either as we want to pass the available libraries at configure time(available libraries selected in buildroot's configuration which is also kconfig based) while using something like pkgconf would not work as the packages are not actually being built when configuring. > > My point here (apart having the same behavior for all bootloaders, that > is EBG, too) is if this patch has drawbacks that I do not understand / > see, because these HAVE_ were explicitely removed with the commit above. > > > Currently, if you tick an option you have to make sure that the required > > dependencies are satisfied or compilation will fail. > > I do not really have a (strong) opinion on this.... > > > > > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== >
Hi, > > >>>> Allow passing through the environment if libubootenv is available > > >>>> for the build target. > > >>>> > > >>>> Defaults to yes because we do not do any library checking for now. > > >>>> > > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > >>>> --- > > >>>> Kconfig | 4 ++++ > > >>>> Makefile.deps | 4 ++++ > > >>>> bootloader/Config.in | 4 ++++ > > >>>> 3 files changed, 12 insertions(+) > > >>>> > > >>>> diff --git a/Kconfig b/Kconfig > > >>>> index 85fa5fd..bcea48b 100644 > > >>>> --- a/Kconfig > > >>>> +++ b/Kconfig > > >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > >>>> bool > > >>>> option env="HAVE_LIBUBI" > > >>>> > > >>>> +config HAVE_LIBUBOOTENV > > >>>> + bool > > >>>> + option env="HAVE_LIBUBOOTENV" > > >>>> + > > >>>> config HAVE_LIBEXT2FS > > >>>> bool > > >>>> option env="HAVE_LIBEXT2FS" > > >>>> diff --git a/Makefile.deps b/Makefile.deps > > >>>> index 08df4e2..97ac96e 100644 > > >>>> --- a/Makefile.deps > > >>>> +++ b/Makefile.deps > > >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > >>>> export HAVE_LIBUBI = y > > >>>> endif > > >>>> > > >>>> +ifeq ($(HAVE_LIBUBOOTENV),) > > >>>> +export HAVE_LIBUBOOTENV = y > > >>>> +endif > > >>>> + > > >>> > > >>> Your changes were already in SWUpdate but they were explicitely removed > > >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > > >>> the bootloader interface can be dinamically loaded (only at startup). > > >>> You want to bring them back, so I see at least a conflict with > > >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > > >> > > >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > > >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > > >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > > >> > > > > Yes, but the same is for EBG: > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > > > > and this is not treated in your patch. > > > > >> The purpose of this is to prevent a build failure due to that missing header. > > > > > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > > > struct libuboot definition w.r.t. the types used therein, see > > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > > > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > > > there's just the dependency on the type definitions. > > > The same is true for EFI Boot Guard as well by the way. > > > > > > So, I do see the following options: > > > > I see several cases depending on the build systems. Probably, we should > > analyze separately. James is fixing inside Buildroot. > > > > 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > > CONFIG_UBOOT is set, libubootenv is automatically added to the list of > > dependencies. > > > > 2 - for Debian / Linux disto, Christian's option 2 works, but > > dependencies are also set into the distro's package. In Debian, > > dependencies are listed in the control file (Build-Depends), and > > libubootenv is always set. > > > > 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > > for this patch. In this sense, James' patch seems correct to me, but > > then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? It may be correct for Buildroot but then it's a downstream patch in Buildroot rather than a patch for SWUpdate upstream, IMO. The others (OE, distros, ...) do the build environment setup taking the wanted SWUpdate configuration into account and then build SWUpdate with all the dependencies for the wanted SWUpdate configuration met. Can't Buildroot do this, too? > Well we never really hit the EBG issue since buildroot hasn't ever packaged > efibootguard at all. I guess we probably should reintroduce the > HAVE_LIBEBGENV variable in case we add efibootguard support in the > future. You will run into the same problem as EFI Boot Guard integration works alike. Eventually, we may probably also have others like zchunk or rsync run-time configurable and then we have the problem again. So, ideally, the HAVE_ options should be set depending on the SWUpdate options chosen and the available libraries on the build/target system at build-time. If they don't match, it doesn't build. This would also solve your problem I guess. > > > (1) Port those external type definition to the bootloader binding > > > implementations in SWUpdate (need to be maintained!) or > > > > We duplicate code and we go into trouble if the libraries are changing > > the API. Right, and it's work on SWUpdate's side that can be avoided, agreed. > For buildroot runtime and compile time libraries should always be the > same as we don't really support binary package installation. > > I think it may make sense to have an option to disable dlopen library > loading entirely as this feature only seems useful for systems that > support binary package installation. Exactly this was the purpose: Enable bootloader selection at run-time by configuration rather than statically at compile-time. This actually enables SWUpdate to be shipped as a package by (binary) distributions with probably support for multiple bootloaders enabled. Then, you have to prepare an SWUpdate package per architecture and just configure it to use the right bootloader (out of those whose support is enabled). If you disable this feature, then you're back at specifically building SWUpdate packages for each architecture + target combination. While this may work (better?) for Buildroot, it destroys the (binary) distribution use case, e.g., Debian. Even so, with the implemented probing, you do have the same run-time and compile-time libraries: They're just loaded differently (dlopen() vs. ldlinux). If you chose at compile-time to just use U-Boot, then only U-Boot support is enabled and is the default, i.e., libubootenv is dlopen()'d on SWUpdate startup. So I do not see a benefit in introducing an option to disable this feature. Did I overlook one? > > > (2) wire SWUpdate compilation to detected libraries on the build system > > > (using e.g. pkgconf) ― this may help other such dependencies as well. > > > > This works on some build system, of course Linux Distros (but it is not > > used in current Debian Package). Yes, some do, some don't and use other mechanisms but it's an option. Anyway, we probably don't want to mandate using it. It was meant as an example of how SWUpdate can detect whether the required dependencies are installed and bailing out otherwise with a probably more "friendly" error message ;) This is for build-time on the build system. On the target system you have to install the required dependencies, too. So you need some machinery to keep build- and run-time dependencies in sync. > I don't think this really works with buildroot either as we want to > pass the available libraries at configure time (available libraries > selected in buildroot's configuration which is also kconfig based) > while using something like pkgconf would not work as the packages are > not actually being built when configuring. Hm, I don't get this point: You have to compile the selected dependencies of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf entries which you can then query for auto-detecting the dependencies of SWUpdate, can't you? > > My point here (apart having the same behavior for all bootloaders, that > > is EBG, too) is if this patch has drawbacks that I do not understand / > > see, because these HAVE_ were explicitely removed with the commit above. They were removed since you select to compile in support for bootloaders at run-time. This doesn't necessarily mean that there actually is the proper library on the target system. So you have to configure it on the target to chose the bootloader for which (1) SWUpdate has support compiled in and (2) whose support library is actually installed. Relying on the HAVE_ options all turned on means you always build support for all bootloaders ― which is not what you may want? > > > Currently, if you tick an option you have to make sure that the required > > > dependencies are satisfied or compilation will fail. > > > I do not really have a (strong) opinion on this.... Kind regards, Christian
On Fri, Dec 2, 2022 at 6:15 AM Christian Storm <christian.storm@siemens.com> wrote: > > Hi, > > > > >>>> Allow passing through the environment if libubootenv is available > > > >>>> for the build target. > > > >>>> > > > >>>> Defaults to yes because we do not do any library checking for now. > > > >>>> > > > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > >>>> --- > > > >>>> Kconfig | 4 ++++ > > > >>>> Makefile.deps | 4 ++++ > > > >>>> bootloader/Config.in | 4 ++++ > > > >>>> 3 files changed, 12 insertions(+) > > > >>>> > > > >>>> diff --git a/Kconfig b/Kconfig > > > >>>> index 85fa5fd..bcea48b 100644 > > > >>>> --- a/Kconfig > > > >>>> +++ b/Kconfig > > > >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > > >>>> bool > > > >>>> option env="HAVE_LIBUBI" > > > >>>> > > > >>>> +config HAVE_LIBUBOOTENV > > > >>>> + bool > > > >>>> + option env="HAVE_LIBUBOOTENV" > > > >>>> + > > > >>>> config HAVE_LIBEXT2FS > > > >>>> bool > > > >>>> option env="HAVE_LIBEXT2FS" > > > >>>> diff --git a/Makefile.deps b/Makefile.deps > > > >>>> index 08df4e2..97ac96e 100644 > > > >>>> --- a/Makefile.deps > > > >>>> +++ b/Makefile.deps > > > >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > > >>>> export HAVE_LIBUBI = y > > > >>>> endif > > > >>>> > > > >>>> +ifeq ($(HAVE_LIBUBOOTENV),) > > > >>>> +export HAVE_LIBUBOOTENV = y > > > >>>> +endif > > > >>>> + > > > >>> > > > >>> Your changes were already in SWUpdate but they were explicitely removed > > > >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > > > >>> the bootloader interface can be dinamically loaded (only at startup). > > > >>> You want to bring them back, so I see at least a conflict with > > > >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > > > >> > > > >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > > > >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > > > >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > > > >> > > > > > > Yes, but the same is for EBG: > > > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > > > > > > and this is not treated in your patch. > > > > > > >> The purpose of this is to prevent a build failure due to that missing header. > > > > > > > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > > > > struct libuboot definition w.r.t. the types used therein, see > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > > > > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > > > > there's just the dependency on the type definitions. > > > > The same is true for EFI Boot Guard as well by the way. > > > > > > > > So, I do see the following options: > > > > > > I see several cases depending on the build systems. Probably, we should > > > analyze separately. James is fixing inside Buildroot. > > > > > > 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > > > CONFIG_UBOOT is set, libubootenv is automatically added to the list of > > > dependencies. > > > > > > 2 - for Debian / Linux disto, Christian's option 2 works, but > > > dependencies are also set into the distro's package. In Debian, > > > dependencies are listed in the control file (Build-Depends), and > > > libubootenv is always set. > > > > > > 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > > > for this patch. In this sense, James' patch seems correct to me, but > > > then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? > > It may be correct for Buildroot but then it's a downstream patch in > Buildroot rather than a patch for SWUpdate upstream, IMO. > The others (OE, distros, ...) do the build environment setup taking the > wanted SWUpdate configuration into account and then build SWUpdate > with all the dependencies for the wanted SWUpdate configuration > met. Can't Buildroot do this, too? Not really, buildroot doesn't select dependencies based on package configurations like that, it tends to tell packages what's available instead based on what's selected in buildroot. > > > > Well we never really hit the EBG issue since buildroot hasn't ever packaged > > efibootguard at all. I guess we probably should reintroduce the > > HAVE_LIBEBGENV variable in case we add efibootguard support in the > > future. > > You will run into the same problem as EFI Boot Guard integration works > alike. Eventually, we may probably also have others like zchunk or rsync > run-time configurable and then we have the problem again. > So, ideally, the HAVE_ options should be set depending on the SWUpdate > options chosen and the available libraries on the build/target system at > build-time. If they don't match, it doesn't build. > This would also solve your problem I guess. The purpose of the HAVE_ options is so that buildroot can tell swupdate what options swupdate can enable when running swupdate-menuconfig inside of buildroot and so that swupdate's menuconfig can say what is missing when being run by the user using buildroot's configure system. > > > > > > (1) Port those external type definition to the bootloader binding > > > > implementations in SWUpdate (need to be maintained!) or > > > > > > We duplicate code and we go into trouble if the libraries are changing > > > the API. > > Right, and it's work on SWUpdate's side that can be avoided, agreed. > > > > For buildroot runtime and compile time libraries should always be the > > same as we don't really support binary package installation. > > > > I think it may make sense to have an option to disable dlopen library > > loading entirely as this feature only seems useful for systems that > > support binary package installation. > > Exactly this was the purpose: Enable bootloader selection at run-time by > configuration rather than statically at compile-time. This actually > enables SWUpdate to be shipped as a package by (binary) distributions > with probably support for multiple bootloaders enabled. > Then, you have to prepare an SWUpdate package per architecture and just > configure it to use the right bootloader (out of those whose support is > enabled). I'm not saying it's not useful for some cases, but as embedded systems that use swupdate often handle updates simply by replacing the entire rootfs it's likely not something needed in a lot of setups as they will only be updating swupdate by replacing the rootfs and not individually. > > If you disable this feature, then you're back at specifically building > SWUpdate packages for each architecture + target combination. > While this may work (better?) for Buildroot, it destroys the (binary) > distribution use case, e.g., Debian. I'm just suggesting a config option to allow it to be built with dlopen library loading disabled. Buildroot isn't a binary distribution so it's just added complexity and may potentially allow say missing library issues to go undetected for longer. > > Even so, with the implemented probing, you do have the same run-time and > compile-time libraries: They're just loaded differently (dlopen() vs. > ldlinux). If you chose at compile-time to just use U-Boot, then only > U-Boot support is enabled and is the default, i.e., libubootenv is > dlopen()'d on SWUpdate startup. > > So I do not see a benefit in introducing an option to disable this > feature. Did I overlook one? The issue I think is that ldlinux loading ensures that if the program runs all expected libraries are available while dlopen tends to be more likely to fail later, it's basically more brittle. Also static linking doesn't work with dlopen(although not sure swupdate really supports that properly in general). > > > > > > > (2) wire SWUpdate compilation to detected libraries on the build system > > > > (using e.g. pkgconf) ― this may help other such dependencies as well. > > > > > > This works on some build system, of course Linux Distros (but it is not > > > used in current Debian Package). > > Yes, some do, some don't and use other mechanisms but it's an option. > Anyway, we probably don't want to mandate using it. It was meant as an > example of how SWUpdate can detect whether the required dependencies are > installed and bailing out otherwise with a probably more "friendly" > error message ;) > > This is for build-time on the build system. On the target system you > have to install the required dependencies, too. So you need some > machinery to keep build- and run-time dependencies in sync. Yeah, for buildroot we kind of have to use something like the env variables since library availability is determined effectively entirely by the buildroot configuration that swupdate is being built inside of. > > > > I don't think this really works with buildroot either as we want to > > pass the available libraries at configure time (available libraries > > selected in buildroot's configuration which is also kconfig based) > > while using something like pkgconf would not work as the packages are > > not actually being built when configuring. > > Hm, I don't get this point: You have to compile the selected dependencies > of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf > entries which you can then query for auto-detecting the dependencies of > SWUpdate, can't you? The "make swupdate-menuconfig" in buildroot which invokes swupdate's own menuconfig happens prior to building any packages and thus pkgconf detection won't actually work at that stage. We only compile the dependencies after both swupdate and buildroot are configured. > > > > > My point here (apart having the same behavior for all bootloaders, that > > > is EBG, too) is if this patch has drawbacks that I do not understand / > > > see, because these HAVE_ were explicitely removed with the commit above. > > They were removed since you select to compile in support for bootloaders > at run-time. This doesn't necessarily mean that there actually is the > proper library on the target system. So you have to configure it on the > target to chose the bootloader for which (1) SWUpdate has support > compiled in and (2) whose support library is actually installed. Yeah, in our case though our build configuration matches the runtime in effectively all use cases as we don't build binary package artifacts. > > Relying on the HAVE_ options all turned on means you always build > support for all bootloaders ― which is not what you may want? We still allow the user to configure the options, we just use the HAVE_ variables to restrict what can be configured. > > > > > > Currently, if you tick an option you have to make sure that the required > > > > dependencies are satisfied or compilation will fail. > > > > I do not really have a (strong) opinion on this.... > > > > > Kind regards, > Christian > > -- > Dr. Christian Storm > Siemens AG, Technology, T CED SES-DE > Otto-Hahn-Ring 6, 81739 München, Germany > > -- > You received this message because you are subscribed to the Google Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20221202101619.pbpcdwf2myje2hmk%40cosmos.
Hi, > > > > >>>> Allow passing through the environment if libubootenv is available > > > > >>>> for the build target. > > > > >>>> > > > > >>>> Defaults to yes because we do not do any library checking for now. > > > > >>>> > > > > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > > >>>> --- > > > > >>>> Kconfig | 4 ++++ > > > > >>>> Makefile.deps | 4 ++++ > > > > >>>> bootloader/Config.in | 4 ++++ > > > > >>>> 3 files changed, 12 insertions(+) > > > > >>>> > > > > >>>> diff --git a/Kconfig b/Kconfig > > > > >>>> index 85fa5fd..bcea48b 100644 > > > > >>>> --- a/Kconfig > > > > >>>> +++ b/Kconfig > > > > >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > > > >>>> bool > > > > >>>> option env="HAVE_LIBUBI" > > > > >>>> > > > > >>>> +config HAVE_LIBUBOOTENV > > > > >>>> + bool > > > > >>>> + option env="HAVE_LIBUBOOTENV" > > > > >>>> + > > > > >>>> config HAVE_LIBEXT2FS > > > > >>>> bool > > > > >>>> option env="HAVE_LIBEXT2FS" > > > > >>>> diff --git a/Makefile.deps b/Makefile.deps > > > > >>>> index 08df4e2..97ac96e 100644 > > > > >>>> --- a/Makefile.deps > > > > >>>> +++ b/Makefile.deps > > > > >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > > > >>>> export HAVE_LIBUBI = y > > > > >>>> endif > > > > >>>> > > > > >>>> +ifeq ($(HAVE_LIBUBOOTENV),) > > > > >>>> +export HAVE_LIBUBOOTENV = y > > > > >>>> +endif > > > > >>>> + > > > > >>> > > > > >>> Your changes were already in SWUpdate but they were explicitely removed > > > > >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > > > > >>> the bootloader interface can be dinamically loaded (only at startup). > > > > >>> You want to bring them back, so I see at least a conflict with > > > > >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > > > > >> > > > > >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > > > > >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > > > > >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > > > > >> > > > > > > > > Yes, but the same is for EBG: > > > > > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > > > > > > > > and this is not treated in your patch. > > > > > > > > >> The purpose of this is to prevent a build failure due to that missing header. > > > > > > > > > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > > > > > struct libuboot definition w.r.t. the types used therein, see > > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > > > > > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > > > > > there's just the dependency on the type definitions. > > > > > The same is true for EFI Boot Guard as well by the way. > > > > > > > > > > So, I do see the following options: > > > > > > > > I see several cases depending on the build systems. Probably, we should > > > > analyze separately. James is fixing inside Buildroot. > > > > > > > > 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > > > > CONFIG_UBOOT is set, libubootenv is automatically added to the list of > > > > dependencies. > > > > > > > > 2 - for Debian / Linux disto, Christian's option 2 works, but > > > > dependencies are also set into the distro's package. In Debian, > > > > dependencies are listed in the control file (Build-Depends), and > > > > libubootenv is always set. > > > > > > > > 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > > > > for this patch. In this sense, James' patch seems correct to me, but > > > > then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? > > > > It may be correct for Buildroot but then it's a downstream patch in > > Buildroot rather than a patch for SWUpdate upstream, IMO. > > The others (OE, distros, ...) do the build environment setup taking the > > wanted SWUpdate configuration into account and then build SWUpdate > > with all the dependencies for the wanted SWUpdate configuration > > met. Can't Buildroot do this, too? > > Not really, buildroot doesn't select dependencies based on package > configurations like that, it tends to tell packages what's available > instead based on what's selected in buildroot. You noted in the patch: "Defaults to yes because we do not do any library checking for now." With this patch you tell SWUpdate libubootenv is available, unconditionally, which might as well not be the case, right? Then, you tell SWUpdate it's available while it is not? [see bottom comment first] > > > Well we never really hit the EBG issue since buildroot hasn't ever packaged > > > efibootguard at all. I guess we probably should reintroduce the > > > HAVE_LIBEBGENV variable in case we add efibootguard support in the > > > future. > > > > You will run into the same problem as EFI Boot Guard integration works > > alike. Eventually, we may probably also have others like zchunk or rsync > > run-time configurable and then we have the problem again. > > So, ideally, the HAVE_ options should be set depending on the SWUpdate > > options chosen and the available libraries on the build/target system at > > build-time. If they don't match, it doesn't build. > > This would also solve your problem I guess. > > The purpose of the HAVE_ options is so that buildroot can tell swupdate > what options swupdate can enable when running swupdate-menuconfig > inside of buildroot and so that swupdate's menuconfig can say what is > missing when being run by the user using buildroot's configure system. OK, but then ― if I got this correctly ― you need to have some sort of library checking in SWUpdate to match SWUpdate's view to Buildroot's? If enabled unconditionally, SWUpdate will present the libubootenv option, always, without ever noting what's missing? [see bottom comment first] > > > > > (1) Port those external type definition to the bootloader binding > > > > > implementations in SWUpdate (need to be maintained!) or > > > > > > > > We duplicate code and we go into trouble if the libraries are changing > > > > the API. > > > > Right, and it's work on SWUpdate's side that can be avoided, agreed. > > > > > > > For buildroot runtime and compile time libraries should always be the > > > same as we don't really support binary package installation. > > > > > > I think it may make sense to have an option to disable dlopen library > > > loading entirely as this feature only seems useful for systems that > > > support binary package installation. > > > > Exactly this was the purpose: Enable bootloader selection at run-time by > > configuration rather than statically at compile-time. This actually > > enables SWUpdate to be shipped as a package by (binary) distributions > > with probably support for multiple bootloaders enabled. > > Then, you have to prepare an SWUpdate package per architecture and just > > configure it to use the right bootloader (out of those whose support is > > enabled). > > I'm not saying it's not useful for some cases, but as embedded systems > that use swupdate often handle updates simply by replacing the entire > rootfs it's likely not something needed in a lot of setups as they will > only be updating swupdate by replacing the rootfs and not individually. Yes, but it's not that uncommon to build the rootfs with a meta-distribution based on binary packages, e.g., ELBE, ISAR, ..., for good reasons. For (binary) distributions, if compile-time tying the bootloader to the SWUpdate binary, you would have to supply binary packages for all combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature, you just have to make *one* package swupdate-<arch>.pkg and configure the bootloader to use. It's a big win for (binary) distributions with respect to maintainability, testing efforts, etc. at IMO no cost for the "classic" embedded use cases. We do exactly this for many products: Use a binary meta-distribution and replace the rootfs in an A/B deployment. > > If you disable this feature, then you're back at specifically building > > SWUpdate packages for each architecture + target combination. > > While this may work (better?) for Buildroot, it destroys the (binary) > > distribution use case, e.g., Debian. > > I'm just suggesting a config option to allow it to be built with dlopen > library loading disabled. Buildroot isn't a binary distribution so it's > just added complexity and may potentially allow say missing library > issues to go undetected for longer. Hm, this would then require maintaining two bindings to each bootloader: one for the "static" linkage and one for dlopen(). If Buildroot tells SWUpdate what's available (and installs this to the target) and SWUpdate would only use what Buildroot told it to use, then I don't see a problem? > > Even so, with the implemented probing, you do have the same run-time and > > compile-time libraries: They're just loaded differently (dlopen() vs. > > ldlinux). If you chose at compile-time to just use U-Boot, then only > > U-Boot support is enabled and is the default, i.e., libubootenv is > > dlopen()'d on SWUpdate startup. > > > > So I do not see a benefit in introducing an option to disable this > > feature. Did I overlook one? > > The issue I think is that ldlinux loading ensures that if the program runs > all expected libraries are available while dlopen tends to be more likely > to fail later, it's basically more brittle. In principle yes, and I would call this a feature for other use cases ;) Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries quite early in the startup of SWUpdate. So, if it fails, it fails early upfront and not while first using the bootloader functionality. This was very important to get similar fail-early behavior like when using ldlinux. Granted, now it fails a bit later but still prior to SWUpdate normal operation, i.e., while SWUpdate's initialization, and even gives you a nice(r) error message ;) For example, this is what I get when I move libebgenv.so.0 out of the way: [TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders: [TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded. [TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded. [TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found. [ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded. [INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present. [INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>. > Also static linking doesn't work with dlopen(although not sure swupdate > really supports that properly in general). Well, at least not without some extra work (post-processing the ELFs which is possible in this case as they're not random run-time plugins but defined bindings) or losing the benefits you're after when statically linking by allowing the "cruft" to creep in via loadable modules again: You have to initialize ld.so for libdl to properly execute dlopen() and this without symbol clashes. It does work but it's an overly ugly hack just for proofing the concept. > > > > > (2) wire SWUpdate compilation to detected libraries on the build system > > > > > (using e.g. pkgconf) ― this may help other such dependencies as well. > > > > > > > > This works on some build system, of course Linux Distros (but it is not > > > > used in current Debian Package). > > > > Yes, some do, some don't and use other mechanisms but it's an option. > > Anyway, we probably don't want to mandate using it. It was meant as an > > example of how SWUpdate can detect whether the required dependencies are > > installed and bailing out otherwise with a probably more "friendly" > > error message ;) > > > > This is for build-time on the build system. On the target system you > > have to install the required dependencies, too. So you need some > > machinery to keep build- and run-time dependencies in sync. > > Yeah, for buildroot we kind of have to use something like the env variables > since library availability is determined effectively entirely by the buildroot > configuration that swupdate is being built inside of. > > > > I don't think this really works with buildroot either as we want to > > > pass the available libraries at configure time (available libraries > > > selected in buildroot's configuration which is also kconfig based) > > > while using something like pkgconf would not work as the packages are > > > not actually being built when configuring. > > > > Hm, I don't get this point: You have to compile the selected dependencies > > of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf > > entries which you can then query for auto-detecting the dependencies of > > SWUpdate, can't you? > > The "make swupdate-menuconfig" in buildroot which invokes swupdate's > own menuconfig happens prior to building any packages and thus pkgconf > detection won't actually work at that stage. We only compile the dependencies > after both swupdate and buildroot are configured. OK, so you kind of "mimic" something like pkgconf by the ENV_ variables which define the available and to be built libraries at the later build stage. Hence, you cannot detect while SWUpdate menuconfig whether a particular library is/will be present or not. Hence, you set libubootenv and thereby convey to Buildroot (?) to install libubootenv at the later build stage. If so, then introducing ENV_libebgenv would also enforce to build/install libebgenv which might be unused as you've opted for using libubootenv? > > > > My point here (apart having the same behavior for all bootloaders, that > > > > is EBG, too) is if this patch has drawbacks that I do not understand / > > > > see, because these HAVE_ were explicitely removed with the commit above. > > > > They were removed since you select to compile in support for bootloaders > > at run-time. This doesn't necessarily mean that there actually is the > > proper library on the target system. So you have to configure it on the > > target to chose the bootloader for which (1) SWUpdate has support > > compiled in and (2) whose support library is actually installed. > > Yeah, in our case though our build configuration matches the runtime > in effectively all use cases as we don't build binary package artifacts. OK, as this is in sync, I think there's no problem with the dlopen() solution per se, right? > > Relying on the HAVE_ options all turned on means you always build > > support for all bootloaders ― which is not what you may want? > > We still allow the user to configure the options, we just use the HAVE_ > variables to restrict what can be configured. OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but only one is used, the one which is ticked in SWUpdate's menuconfig? Thanks for bearing with me getting Buildroot's inner workings straight in my mind! ;) Kind regards, Christian
On Mon, Dec 5, 2022 at 6:31 AM Christian Storm <christian.storm@siemens.com> wrote: > > Hi, > > > > > > >>>> Allow passing through the environment if libubootenv is available > > > > > >>>> for the build target. > > > > > >>>> > > > > > >>>> Defaults to yes because we do not do any library checking for now. > > > > > >>>> > > > > > >>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > > > > >>>> --- > > > > > >>>> Kconfig | 4 ++++ > > > > > >>>> Makefile.deps | 4 ++++ > > > > > >>>> bootloader/Config.in | 4 ++++ > > > > > >>>> 3 files changed, 12 insertions(+) > > > > > >>>> > > > > > >>>> diff --git a/Kconfig b/Kconfig > > > > > >>>> index 85fa5fd..bcea48b 100644 > > > > > >>>> --- a/Kconfig > > > > > >>>> +++ b/Kconfig > > > > > >>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > > > > > >>>> bool > > > > > >>>> option env="HAVE_LIBUBI" > > > > > >>>> > > > > > >>>> +config HAVE_LIBUBOOTENV > > > > > >>>> + bool > > > > > >>>> + option env="HAVE_LIBUBOOTENV" > > > > > >>>> + > > > > > >>>> config HAVE_LIBEXT2FS > > > > > >>>> bool > > > > > >>>> option env="HAVE_LIBEXT2FS" > > > > > >>>> diff --git a/Makefile.deps b/Makefile.deps > > > > > >>>> index 08df4e2..97ac96e 100644 > > > > > >>>> --- a/Makefile.deps > > > > > >>>> +++ b/Makefile.deps > > > > > >>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > > > > > >>>> export HAVE_LIBUBI = y > > > > > >>>> endif > > > > > >>>> > > > > > >>>> +ifeq ($(HAVE_LIBUBOOTENV),) > > > > > >>>> +export HAVE_LIBUBOOTENV = y > > > > > >>>> +endif > > > > > >>>> + > > > > > >>> > > > > > >>> Your changes were already in SWUpdate but they were explicitely removed > > > > > >>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > > > > > >>> the bootloader interface can be dinamically loaded (only at startup). > > > > > >>> You want to bring them back, so I see at least a conflict with > > > > > >>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > > > > > >> > > > > > >> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > > > > > >> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > > > > > >> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > > > > > >> > > > > > > > > > > Yes, but the same is for EBG: > > > > > > > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > > > > > > > > > > and this is not treated in your patch. > > > > > > > > > > >> The purpose of this is to prevent a build failure due to that missing header. > > > > > > > > > > > > CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > > > > > > struct libuboot definition w.r.t. the types used therein, see > > > > > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > > > > > > Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > > > > > > there's just the dependency on the type definitions. > > > > > > The same is true for EFI Boot Guard as well by the way. > > > > > > > > > > > > So, I do see the following options: > > > > > > > > > > I see several cases depending on the build systems. Probably, we should > > > > > analyze separately. James is fixing inside Buildroot. > > > > > > > > > > 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > > > > > CONFIG_UBOOT is set, libubootenv is automatically added to the list of > > > > > dependencies. > > > > > > > > > > 2 - for Debian / Linux disto, Christian's option 2 works, but > > > > > dependencies are also set into the distro's package. In Debian, > > > > > dependencies are listed in the control file (Build-Depends), and > > > > > libubootenv is always set. > > > > > > > > > > 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > > > > > for this patch. In this sense, James' patch seems correct to me, but > > > > > then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? > > > > > > It may be correct for Buildroot but then it's a downstream patch in > > > Buildroot rather than a patch for SWUpdate upstream, IMO. > > > The others (OE, distros, ...) do the build environment setup taking the > > > wanted SWUpdate configuration into account and then build SWUpdate > > > with all the dependencies for the wanted SWUpdate configuration > > > met. Can't Buildroot do this, too? > > > > Not really, buildroot doesn't select dependencies based on package > > configurations like that, it tends to tell packages what's available > > instead based on what's selected in buildroot. > > You noted in the patch: "Defaults to yes because we do not do any library > checking for now." > With this patch you tell SWUpdate libubootenv is available, unconditionally, > which might as well not be the case, right? Then, you tell SWUpdate it's > available while it is not? > [see bottom comment first] > > > > > > Well we never really hit the EBG issue since buildroot hasn't ever packaged > > > > efibootguard at all. I guess we probably should reintroduce the > > > > HAVE_LIBEBGENV variable in case we add efibootguard support in the > > > > future. > > > > > > You will run into the same problem as EFI Boot Guard integration works > > > alike. Eventually, we may probably also have others like zchunk or rsync > > > run-time configurable and then we have the problem again. > > > So, ideally, the HAVE_ options should be set depending on the SWUpdate > > > options chosen and the available libraries on the build/target system at > > > build-time. If they don't match, it doesn't build. > > > This would also solve your problem I guess. > > > > The purpose of the HAVE_ options is so that buildroot can tell swupdate > > what options swupdate can enable when running swupdate-menuconfig > > inside of buildroot and so that swupdate's menuconfig can say what is > > missing when being run by the user using buildroot's configure system. > > OK, but then ― if I got this correctly ― you need to have some sort of > library checking in SWUpdate to match SWUpdate's view to Buildroot's? > If enabled unconditionally, SWUpdate will present the libubootenv > option, always, without ever noting what's missing? > [see bottom comment first] Buildroot passes the env flags when invoking swupdate's menuconfig: https://github.com/buildroot/buildroot/blob/master/package/swupdate/swupdate.mk As these values are passed by buildroot swupdate doesn't need library checking beyond the env variable checking effectively. > > > > > > > > (1) Port those external type definition to the bootloader binding > > > > > > implementations in SWUpdate (need to be maintained!) or > > > > > > > > > > We duplicate code and we go into trouble if the libraries are changing > > > > > the API. > > > > > > Right, and it's work on SWUpdate's side that can be avoided, agreed. > > > > > > > > > > For buildroot runtime and compile time libraries should always be the > > > > same as we don't really support binary package installation. > > > > > > > > I think it may make sense to have an option to disable dlopen library > > > > loading entirely as this feature only seems useful for systems that > > > > support binary package installation. > > > > > > Exactly this was the purpose: Enable bootloader selection at run-time by > > > configuration rather than statically at compile-time. This actually > > > enables SWUpdate to be shipped as a package by (binary) distributions > > > with probably support for multiple bootloaders enabled. > > > Then, you have to prepare an SWUpdate package per architecture and just > > > configure it to use the right bootloader (out of those whose support is > > > enabled). > > > > I'm not saying it's not useful for some cases, but as embedded systems > > that use swupdate often handle updates simply by replacing the entire > > rootfs it's likely not something needed in a lot of setups as they will > > only be updating swupdate by replacing the rootfs and not individually. > > Yes, but it's not that uncommon to build the rootfs with a meta-distribution > based on binary packages, e.g., ELBE, ISAR, ..., for good reasons. > For (binary) distributions, if compile-time tying the bootloader to the > SWUpdate binary, you would have to supply binary packages for all > combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature, > you just have to make *one* package swupdate-<arch>.pkg and configure > the bootloader to use. It's a big win for (binary) distributions with > respect to maintainability, testing efforts, etc. at IMO no cost for the > "classic" embedded use cases. > We do exactly this for many products: Use a binary meta-distribution and > replace the rootfs in an A/B deployment. I mostly mean the dlopen part isn't likely to be useful for buildroot due to the lack of binary package support in general. Separate issue from multi-bootloader in some ways. > > > > > If you disable this feature, then you're back at specifically building > > > SWUpdate packages for each architecture + target combination. > > > While this may work (better?) for Buildroot, it destroys the (binary) > > > distribution use case, e.g., Debian. > > > > I'm just suggesting a config option to allow it to be built with dlopen > > library loading disabled. Buildroot isn't a binary distribution so it's > > just added complexity and may potentially allow say missing library > > issues to go undetected for longer. > > Hm, this would then require maintaining two bindings to each bootloader: > one for the "static" linkage and one for dlopen(). If Buildroot tells > SWUpdate what's available (and installs this to the target) and SWUpdate > would only use what Buildroot told it to use, then I don't see a problem? > > > > > Even so, with the implemented probing, you do have the same run-time and > > > compile-time libraries: They're just loaded differently (dlopen() vs. > > > ldlinux). If you chose at compile-time to just use U-Boot, then only > > > U-Boot support is enabled and is the default, i.e., libubootenv is > > > dlopen()'d on SWUpdate startup. > > > > > > So I do not see a benefit in introducing an option to disable this > > > feature. Did I overlook one? > > > > The issue I think is that ldlinux loading ensures that if the program runs > > all expected libraries are available while dlopen tends to be more likely > > to fail later, it's basically more brittle. > > In principle yes, and I would call this a feature for other use cases ;) > Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries > quite early in the startup of SWUpdate. So, if it fails, it fails early > upfront and not while first using the bootloader functionality. > This was very important to get similar fail-early behavior like when > using ldlinux. Granted, now it fails a bit later but still prior to > SWUpdate normal operation, i.e., while SWUpdate's initialization, and > even gives you a nice(r) error message ;) Ok, wasn't aware it would throw messages like that so I guess that's less of a concern. > > For example, this is what I get when I move libebgenv.so.0 out of the way: > [TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders: > [TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded. > [TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded. > [TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found. > [ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded. > [INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present. > [INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>. > > > > Also static linking doesn't work with dlopen(although not sure swupdate > > really supports that properly in general). > > Well, at least not without some extra work (post-processing the ELFs > which is possible in this case as they're not random run-time plugins > but defined bindings) or losing the benefits you're after when > statically linking by allowing the "cruft" to creep in via loadable > modules again: You have to initialize ld.so for libdl to properly > execute dlopen() and this without symbol clashes. It does work but > it's an overly ugly hack just for proofing the concept. I think static linking would be the main reason to support non-dlopen libraries here as static linking is still somewhat common on lower end boards. > > > > > > > > (2) wire SWUpdate compilation to detected libraries on the build system > > > > > > (using e.g. pkgconf) ― this may help other such dependencies as well. > > > > > > > > > > This works on some build system, of course Linux Distros (but it is not > > > > > used in current Debian Package). > > > > > > Yes, some do, some don't and use other mechanisms but it's an option. > > > Anyway, we probably don't want to mandate using it. It was meant as an > > > example of how SWUpdate can detect whether the required dependencies are > > > installed and bailing out otherwise with a probably more "friendly" > > > error message ;) > > > > > > This is for build-time on the build system. On the target system you > > > have to install the required dependencies, too. So you need some > > > machinery to keep build- and run-time dependencies in sync. > > > > Yeah, for buildroot we kind of have to use something like the env variables > > since library availability is determined effectively entirely by the buildroot > > configuration that swupdate is being built inside of. > > > > > > I don't think this really works with buildroot either as we want to > > > > pass the available libraries at configure time (available libraries > > > > selected in buildroot's configuration which is also kconfig based) > > > > while using something like pkgconf would not work as the packages are > > > > not actually being built when configuring. > > > > > > Hm, I don't get this point: You have to compile the selected dependencies > > > of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf > > > entries which you can then query for auto-detecting the dependencies of > > > SWUpdate, can't you? > > > > The "make swupdate-menuconfig" in buildroot which invokes swupdate's > > own menuconfig happens prior to building any packages and thus pkgconf > > detection won't actually work at that stage. We only compile the dependencies > > after both swupdate and buildroot are configured. > > OK, so you kind of "mimic" something like pkgconf by the ENV_ variables > which define the available and to be built libraries at the later build > stage. Hence, you cannot detect while SWUpdate menuconfig whether a > particular library is/will be present or not. Hence, you set libubootenv > and thereby convey to Buildroot (?) to install libubootenv at the later > build stage. If so, then introducing ENV_libebgenv would also enforce > to build/install libebgenv which might be unused as you've opted for > using libubootenv? Swupdate doesn't tell buildroot to install anything, buildroot tells swupdate what features it's allowed to use based on the buildroot config. > > > > > > > My point here (apart having the same behavior for all bootloaders, that > > > > > is EBG, too) is if this patch has drawbacks that I do not understand / > > > > > see, because these HAVE_ were explicitely removed with the commit above. > > > > > > They were removed since you select to compile in support for bootloaders > > > at run-time. This doesn't necessarily mean that there actually is the > > > proper library on the target system. So you have to configure it on the > > > target to chose the bootloader for which (1) SWUpdate has support > > > compiled in and (2) whose support library is actually installed. > > > > Yeah, in our case though our build configuration matches the runtime > > in effectively all use cases as we don't build binary package artifacts. > > OK, as this is in sync, I think there's no problem with the dlopen() > solution per se, right? Static linking being broken is probably the main issue. > > > > > Relying on the HAVE_ options all turned on means you always build > > > support for all bootloaders ― which is not what you may want? > > > > We still allow the user to configure the options, we just use the HAVE_ > > variables to restrict what can be configured. > > OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but > only one is used, the one which is ticked in SWUpdate's menuconfig? Well the HAVE variables are used to tell swupdate what it's allowed to configure, but the swupdate config still determines what is actually built in swupdate. > > > > Thanks for bearing with me getting Buildroot's inner workings straight > in my mind! ;) > > > > Kind regards, > Christian > > -- > Dr. Christian Storm > Siemens AG, Technology, T CED SES-DE > Otto-Hahn-Ring 6, 81739 München, Germany > > -- > You received this message because you are subscribed to the Google Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20221205103146.d4ibd34o5o443ovy%40cosmos.
Hi James, Christian, On 13.12.22 15:40, James Hilliard wrote: > On Mon, Dec 5, 2022 at 6:31 AM Christian Storm > <christian.storm@siemens.com> wrote: >> >> Hi, >> >>>>>>>>>> Allow passing through the environment if libubootenv is available >>>>>>>>>> for the build target. >>>>>>>>>> >>>>>>>>>> Defaults to yes because we do not do any library checking for now. >>>>>>>>>> >>>>>>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>>>>>>>> --- >>>>>>>>>> Kconfig | 4 ++++ >>>>>>>>>> Makefile.deps | 4 ++++ >>>>>>>>>> bootloader/Config.in | 4 ++++ >>>>>>>>>> 3 files changed, 12 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/Kconfig b/Kconfig >>>>>>>>>> index 85fa5fd..bcea48b 100644 >>>>>>>>>> --- a/Kconfig >>>>>>>>>> +++ b/Kconfig >>>>>>>>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI >>>>>>>>>> bool >>>>>>>>>> option env="HAVE_LIBUBI" >>>>>>>>>> >>>>>>>>>> +config HAVE_LIBUBOOTENV >>>>>>>>>> + bool >>>>>>>>>> + option env="HAVE_LIBUBOOTENV" >>>>>>>>>> + >>>>>>>>>> config HAVE_LIBEXT2FS >>>>>>>>>> bool >>>>>>>>>> option env="HAVE_LIBEXT2FS" >>>>>>>>>> diff --git a/Makefile.deps b/Makefile.deps >>>>>>>>>> index 08df4e2..97ac96e 100644 >>>>>>>>>> --- a/Makefile.deps >>>>>>>>>> +++ b/Makefile.deps >>>>>>>>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) >>>>>>>>>> export HAVE_LIBUBI = y >>>>>>>>>> endif >>>>>>>>>> >>>>>>>>>> +ifeq ($(HAVE_LIBUBOOTENV),) >>>>>>>>>> +export HAVE_LIBUBOOTENV = y >>>>>>>>>> +endif >>>>>>>>>> + >>>>>>>>> >>>>>>>>> Your changes were already in SWUpdate but they were explicitely removed >>>>>>>>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and >>>>>>>>> the bootloader interface can be dinamically loaded (only at startup). >>>>>>>>> You want to bring them back, so I see at least a conflict with >>>>>>>>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? >>>>>>>> >>>>>>>> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? >>>>>>>> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 >>>>>>>> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb >>>>>>>> >>>>>> >>>>>> Yes, but the same is for EBG: >>>>>> >>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 >>>>>> >>>>>> and this is not treated in your patch. >>>>>> >>>>>>>> The purpose of this is to prevent a build failure due to that missing header. >>>>>>> >>>>>>> CONFIG_UBOOT depends on libuboot.h from libubootenv for proper >>>>>>> struct libuboot definition w.r.t. the types used therein, see >>>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 >>>>>>> Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning >>>>>>> there's just the dependency on the type definitions. >>>>>>> The same is true for EFI Boot Guard as well by the way. >>>>>>> >>>>>>> So, I do see the following options: >>>>>> >>>>>> I see several cases depending on the build systems. Probably, we should >>>>>> analyze separately. James is fixing inside Buildroot. >>>>>> >>>>>> 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If >>>>>> CONFIG_UBOOT is set, libubootenv is automatically added to the list of >>>>>> dependencies. >>>>>> >>>>>> 2 - for Debian / Linux disto, Christian's option 2 works, but >>>>>> dependencies are also set into the distro's package. In Debian, >>>>>> dependencies are listed in the control file (Build-Depends), and >>>>>> libubootenv is always set. >>>>>> >>>>>> 3 - Buildroot have always used the HAVE_*, so I guess this is the reason >>>>>> for this patch. In this sense, James' patch seems correct to me, but >>>>>> then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? >>>> >>>> It may be correct for Buildroot but then it's a downstream patch in >>>> Buildroot rather than a patch for SWUpdate upstream, IMO. >>>> The others (OE, distros, ...) do the build environment setup taking the >>>> wanted SWUpdate configuration into account and then build SWUpdate >>>> with all the dependencies for the wanted SWUpdate configuration >>>> met. Can't Buildroot do this, too? >>> >>> Not really, buildroot doesn't select dependencies based on package >>> configurations like that, it tends to tell packages what's available >>> instead based on what's selected in buildroot. >> >> You noted in the patch: "Defaults to yes because we do not do any library >> checking for now." >> With this patch you tell SWUpdate libubootenv is available, unconditionally, >> which might as well not be the case, right? Then, you tell SWUpdate it's >> available while it is not? >> [see bottom comment first] >> >> >>>>> Well we never really hit the EBG issue since buildroot hasn't ever packaged >>>>> efibootguard at all. I guess we probably should reintroduce the >>>>> HAVE_LIBEBGENV variable in case we add efibootguard support in the >>>>> future. >>>> >>>> You will run into the same problem as EFI Boot Guard integration works >>>> alike. Eventually, we may probably also have others like zchunk or rsync >>>> run-time configurable and then we have the problem again. >>>> So, ideally, the HAVE_ options should be set depending on the SWUpdate >>>> options chosen and the available libraries on the build/target system at >>>> build-time. If they don't match, it doesn't build. >>>> This would also solve your problem I guess. >>> >>> The purpose of the HAVE_ options is so that buildroot can tell swupdate >>> what options swupdate can enable when running swupdate-menuconfig >>> inside of buildroot and so that swupdate's menuconfig can say what is >>> missing when being run by the user using buildroot's configure system. >> >> OK, but then ― if I got this correctly ― you need to have some sort of >> library checking in SWUpdate to match SWUpdate's view to Buildroot's? >> If enabled unconditionally, SWUpdate will present the libubootenv >> option, always, without ever noting what's missing? >> [see bottom comment first] > > Buildroot passes the env flags when invoking swupdate's menuconfig: > https://github.com/buildroot/buildroot/blob/master/package/swupdate/swupdate.mk > > As these values are passed by buildroot swupdate doesn't need library > checking beyond the env variable checking effectively. Right - I will just add that the HAVE_ configuration was introduced to support Buildroot. Neither Yocto nor Debian needs this, because they have their own mechanism (via packageconfig or DEPENDS in recipe). In both Yocto and Buildroot, we do not need some sort of library checking: the packages in Buildroot are strict defined, and recipe in Yocto reads SWUpdate configuration to set dependency to the bootloader library. To sumarize the history: dynamic loading is important for standard linux distros, else it is difficult to create a SWUpdate package (.deb) that works in any condition. > >> >> >>>>>>> (1) Port those external type definition to the bootloader binding >>>>>>> implementations in SWUpdate (need to be maintained!) or >>>>>> >>>>>> We duplicate code and we go into trouble if the libraries are changing >>>>>> the API. >>>> >>>> Right, and it's work on SWUpdate's side that can be avoided, agreed. >>>> >>>> >>>>> For buildroot runtime and compile time libraries should always be the >>>>> same as we don't really support binary package installation. >>>>> >>>>> I think it may make sense to have an option to disable dlopen library >>>>> loading entirely as this feature only seems useful for systems that >>>>> support binary package installation. >>>> >>>> Exactly this was the purpose: Enable bootloader selection at run-time by >>>> configuration rather than statically at compile-time. This actually >>>> enables SWUpdate to be shipped as a package by (binary) distributions >>>> with probably support for multiple bootloaders enabled. >>>> Then, you have to prepare an SWUpdate package per architecture and just >>>> configure it to use the right bootloader (out of those whose support is >>>> enabled). >>> >>> I'm not saying it's not useful for some cases, but as embedded systems >>> that use swupdate often handle updates simply by replacing the entire >>> rootfs it's likely not something needed in a lot of setups as they will >>> only be updating swupdate by replacing the rootfs and not individually. >> >> Yes, but it's not that uncommon to build the rootfs with a meta-distribution >> based on binary packages, e.g., ELBE, ISAR, ..., for good reasons. I think we completely agree we have different use cases :-). My point is just to understand if re-adding HAVE_<bootloader>, that we had in the past, can have some drawbacks and damage the use cases with distros, where dlopen() is strictly required. In case of Buildroot /Yocto, well, SWUpdate will still dlopen(), but just one library is built and available. >> For (binary) distributions, if compile-time tying the bootloader to the >> SWUpdate binary, you would have to supply binary packages for all >> combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature, >> you just have to make *one* package swupdate-<arch>.pkg and configure >> the bootloader to use. It's a big win for (binary) distributions with >> respect to maintainability, testing efforts, etc. Absolutely, it was added for binary distributions. But Buildroot / Yocto are building from sources. > at IMO no cost for the >> "classic" embedded use cases. >> We do exactly this for many products: Use a binary meta-distribution and >> replace the rootfs in an A/B deployment. > > I mostly mean the dlopen part isn't likely to be useful for buildroot due > to the lack of binary package support in general. Separate issue from > multi-bootloader in some ways. It is not useful, like for Yocto, but it should not have drawbacks if the link is done at the startup instead of static linking. > >> >> >>>> If you disable this feature, then you're back at specifically building >>>> SWUpdate packages for each architecture + target combination. >>>> While this may work (better?) for Buildroot, it destroys the (binary) >>>> distribution use case, e.g., Debian. >>> >>> I'm just suggesting a config option to allow it to be built with dlopen >>> library loading disabled. Buildroot isn't a binary distribution so it's >>> just added complexity and may potentially allow say missing library >>> issues to go undetected for longer. The reason for dlopen() is to have the same mechanism for all use cases (binary distros + built from sources). In Yocto, I do not see any evident disadvantage, because I force the dependency at build time. Which is the disadvantage with Buildroot ? libubootenv should be still be set in your board defconfig to make it working, and if libubootenv is in your rootfs, it should be the same if linking is done when SWUpdate starts. >> >> Hm, this would then require maintaining two bindings to each bootloader: >> one for the "static" linkage and one for dlopen(). Right, and goal was to have a solution for all. > If Buildroot tells >> SWUpdate what's available (and installs this to the target) and SWUpdate >> would only use what Buildroot told it to use, then I don't see a problem? I am not seeing the problem, too - so I like to understand what I am missing. >> >> >>>> Even so, with the implemented probing, you do have the same run-time and >>>> compile-time libraries: They're just loaded differently (dlopen() vs. >>>> ldlinux). If you chose at compile-time to just use U-Boot, then only >>>> U-Boot support is enabled and is the default, i.e., libubootenv is >>>> dlopen()'d on SWUpdate startup. >>>> >>>> So I do not see a benefit in introducing an option to disable this >>>> feature. Did I overlook one? I will also prefer to avoid this if it is not really strictly required - that means, Buildroot support is broken and there are no other way to fix it. >>> >>> The issue I think is that ldlinux loading ensures that if the program runs >>> all expected libraries are available while dlopen tends to be more likely >>> to fail later, it's basically more brittle. >> >> In principle yes, and I would call this a feature for other use cases ;) >> Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries >> quite early in the startup of SWUpdate. So, if it fails, it fails early >> upfront and not while first using the bootloader functionality. >> This was very important to get similar fail-early behavior like when >> using ldlinux. Granted, now it fails a bit later but still prior to >> SWUpdate normal operation, i.e., while SWUpdate's initialization, and >> even gives you a nice(r) error message ;) > > Ok, wasn't aware it would throw messages like that so I guess that's less > of a concern. Right, SWUpdate should not be started if the required library is not loaded. This was a must. > >> >> For example, this is what I get when I move libebgenv.so.0 out of the way: >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders: >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded. >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded. >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found. >> [ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded. >> [INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present. >> [INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>. >> >> >>> Also static linking doesn't work with dlopen(although not sure swupdate >>> really supports that properly in general). >> >> Well, at least not without some extra work (post-processing the ELFs >> which is possible in this case as they're not random run-time plugins >> but defined bindings) or losing the benefits you're after when >> statically linking by allowing the "cruft" to creep in via loadable >> modules again: You have to initialize ld.so for libdl to properly >> execute dlopen() and this without symbol clashes. It does work but >> it's an overly ugly hack just for proofing the concept. > > I think static linking would be the main reason to support non-dlopen > libraries here as static linking is still somewhat common on lower end > boards. I fully agree that we cannot have a running SWUpdate and later during an update, SWUpdate reports that a library cannot be found. This is not acceptable. > >> >> >>>>>>> (2) wire SWUpdate compilation to detected libraries on the build system >>>>>>> (using e.g. pkgconf) ― this may help other such dependencies as well. >>>>>> >>>>>> This works on some build system, of course Linux Distros (but it is not >>>>>> used in current Debian Package). >>>> >>>> Yes, some do, some don't and use other mechanisms but it's an option. >>>> Anyway, we probably don't want to mandate using it. It was meant as an >>>> example of how SWUpdate can detect whether the required dependencies are >>>> installed and bailing out otherwise with a probably more "friendly" >>>> error message ;) >>>> >>>> This is for build-time on the build system. On the target system you >>>> have to install the required dependencies, too. So you need some >>>> machinery to keep build- and run-time dependencies in sync. >>> >>> Yeah, for buildroot we kind of have to use something like the env variables >>> since library availability is determined effectively entirely by the buildroot >>> configuration that swupdate is being built inside of. >>> >>>>> I don't think this really works with buildroot either as we want to >>>>> pass the available libraries at configure time (available libraries >>>>> selected in buildroot's configuration which is also kconfig based) >>>>> while using something like pkgconf would not work as the packages are >>>>> not actually being built when configuring. >>>> >>>> Hm, I don't get this point: You have to compile the selected dependencies >>>> of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf >>>> entries which you can then query for auto-detecting the dependencies of >>>> SWUpdate, can't you? >>> >>> The "make swupdate-menuconfig" in buildroot which invokes swupdate's >>> own menuconfig happens prior to building any packages and thus pkgconf >>> detection won't actually work at that stage. We only compile the dependencies >>> after both swupdate and buildroot are configured. >> >> OK, so you kind of "mimic" something like pkgconf by the ENV_ variables >> which define the available and to be built libraries at the later build >> stage. Hence, you cannot detect while SWUpdate menuconfig whether a >> particular library is/will be present or not. Hence, you set libubootenv >> and thereby convey to Buildroot (?) to install libubootenv at the later >> build stage. If so, then introducing ENV_libebgenv would also enforce >> to build/install libebgenv which might be unused as you've opted for >> using libubootenv? > > Swupdate doesn't tell buildroot to install anything, buildroot tells swupdate > what features it's allowed to use based on the buildroot config. That's right, it works in the other way. > >> >> >>>>>> My point here (apart having the same behavior for all bootloaders, that >>>>>> is EBG, too) is if this patch has drawbacks that I do not understand / >>>>>> see, because these HAVE_ were explicitely removed with the commit above. >>>> >>>> They were removed since you select to compile in support for bootloaders >>>> at run-time. This doesn't necessarily mean that there actually is the >>>> proper library on the target system. So you have to configure it on the >>>> target to chose the bootloader for which (1) SWUpdate has support >>>> compiled in and (2) whose support library is actually installed. >>> >>> Yeah, in our case though our build configuration matches the runtime >>> in effectively all use cases as we don't build binary package artifacts. >> >> OK, as this is in sync, I think there's no problem with the dlopen() >> solution per se, right? > > Static linking being broken is probably the main issue. Does it mean that we agree that there should not be problem with dlopen() per se, right ? Buildroot (like Yocto) will just have one possible library, Debian-Like could choose at startup which library should be linked, but we can support both of them, right ? > >> >> >>>> Relying on the HAVE_ options all turned on means you always build >>>> support for all bootloaders ― which is not what you may want? >>> >>> We still allow the user to configure the options, we just use the HAVE_ >>> variables to restrict what can be configured. >> >> OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but >> only one is used, the one which is ticked in SWUpdate's menuconfig? > > Well the HAVE variables are used to tell swupdate what it's allowed to > configure, Right. > but the swupdate config still determines what is actually built in swupdate. Correct. Do we have a deal about what should be done ? Can we put again HAVE_ (used by Buildroot, ignored by other use cases) ? Any open issue having dlopen() in Buildroot, even if, let's say, it is quite useless ? Best regards, Stefano
Hi, > [...] > Do we have a deal about what should be done ? Can we put again HAVE_ (used by > Buildroot, ignored by other use cases) ? Any open issue having dlopen() in > Buildroot, even if, let's say, it is quite useless ? I do not see a (real) problem. So you will put in the HAVE_ for libubootenv as well as for EFI Boot Guard? Kind regards, Christian
On Thu, Dec 15, 2022 at 12:00 PM Stefano Babic <sbabic@denx.de> wrote: > > Hi James, Christian, > > On 13.12.22 15:40, James Hilliard wrote: > > On Mon, Dec 5, 2022 at 6:31 AM Christian Storm > > <christian.storm@siemens.com> wrote: > >> > >> Hi, > >> > >>>>>>>>>> Allow passing through the environment if libubootenv is available > >>>>>>>>>> for the build target. > >>>>>>>>>> > >>>>>>>>>> Defaults to yes because we do not do any library checking for now. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > >>>>>>>>>> --- > >>>>>>>>>> Kconfig | 4 ++++ > >>>>>>>>>> Makefile.deps | 4 ++++ > >>>>>>>>>> bootloader/Config.in | 4 ++++ > >>>>>>>>>> 3 files changed, 12 insertions(+) > >>>>>>>>>> > >>>>>>>>>> diff --git a/Kconfig b/Kconfig > >>>>>>>>>> index 85fa5fd..bcea48b 100644 > >>>>>>>>>> --- a/Kconfig > >>>>>>>>>> +++ b/Kconfig > >>>>>>>>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI > >>>>>>>>>> bool > >>>>>>>>>> option env="HAVE_LIBUBI" > >>>>>>>>>> > >>>>>>>>>> +config HAVE_LIBUBOOTENV > >>>>>>>>>> + bool > >>>>>>>>>> + option env="HAVE_LIBUBOOTENV" > >>>>>>>>>> + > >>>>>>>>>> config HAVE_LIBEXT2FS > >>>>>>>>>> bool > >>>>>>>>>> option env="HAVE_LIBEXT2FS" > >>>>>>>>>> diff --git a/Makefile.deps b/Makefile.deps > >>>>>>>>>> index 08df4e2..97ac96e 100644 > >>>>>>>>>> --- a/Makefile.deps > >>>>>>>>>> +++ b/Makefile.deps > >>>>>>>>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) > >>>>>>>>>> export HAVE_LIBUBI = y > >>>>>>>>>> endif > >>>>>>>>>> > >>>>>>>>>> +ifeq ($(HAVE_LIBUBOOTENV),) > >>>>>>>>>> +export HAVE_LIBUBOOTENV = y > >>>>>>>>>> +endif > >>>>>>>>>> + > >>>>>>>>> > >>>>>>>>> Your changes were already in SWUpdate but they were explicitely removed > >>>>>>>>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and > >>>>>>>>> the bootloader interface can be dinamically loaded (only at startup). > >>>>>>>>> You want to bring them back, so I see at least a conflict with > >>>>>>>>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? > >>>>>>>> > >>>>>>>> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? > >>>>>>>> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 > >>>>>>>> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb > >>>>>>>> > >>>>>> > >>>>>> Yes, but the same is for EBG: > >>>>>> > >>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 > >>>>>> > >>>>>> and this is not treated in your patch. > >>>>>> > >>>>>>>> The purpose of this is to prevent a build failure due to that missing header. > >>>>>>> > >>>>>>> CONFIG_UBOOT depends on libuboot.h from libubootenv for proper > >>>>>>> struct libuboot definition w.r.t. the types used therein, see > >>>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 > >>>>>>> Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning > >>>>>>> there's just the dependency on the type definitions. > >>>>>>> The same is true for EFI Boot Guard as well by the way. > >>>>>>> > >>>>>>> So, I do see the following options: > >>>>>> > >>>>>> I see several cases depending on the build systems. Probably, we should > >>>>>> analyze separately. James is fixing inside Buildroot. > >>>>>> > >>>>>> 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If > >>>>>> CONFIG_UBOOT is set, libubootenv is automatically added to the list of > >>>>>> dependencies. > >>>>>> > >>>>>> 2 - for Debian / Linux disto, Christian's option 2 works, but > >>>>>> dependencies are also set into the distro's package. In Debian, > >>>>>> dependencies are listed in the control file (Build-Depends), and > >>>>>> libubootenv is always set. > >>>>>> > >>>>>> 3 - Buildroot have always used the HAVE_*, so I guess this is the reason > >>>>>> for this patch. In this sense, James' patch seems correct to me, but > >>>>>> then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? > >>>> > >>>> It may be correct for Buildroot but then it's a downstream patch in > >>>> Buildroot rather than a patch for SWUpdate upstream, IMO. > >>>> The others (OE, distros, ...) do the build environment setup taking the > >>>> wanted SWUpdate configuration into account and then build SWUpdate > >>>> with all the dependencies for the wanted SWUpdate configuration > >>>> met. Can't Buildroot do this, too? > >>> > >>> Not really, buildroot doesn't select dependencies based on package > >>> configurations like that, it tends to tell packages what's available > >>> instead based on what's selected in buildroot. > >> > >> You noted in the patch: "Defaults to yes because we do not do any library > >> checking for now." > >> With this patch you tell SWUpdate libubootenv is available, unconditionally, > >> which might as well not be the case, right? Then, you tell SWUpdate it's > >> available while it is not? > >> [see bottom comment first] > >> > >> > >>>>> Well we never really hit the EBG issue since buildroot hasn't ever packaged > >>>>> efibootguard at all. I guess we probably should reintroduce the > >>>>> HAVE_LIBEBGENV variable in case we add efibootguard support in the > >>>>> future. > >>>> > >>>> You will run into the same problem as EFI Boot Guard integration works > >>>> alike. Eventually, we may probably also have others like zchunk or rsync > >>>> run-time configurable and then we have the problem again. > >>>> So, ideally, the HAVE_ options should be set depending on the SWUpdate > >>>> options chosen and the available libraries on the build/target system at > >>>> build-time. If they don't match, it doesn't build. > >>>> This would also solve your problem I guess. > >>> > >>> The purpose of the HAVE_ options is so that buildroot can tell swupdate > >>> what options swupdate can enable when running swupdate-menuconfig > >>> inside of buildroot and so that swupdate's menuconfig can say what is > >>> missing when being run by the user using buildroot's configure system. > >> > >> OK, but then ― if I got this correctly ― you need to have some sort of > >> library checking in SWUpdate to match SWUpdate's view to Buildroot's? > >> If enabled unconditionally, SWUpdate will present the libubootenv > >> option, always, without ever noting what's missing? > >> [see bottom comment first] > > > > Buildroot passes the env flags when invoking swupdate's menuconfig: > > https://github.com/buildroot/buildroot/blob/master/package/swupdate/swupdate.mk > > > > As these values are passed by buildroot swupdate doesn't need library > > checking beyond the env variable checking effectively. > > Right - I will just add that the HAVE_ configuration was introduced to > support Buildroot. Neither Yocto nor Debian needs this, because they > have their own mechanism (via packageconfig or DEPENDS in recipe). > > In both Yocto and Buildroot, we do not need some sort of library > checking: the packages in Buildroot are strict defined, and recipe in > Yocto reads SWUpdate configuration to set dependency to the bootloader > library. Note that buildroot AFAIU isn't capable of adjusting package dependencies like yocto can. We use the HAVE_ variables so that the swupdate configuration is appropriately restricted based on the buildroot configuration. > > To sumarize the history: dynamic loading is important for standard linux > distros, else it is difficult to create a SWUpdate package (.deb) that > works in any condition. > > > > >> > >> > >>>>>>> (1) Port those external type definition to the bootloader binding > >>>>>>> implementations in SWUpdate (need to be maintained!) or > >>>>>> > >>>>>> We duplicate code and we go into trouble if the libraries are changing > >>>>>> the API. > >>>> > >>>> Right, and it's work on SWUpdate's side that can be avoided, agreed. > >>>> > >>>> > >>>>> For buildroot runtime and compile time libraries should always be the > >>>>> same as we don't really support binary package installation. > >>>>> > >>>>> I think it may make sense to have an option to disable dlopen library > >>>>> loading entirely as this feature only seems useful for systems that > >>>>> support binary package installation. > >>>> > >>>> Exactly this was the purpose: Enable bootloader selection at run-time by > >>>> configuration rather than statically at compile-time. This actually > >>>> enables SWUpdate to be shipped as a package by (binary) distributions > >>>> with probably support for multiple bootloaders enabled. > >>>> Then, you have to prepare an SWUpdate package per architecture and just > >>>> configure it to use the right bootloader (out of those whose support is > >>>> enabled). > >>> > >>> I'm not saying it's not useful for some cases, but as embedded systems > >>> that use swupdate often handle updates simply by replacing the entire > >>> rootfs it's likely not something needed in a lot of setups as they will > >>> only be updating swupdate by replacing the rootfs and not individually. > >> > >> Yes, but it's not that uncommon to build the rootfs with a meta-distribution > >> based on binary packages, e.g., ELBE, ISAR, ..., for good reasons. > > I think we completely agree we have different use cases :-). > > My point is just to understand if re-adding HAVE_<bootloader>, that we > had in the past, can have some drawbacks and damage the use cases with > distros, where dlopen() is strictly required. In case of Buildroot > /Yocto, well, SWUpdate will still dlopen(), but just one library is > built and available. It's mostly an issue for compile time dependencies I guess, buildroot passes HAVE_ variables based on compile time dependencies which just so happen to be the same as runtime dependencies in our case. > > >> For (binary) distributions, if compile-time tying the bootloader to the > >> SWUpdate binary, you would have to supply binary packages for all > >> combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature, > >> you just have to make *one* package swupdate-<arch>.pkg and configure > >> the bootloader to use. It's a big win for (binary) distributions with > >> respect to maintainability, testing efforts, etc. > > Absolutely, it was added for binary distributions. But Buildroot / Yocto > are building from sources. > > > at IMO no cost for the > >> "classic" embedded use cases. > >> We do exactly this for many products: Use a binary meta-distribution and > >> replace the rootfs in an A/B deployment. > > > > I mostly mean the dlopen part isn't likely to be useful for buildroot due > > to the lack of binary package support in general. Separate issue from > > multi-bootloader in some ways. > > It is not useful, like for Yocto, but it should not have drawbacks if > the link is done at the startup instead of static linking. > > > > >> > >> > >>>> If you disable this feature, then you're back at specifically building > >>>> SWUpdate packages for each architecture + target combination. > >>>> While this may work (better?) for Buildroot, it destroys the (binary) > >>>> distribution use case, e.g., Debian. > >>> > >>> I'm just suggesting a config option to allow it to be built with dlopen > >>> library loading disabled. Buildroot isn't a binary distribution so it's > >>> just added complexity and may potentially allow say missing library > >>> issues to go undetected for longer. > > The reason for dlopen() is to have the same mechanism for all use cases > (binary distros + built from sources). In Yocto, I do not see any > evident disadvantage, because I force the dependency at build time. > Which is the disadvantage with Buildroot ? libubootenv should be still > be set in your board defconfig to make it working, and if libubootenv is > in your rootfs, it should be the same if linking is done when SWUpdate > starts. > > >> > >> Hm, this would then require maintaining two bindings to each bootloader: > >> one for the "static" linkage and one for dlopen(). > > Right, and goal was to have a solution for all. > > > If Buildroot tells > >> SWUpdate what's available (and installs this to the target) and SWUpdate > >> would only use what Buildroot told it to use, then I don't see a problem? > > I am not seeing the problem, too - so I like to understand what I am > missing. > > >> > >> > >>>> Even so, with the implemented probing, you do have the same run-time and > >>>> compile-time libraries: They're just loaded differently (dlopen() vs. > >>>> ldlinux). If you chose at compile-time to just use U-Boot, then only > >>>> U-Boot support is enabled and is the default, i.e., libubootenv is > >>>> dlopen()'d on SWUpdate startup. > >>>> > >>>> So I do not see a benefit in introducing an option to disable this > >>>> feature. Did I overlook one? > > I will also prefer to avoid this if it is not really strictly required - > that means, Buildroot support is broken and there are no other way to > fix it. > > >>> > >>> The issue I think is that ldlinux loading ensures that if the program runs > >>> all expected libraries are available while dlopen tends to be more likely > >>> to fail later, it's basically more brittle. > >> > >> In principle yes, and I would call this a feature for other use cases ;) > >> Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries > >> quite early in the startup of SWUpdate. So, if it fails, it fails early > >> upfront and not while first using the bootloader functionality. > >> This was very important to get similar fail-early behavior like when > >> using ldlinux. Granted, now it fails a bit later but still prior to > >> SWUpdate normal operation, i.e., while SWUpdate's initialization, and > >> even gives you a nice(r) error message ;) > > > > Ok, wasn't aware it would throw messages like that so I guess that's less > > of a concern. > > Right, SWUpdate should not be started if the required library is not > loaded. This was a must. > > > > >> > >> For example, this is what I get when I move libebgenv.so.0 out of the way: > >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders: > >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded. > >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded. > >> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found. > >> [ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded. > >> [INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present. > >> [INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>. > >> > >> > >>> Also static linking doesn't work with dlopen(although not sure swupdate > >>> really supports that properly in general). > >> > >> Well, at least not without some extra work (post-processing the ELFs > >> which is possible in this case as they're not random run-time plugins > >> but defined bindings) or losing the benefits you're after when > >> statically linking by allowing the "cruft" to creep in via loadable > >> modules again: You have to initialize ld.so for libdl to properly > >> execute dlopen() and this without symbol clashes. It does work but > >> it's an overly ugly hack just for proofing the concept. > > > > I think static linking would be the main reason to support non-dlopen > > libraries here as static linking is still somewhat common on lower end > > boards. > > I fully agree that we cannot have a running SWUpdate and later during an > update, SWUpdate reports that a library cannot be found. This is not > acceptable. > > > > >> > >> > >>>>>>> (2) wire SWUpdate compilation to detected libraries on the build system > >>>>>>> (using e.g. pkgconf) ― this may help other such dependencies as well. > >>>>>> > >>>>>> This works on some build system, of course Linux Distros (but it is not > >>>>>> used in current Debian Package). > >>>> > >>>> Yes, some do, some don't and use other mechanisms but it's an option. > >>>> Anyway, we probably don't want to mandate using it. It was meant as an > >>>> example of how SWUpdate can detect whether the required dependencies are > >>>> installed and bailing out otherwise with a probably more "friendly" > >>>> error message ;) > >>>> > >>>> This is for build-time on the build system. On the target system you > >>>> have to install the required dependencies, too. So you need some > >>>> machinery to keep build- and run-time dependencies in sync. > >>> > >>> Yeah, for buildroot we kind of have to use something like the env variables > >>> since library availability is determined effectively entirely by the buildroot > >>> configuration that swupdate is being built inside of. > >>> > >>>>> I don't think this really works with buildroot either as we want to > >>>>> pass the available libraries at configure time (available libraries > >>>>> selected in buildroot's configuration which is also kconfig based) > >>>>> while using something like pkgconf would not work as the packages are > >>>>> not actually being built when configuring. > >>>> > >>>> Hm, I don't get this point: You have to compile the selected dependencies > >>>> of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf > >>>> entries which you can then query for auto-detecting the dependencies of > >>>> SWUpdate, can't you? > >>> > >>> The "make swupdate-menuconfig" in buildroot which invokes swupdate's > >>> own menuconfig happens prior to building any packages and thus pkgconf > >>> detection won't actually work at that stage. We only compile the dependencies > >>> after both swupdate and buildroot are configured. > >> > >> OK, so you kind of "mimic" something like pkgconf by the ENV_ variables > >> which define the available and to be built libraries at the later build > >> stage. Hence, you cannot detect while SWUpdate menuconfig whether a > >> particular library is/will be present or not. Hence, you set libubootenv > >> and thereby convey to Buildroot (?) to install libubootenv at the later > >> build stage. If so, then introducing ENV_libebgenv would also enforce > >> to build/install libebgenv which might be unused as you've opted for > >> using libubootenv? > > > > Swupdate doesn't tell buildroot to install anything, buildroot tells swupdate > > what features it's allowed to use based on the buildroot config. > > That's right, it works in the other way. > > > > >> > >> > >>>>>> My point here (apart having the same behavior for all bootloaders, that > >>>>>> is EBG, too) is if this patch has drawbacks that I do not understand / > >>>>>> see, because these HAVE_ were explicitely removed with the commit above. > >>>> > >>>> They were removed since you select to compile in support for bootloaders > >>>> at run-time. This doesn't necessarily mean that there actually is the > >>>> proper library on the target system. So you have to configure it on the > >>>> target to chose the bootloader for which (1) SWUpdate has support > >>>> compiled in and (2) whose support library is actually installed. > >>> > >>> Yeah, in our case though our build configuration matches the runtime > >>> in effectively all use cases as we don't build binary package artifacts. > >> > >> OK, as this is in sync, I think there's no problem with the dlopen() > >> solution per se, right? > > > > Static linking being broken is probably the main issue. > > Does it mean that we agree that there should not be problem with > dlopen() per se, right ? Buildroot (like Yocto) will just have one > possible library, Debian-Like could choose at startup which library > should be linked, but we can support both of them, right ? From my understanding dlopen() is entirely incompatible with pure static builds. These AFAIU would be non-glibc userspaces such as uclibc/musl. > > > > >> > >> > >>>> Relying on the HAVE_ options all turned on means you always build > >>>> support for all bootloaders ― which is not what you may want? > >>> > >>> We still allow the user to configure the options, we just use the HAVE_ > >>> variables to restrict what can be configured. > >> > >> OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but > >> only one is used, the one which is ticked in SWUpdate's menuconfig? > > > > Well the HAVE variables are used to tell swupdate what it's allowed to > > configure, > > Right. > > > but the swupdate config still determines what is actually built in swupdate. > > Correct. > > Do we have a deal about what should be done ? Can we put again HAVE_ > (used by Buildroot, ignored by other use cases) ? Any open issue having > dlopen() in Buildroot, even if, let's say, it is quite useless ? The HAVE_ variables should only restrict option selections when defined so I don't think they will cause issues for other use cases when not used. The dlopen() issue is more a matter of if we want to support fully static swupdate binaries(ie uclibc/musl fully static userspace builds). > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > =====================================================================
Hi James, On 04.01.23 00:52, James Hilliard wrote: > On Thu, Dec 15, 2022 at 12:00 PM Stefano Babic <sbabic@denx.de> wrote: >> >> Hi James, Christian, >> >> On 13.12.22 15:40, James Hilliard wrote: >>> On Mon, Dec 5, 2022 at 6:31 AM Christian Storm >>> <christian.storm@siemens.com> wrote: >>>> >>>> Hi, >>>> >>>>>>>>>>>> Allow passing through the environment if libubootenv is available >>>>>>>>>>>> for the build target. >>>>>>>>>>>> >>>>>>>>>>>> Defaults to yes because we do not do any library checking for now. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>>>>>>>>>>> --- >>>>>>>>>>>> Kconfig | 4 ++++ >>>>>>>>>>>> Makefile.deps | 4 ++++ >>>>>>>>>>>> bootloader/Config.in | 4 ++++ >>>>>>>>>>>> 3 files changed, 12 insertions(+) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/Kconfig b/Kconfig >>>>>>>>>>>> index 85fa5fd..bcea48b 100644 >>>>>>>>>>>> --- a/Kconfig >>>>>>>>>>>> +++ b/Kconfig >>>>>>>>>>>> @@ -53,6 +53,10 @@ config HAVE_LIBUBI >>>>>>>>>>>> bool >>>>>>>>>>>> option env="HAVE_LIBUBI" >>>>>>>>>>>> >>>>>>>>>>>> +config HAVE_LIBUBOOTENV >>>>>>>>>>>> + bool >>>>>>>>>>>> + option env="HAVE_LIBUBOOTENV" >>>>>>>>>>>> + >>>>>>>>>>>> config HAVE_LIBEXT2FS >>>>>>>>>>>> bool >>>>>>>>>>>> option env="HAVE_LIBEXT2FS" >>>>>>>>>>>> diff --git a/Makefile.deps b/Makefile.deps >>>>>>>>>>>> index 08df4e2..97ac96e 100644 >>>>>>>>>>>> --- a/Makefile.deps >>>>>>>>>>>> +++ b/Makefile.deps >>>>>>>>>>>> @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) >>>>>>>>>>>> export HAVE_LIBUBI = y >>>>>>>>>>>> endif >>>>>>>>>>>> >>>>>>>>>>>> +ifeq ($(HAVE_LIBUBOOTENV),) >>>>>>>>>>>> +export HAVE_LIBUBOOTENV = y >>>>>>>>>>>> +endif >>>>>>>>>>>> + >>>>>>>>>>> >>>>>>>>>>> Your changes were already in SWUpdate but they were explicitely removed >>>>>>>>>>> with Christian's commit b8897ed695e1cd954859142b14ec8546d2e7994a, and >>>>>>>>>>> the bootloader interface can be dinamically loaded (only at startup). >>>>>>>>>>> You want to bring them back, so I see at least a conflict with >>>>>>>>>>> Christian's work. Is it for you not enought if CONFIG_UBOOT is not set ? >>>>>>>>>> >>>>>>>>>> Doesn't CONFIG_UBOOT have a compile time dependency on libubootenv? >>>>>>>>>> https://github.com/sbabic/swupdate/blob/d61fd87c851a6728ff21248e70d4fd718f8b5865/bootloader/uboot.c#L23 >>>>>>>>>> https://github.com/buildroot/buildroot/commit/a11b36089b34dcf0c1b90835a436f50efec3d8fb >>>>>>>>>> >>>>>>>> >>>>>>>> Yes, but the same is for EBG: >>>>>>>> >>>>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L16 >>>>>>>> >>>>>>>> and this is not treated in your patch. >>>>>>>> >>>>>>>>>> The purpose of this is to prevent a build failure due to that missing header. >>>>>>>>> >>>>>>>>> CONFIG_UBOOT depends on libuboot.h from libubootenv for proper >>>>>>>>> struct libuboot definition w.r.t. the types used therein, see >>>>>>>>> https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L28-L38 >>>>>>>>> Then, it's dynamically loaded by dlopen("libubootenv.so.0"), meaning >>>>>>>>> there's just the dependency on the type definitions. >>>>>>>>> The same is true for EFI Boot Guard as well by the way. >>>>>>>>> >>>>>>>>> So, I do see the following options: >>>>>>>> >>>>>>>> I see several cases depending on the build systems. Probably, we should >>>>>>>> analyze separately. James is fixing inside Buildroot. >>>>>>>> >>>>>>>> 1 - for OE, DEPENDS is set after parsing SWUpdate's defconfig. If >>>>>>>> CONFIG_UBOOT is set, libubootenv is automatically added to the list of >>>>>>>> dependencies. >>>>>>>> >>>>>>>> 2 - for Debian / Linux disto, Christian's option 2 works, but >>>>>>>> dependencies are also set into the distro's package. In Debian, >>>>>>>> dependencies are listed in the control file (Build-Depends), and >>>>>>>> libubootenv is always set. >>>>>>>> >>>>>>>> 3 - Buildroot have always used the HAVE_*, so I guess this is the reason >>>>>>>> for this patch. In this sense, James' patch seems correct to me, but >>>>>>>> then why is EBG missing ? Shouldn't we reintroduce HAVE_LIBEBGENV as well ? >>>>>> >>>>>> It may be correct for Buildroot but then it's a downstream patch in >>>>>> Buildroot rather than a patch for SWUpdate upstream, IMO. >>>>>> The others (OE, distros, ...) do the build environment setup taking the >>>>>> wanted SWUpdate configuration into account and then build SWUpdate >>>>>> with all the dependencies for the wanted SWUpdate configuration >>>>>> met. Can't Buildroot do this, too? >>>>> >>>>> Not really, buildroot doesn't select dependencies based on package >>>>> configurations like that, it tends to tell packages what's available >>>>> instead based on what's selected in buildroot. >>>> >>>> You noted in the patch: "Defaults to yes because we do not do any library >>>> checking for now." >>>> With this patch you tell SWUpdate libubootenv is available, unconditionally, >>>> which might as well not be the case, right? Then, you tell SWUpdate it's >>>> available while it is not? >>>> [see bottom comment first] >>>> >>>> >>>>>>> Well we never really hit the EBG issue since buildroot hasn't ever packaged >>>>>>> efibootguard at all. I guess we probably should reintroduce the >>>>>>> HAVE_LIBEBGENV variable in case we add efibootguard support in the >>>>>>> future. >>>>>> >>>>>> You will run into the same problem as EFI Boot Guard integration works >>>>>> alike. Eventually, we may probably also have others like zchunk or rsync >>>>>> run-time configurable and then we have the problem again. >>>>>> So, ideally, the HAVE_ options should be set depending on the SWUpdate >>>>>> options chosen and the available libraries on the build/target system at >>>>>> build-time. If they don't match, it doesn't build. >>>>>> This would also solve your problem I guess. >>>>> >>>>> The purpose of the HAVE_ options is so that buildroot can tell swupdate >>>>> what options swupdate can enable when running swupdate-menuconfig >>>>> inside of buildroot and so that swupdate's menuconfig can say what is >>>>> missing when being run by the user using buildroot's configure system. >>>> >>>> OK, but then ― if I got this correctly ― you need to have some sort of >>>> library checking in SWUpdate to match SWUpdate's view to Buildroot's? >>>> If enabled unconditionally, SWUpdate will present the libubootenv >>>> option, always, without ever noting what's missing? >>>> [see bottom comment first] >>> >>> Buildroot passes the env flags when invoking swupdate's menuconfig: >>> https://github.com/buildroot/buildroot/blob/master/package/swupdate/swupdate.mk >>> >>> As these values are passed by buildroot swupdate doesn't need library >>> checking beyond the env variable checking effectively. >> >> Right - I will just add that the HAVE_ configuration was introduced to >> support Buildroot. Neither Yocto nor Debian needs this, because they >> have their own mechanism (via packageconfig or DEPENDS in recipe). >> >> In both Yocto and Buildroot, we do not need some sort of library >> checking: the packages in Buildroot are strict defined, and recipe in >> Yocto reads SWUpdate configuration to set dependency to the bootloader >> library. > > Note that buildroot AFAIU isn't capable of adjusting package dependencies > like yocto can. We use the HAVE_ variables so that the swupdate configuration > is appropriately restricted based on the buildroot configuration. Right, this is the way we introduced at the beginning. > >> >> To sumarize the history: dynamic loading is important for standard linux >> distros, else it is difficult to create a SWUpdate package (.deb) that >> works in any condition. >> >>> >>>> >>>> >>>>>>>>> (1) Port those external type definition to the bootloader binding >>>>>>>>> implementations in SWUpdate (need to be maintained!) or >>>>>>>> >>>>>>>> We duplicate code and we go into trouble if the libraries are changing >>>>>>>> the API. >>>>>> >>>>>> Right, and it's work on SWUpdate's side that can be avoided, agreed. >>>>>> >>>>>> >>>>>>> For buildroot runtime and compile time libraries should always be the >>>>>>> same as we don't really support binary package installation. >>>>>>> >>>>>>> I think it may make sense to have an option to disable dlopen library >>>>>>> loading entirely as this feature only seems useful for systems that >>>>>>> support binary package installation. >>>>>> >>>>>> Exactly this was the purpose: Enable bootloader selection at run-time by >>>>>> configuration rather than statically at compile-time. This actually >>>>>> enables SWUpdate to be shipped as a package by (binary) distributions >>>>>> with probably support for multiple bootloaders enabled. >>>>>> Then, you have to prepare an SWUpdate package per architecture and just >>>>>> configure it to use the right bootloader (out of those whose support is >>>>>> enabled). >>>>> >>>>> I'm not saying it's not useful for some cases, but as embedded systems >>>>> that use swupdate often handle updates simply by replacing the entire >>>>> rootfs it's likely not something needed in a lot of setups as they will >>>>> only be updating swupdate by replacing the rootfs and not individually. >>>> >>>> Yes, but it's not that uncommon to build the rootfs with a meta-distribution >>>> based on binary packages, e.g., ELBE, ISAR, ..., for good reasons. >> >> I think we completely agree we have different use cases :-). >> >> My point is just to understand if re-adding HAVE_<bootloader>, that we >> had in the past, can have some drawbacks and damage the use cases with >> distros, where dlopen() is strictly required. In case of Buildroot >> /Yocto, well, SWUpdate will still dlopen(), but just one library is >> built and available. > > It's mostly an issue for compile time dependencies I guess, buildroot passes > HAVE_ variables based on compile time dependencies which just so happen > to be the same as runtime dependencies in our case. Right. > >> >>>> For (binary) distributions, if compile-time tying the bootloader to the >>>> SWUpdate binary, you would have to supply binary packages for all >>>> combinations, i.e., swupdate-<bootloader>-<arch>.pkg. With this feature, >>>> you just have to make *one* package swupdate-<arch>.pkg and configure >>>> the bootloader to use. It's a big win for (binary) distributions with >>>> respect to maintainability, testing efforts, etc. >> >> Absolutely, it was added for binary distributions. But Buildroot / Yocto >> are building from sources. >> >>> at IMO no cost for the >>>> "classic" embedded use cases. >>>> We do exactly this for many products: Use a binary meta-distribution and >>>> replace the rootfs in an A/B deployment. >>> >>> I mostly mean the dlopen part isn't likely to be useful for buildroot due >>> to the lack of binary package support in general. Separate issue from >>> multi-bootloader in some ways. >> >> It is not useful, like for Yocto, but it should not have drawbacks if >> the link is done at the startup instead of static linking. >> >>> >>>> >>>> >>>>>> If you disable this feature, then you're back at specifically building >>>>>> SWUpdate packages for each architecture + target combination. >>>>>> While this may work (better?) for Buildroot, it destroys the (binary) >>>>>> distribution use case, e.g., Debian. >>>>> >>>>> I'm just suggesting a config option to allow it to be built with dlopen >>>>> library loading disabled. Buildroot isn't a binary distribution so it's >>>>> just added complexity and may potentially allow say missing library >>>>> issues to go undetected for longer. >> >> The reason for dlopen() is to have the same mechanism for all use cases >> (binary distros + built from sources). In Yocto, I do not see any >> evident disadvantage, because I force the dependency at build time. >> Which is the disadvantage with Buildroot ? libubootenv should be still >> be set in your board defconfig to make it working, and if libubootenv is >> in your rootfs, it should be the same if linking is done when SWUpdate >> starts. >> >>>> >>>> Hm, this would then require maintaining two bindings to each bootloader: >>>> one for the "static" linkage and one for dlopen(). >> >> Right, and goal was to have a solution for all. >> >>> If Buildroot tells >>>> SWUpdate what's available (and installs this to the target) and SWUpdate >>>> would only use what Buildroot told it to use, then I don't see a problem? >> >> I am not seeing the problem, too - so I like to understand what I am >> missing. >> >>>> >>>> >>>>>> Even so, with the implemented probing, you do have the same run-time and >>>>>> compile-time libraries: They're just loaded differently (dlopen() vs. >>>>>> ldlinux). If you chose at compile-time to just use U-Boot, then only >>>>>> U-Boot support is enabled and is the default, i.e., libubootenv is >>>>>> dlopen()'d on SWUpdate startup. >>>>>> >>>>>> So I do not see a benefit in introducing an option to disable this >>>>>> feature. Did I overlook one? >> >> I will also prefer to avoid this if it is not really strictly required - >> that means, Buildroot support is broken and there are no other way to >> fix it. >> >>>>> >>>>> The issue I think is that ldlinux loading ensures that if the program runs >>>>> all expected libraries are available while dlopen tends to be more likely >>>>> to fail later, it's basically more brittle. >>>> >>>> In principle yes, and I would call this a feature for other use cases ;) >>>> Anyway, that's why we use dlopen() with RTLD_NOW and load the libraries >>>> quite early in the startup of SWUpdate. So, if it fails, it fails early >>>> upfront and not while first using the bootloader functionality. >>>> This was very important to get similar fail-early behavior like when >>>> using ldlinux. Granted, now it fails a bit later but still prior to >>>> SWUpdate normal operation, i.e., while SWUpdate's initialization, and >>>> even gives you a nice(r) error message ;) >>> >>> Ok, wasn't aware it would throw messages like that so I guess that's less >>> of a concern. >> >> Right, SWUpdate should not be started if the required library is not >> loaded. This was a must. >> >>> >>>> >>>> For example, this is what I get when I move libebgenv.so.0 out of the way: >>>> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : Registered bootloaders: >>>> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : uboot loaded. >>>> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : none loaded. >>>> [TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg shared lib not found. >>>> [ERROR] : SWUPDATE failed [0] ERROR core/swupdate.c : main : 779 : Default bootloader interface 'ebg' couldn't be loaded. >>>> [INFO ] : SWUPDATE running : [main] : Check that the bootloader interface shared library is present. >>>> [INFO ] : SWUPDATE running : [main] : Or chose another bootloader interface by supplying -B <loader>. >>>> >>>> >>>>> Also static linking doesn't work with dlopen(although not sure swupdate >>>>> really supports that properly in general). >>>> >>>> Well, at least not without some extra work (post-processing the ELFs >>>> which is possible in this case as they're not random run-time plugins >>>> but defined bindings) or losing the benefits you're after when >>>> statically linking by allowing the "cruft" to creep in via loadable >>>> modules again: You have to initialize ld.so for libdl to properly >>>> execute dlopen() and this without symbol clashes. It does work but >>>> it's an overly ugly hack just for proofing the concept. >>> >>> I think static linking would be the main reason to support non-dlopen >>> libraries here as static linking is still somewhat common on lower end >>> boards. >> >> I fully agree that we cannot have a running SWUpdate and later during an >> update, SWUpdate reports that a library cannot be found. This is not >> acceptable. >> >>> >>>> >>>> >>>>>>>>> (2) wire SWUpdate compilation to detected libraries on the build system >>>>>>>>> (using e.g. pkgconf) ― this may help other such dependencies as well. >>>>>>>> >>>>>>>> This works on some build system, of course Linux Distros (but it is not >>>>>>>> used in current Debian Package). >>>>>> >>>>>> Yes, some do, some don't and use other mechanisms but it's an option. >>>>>> Anyway, we probably don't want to mandate using it. It was meant as an >>>>>> example of how SWUpdate can detect whether the required dependencies are >>>>>> installed and bailing out otherwise with a probably more "friendly" >>>>>> error message ;) >>>>>> >>>>>> This is for build-time on the build system. On the target system you >>>>>> have to install the required dependencies, too. So you need some >>>>>> machinery to keep build- and run-time dependencies in sync. >>>>> >>>>> Yeah, for buildroot we kind of have to use something like the env variables >>>>> since library availability is determined effectively entirely by the buildroot >>>>> configuration that swupdate is being built inside of. >>>>> >>>>>>> I don't think this really works with buildroot either as we want to >>>>>>> pass the available libraries at configure time (available libraries >>>>>>> selected in buildroot's configuration which is also kconfig based) >>>>>>> while using something like pkgconf would not work as the packages are >>>>>>> not actually being built when configuring. >>>>>> >>>>>> Hm, I don't get this point: You have to compile the selected dependencies >>>>>> of SWUpdate prior to compiling SWUpdate, so you do/may have the pkgconf >>>>>> entries which you can then query for auto-detecting the dependencies of >>>>>> SWUpdate, can't you? >>>>> >>>>> The "make swupdate-menuconfig" in buildroot which invokes swupdate's >>>>> own menuconfig happens prior to building any packages and thus pkgconf >>>>> detection won't actually work at that stage. We only compile the dependencies >>>>> after both swupdate and buildroot are configured. >>>> >>>> OK, so you kind of "mimic" something like pkgconf by the ENV_ variables >>>> which define the available and to be built libraries at the later build >>>> stage. Hence, you cannot detect while SWUpdate menuconfig whether a >>>> particular library is/will be present or not. Hence, you set libubootenv >>>> and thereby convey to Buildroot (?) to install libubootenv at the later >>>> build stage. If so, then introducing ENV_libebgenv would also enforce >>>> to build/install libebgenv which might be unused as you've opted for >>>> using libubootenv? >>> >>> Swupdate doesn't tell buildroot to install anything, buildroot tells swupdate >>> what features it's allowed to use based on the buildroot config. >> >> That's right, it works in the other way. >> >>> >>>> >>>> >>>>>>>> My point here (apart having the same behavior for all bootloaders, that >>>>>>>> is EBG, too) is if this patch has drawbacks that I do not understand / >>>>>>>> see, because these HAVE_ were explicitely removed with the commit above. >>>>>> >>>>>> They were removed since you select to compile in support for bootloaders >>>>>> at run-time. This doesn't necessarily mean that there actually is the >>>>>> proper library on the target system. So you have to configure it on the >>>>>> target to chose the bootloader for which (1) SWUpdate has support >>>>>> compiled in and (2) whose support library is actually installed. >>>>> >>>>> Yeah, in our case though our build configuration matches the runtime >>>>> in effectively all use cases as we don't build binary package artifacts. >>>> >>>> OK, as this is in sync, I think there's no problem with the dlopen() >>>> solution per se, right? >>> >>> Static linking being broken is probably the main issue. >> >> Does it mean that we agree that there should not be problem with >> dlopen() per se, right ? Buildroot (like Yocto) will just have one >> possible library, Debian-Like could choose at startup which library >> should be linked, but we can support both of them, right ? > > From my understanding dlopen() is entirely incompatible with pure static > builds. These AFAIU would be non-glibc userspaces such as uclibc/musl. This is a good point. SWUpdate was already compatible with musl, and there are reports from ML from users using it, surely an older version. It is a pity that we drop support (at least for musl). I think we have to reintroduce it, and add static linking as configuration option in case dynamic linking is not available. > >> >>> >>>> >>>> >>>>>> Relying on the HAVE_ options all turned on means you always build >>>>>> support for all bootloaders ― which is not what you may want? >>>>> >>>>> We still allow the user to configure the options, we just use the HAVE_ >>>>> variables to restrict what can be configured. >>>> >>>> OK, if we HAVE_libubootenv and HAVE_libebgenv, then both are build but >>>> only one is used, the one which is ticked in SWUpdate's menuconfig? >>> >>> Well the HAVE variables are used to tell swupdate what it's allowed to >>> configure, >> >> Right. >> >>> but the swupdate config still determines what is actually built in swupdate. >> >> Correct. >> >> Do we have a deal about what should be done ? Can we put again HAVE_ >> (used by Buildroot, ignored by other use cases) ? Any open issue having >> dlopen() in Buildroot, even if, let's say, it is quite useless ? > > The HAVE_ variables should only restrict option selections when defined so I > don't think they will cause issues for other use cases when not used. > > The dlopen() issue is more a matter of if we want to support fully static > swupdate binaries(ie uclibc/musl fully static userspace builds). Very good point, and MUSL support should be added again. Best regards, Stefano
diff --git a/Kconfig b/Kconfig index 85fa5fd..bcea48b 100644 --- a/Kconfig +++ b/Kconfig @@ -53,6 +53,10 @@ config HAVE_LIBUBI bool option env="HAVE_LIBUBI" +config HAVE_LIBUBOOTENV + bool + option env="HAVE_LIBUBOOTENV" + config HAVE_LIBEXT2FS bool option env="HAVE_LIBEXT2FS" diff --git a/Makefile.deps b/Makefile.deps index 08df4e2..97ac96e 100644 --- a/Makefile.deps +++ b/Makefile.deps @@ -42,6 +42,10 @@ ifeq ($(HAVE_LIBUBI),) export HAVE_LIBUBI = y endif +ifeq ($(HAVE_LIBUBOOTENV),) +export HAVE_LIBUBOOTENV = y +endif + ifeq ($(HAVE_LIBZEROMQ),) export HAVE_LIBZEROMQ = y endif diff --git a/bootloader/Config.in b/bootloader/Config.in index 1744b61..fc133f3 100644 --- a/bootloader/Config.in +++ b/bootloader/Config.in @@ -20,10 +20,14 @@ config BOOTLOADER_EBG config UBOOT bool "U-Boot" + depends on HAVE_LIBUBOOTENV help Support for U-Boot https://www.denx.de/wiki/U-Boot +comment "U-Boot needs libubootenv" + depends on !HAVE_LIBUBOOTENV + config UBOOT_FWENV string "U-Boot Environment Configuration file" depends on UBOOT
Allow passing through the environment if libubootenv is available for the build target. Defaults to yes because we do not do any library checking for now. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- Kconfig | 4 ++++ Makefile.deps | 4 ++++ bootloader/Config.in | 4 ++++ 3 files changed, 12 insertions(+)