| Message ID | 20200425000629.2068191-1-heiko@sntech.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | boot/uboot: add option to define custom dependencies | expand |
Hello Heiko, On Sat, 25 Apr 2020 02:06:29 +0200 Heiko Stuebner <heiko@sntech.de> wrote: > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > A custom uboot version my depend on additional unspecified packages > to be built before the uboot build is attempted. > > One example is an additional config fragment referencing things > from other packages, so add an option similar to the config fragments > where these can be defined. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> Hm, this is interesting. Out of curiosity, what is the specific case/issue you had ? We already have a bunch of BR2_TARGET_UBOOT_NEEDS_xyz options: config BR2_TARGET_UBOOT_NEEDS_DTC config BR2_TARGET_UBOOT_NEEDS_PYLIBFDT config BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS config BR2_TARGET_UBOOT_NEEDS_OPENSSL config BR2_TARGET_UBOOT_NEEDS_LZOP and I just realized that we will need: config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYLIBFDT config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYELFTOOLS indeed, as of U-Boot 2020.01, a number of Python scripts, including the pylibfdt stuff, and binman, are now Python 3 only. We already have a few build failures in the autobuilders due to this. And http://patchwork.ozlabs.org/project/buildroot/patch/20200413171831.12079-1-vincent.stehle@laposte.net/ enables host-python3 to get around this, but I don't see how it can work as it doesn't guarantee that host-python3 is built before U-Boot. So perhaps we should get rid of all this craziness and have just this BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES ? Questions are: - Is this sufficiently user-friendly? It's already not necessary easy to know when one has to enable NEEDS_DTC, NEEDS_PYLIBFDT, etc. So if instead of that one has to know that host-dtc host-python host-swig, etc. have to be enabled. - How to not break too much backward compatibility. Either we do Config.in.legacy handling as we usually do. Or we keep them as hidden booleans and have compatibility logic in uboot.mk to add the proper dependencies. Yann, Peter, Arnout, any opinion on this ? Thanks, Thomas
Heiko, Thomas, All, On 2020-04-25 15:38 +0200, Thomas Petazzoni spake thusly: > On Sat, 25 Apr 2020 02:06:29 +0200 > Heiko Stuebner <heiko@sntech.de> wrote: > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > A custom uboot version my depend on additional unspecified packages > > to be built before the uboot build is attempted. > > > > One example is an additional config fragment referencing things > > from other packages, so add an option similar to the config fragments > > where these can be defined. > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > Hm, this is interesting. Out of curiosity, what is the specific > case/issue you had ? > > We already have a bunch of BR2_TARGET_UBOOT_NEEDS_xyz options: > > config BR2_TARGET_UBOOT_NEEDS_DTC > config BR2_TARGET_UBOOT_NEEDS_PYLIBFDT > config BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS > config BR2_TARGET_UBOOT_NEEDS_OPENSSL > config BR2_TARGET_UBOOT_NEEDS_LZOP > > and I just realized that we will need: > > config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYLIBFDT > config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYELFTOOLS > > indeed, as of U-Boot 2020.01, a number of Python scripts, including the > pylibfdt stuff, and binman, are now Python 3 only. We already have a > few build failures in the autobuilders due to this. And > http://patchwork.ozlabs.org/project/buildroot/patch/20200413171831.12079-1-vincent.stehle@laposte.net/ > enables host-python3 to get around this, but I don't see how it can > work as it doesn't guarantee that host-python3 is built before U-Boot. > > So perhaps we should get rid of all this craziness and have just this > BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES ? > > Questions are: > > - Is this sufficiently user-friendly? It's already not necessary easy > to know when one has to enable NEEDS_DTC, NEEDS_PYLIBFDT, etc. So if > instead of that one has to know that host-dtc host-python host-swig, > etc. have to be enabled. > > - How to not break too much backward compatibility. Either we do > Config.in.legacy handling as we usually do. Or we keep them as > hidden booleans and have compatibility logic in uboot.mk to add the > proper dependencies. > > Yann, Peter, Arnout, any opinion on this ? My opinion on that patch is that i am definitely not in favour of it. If we go that route, then we would have to allow adding any such arbitrary dependencies to a wide range of packages. This is not acceptable in my opinion. Now, there are two situations: - the tool is already in Buildroot: add a new _NEEDS_FOO option like we already have. - the tool is in a br2-external tre: this is in my opinion better served by working on the evaluation-postpone changes Arnou and I have been suggesting for quite a while now. Yes, the second situation is curently cumbersome for some. but remember that br-2xternal is just providing a Makefile fragment that is included in the main Makefiel of Buildrot. As such, you can complement the internal dependencies in a hackish way: $(UBOOT_BUILDDIR)/.spatmp_configred: my-custom-package Yes, this is hackish, but the hack is in your br2-external tree, not in Buildroot. So, I am definitely not in favour of adding such an option as the proposed BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES. Regards, Yann E. MORIN.
Hello, On Sat, 25 Apr 2020 23:13:50 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > My opinion on that patch is that i am definitely not in favour of it. If > we go that route, then we would have to allow adding any such arbitrary > dependencies to a wide range of packages. Without necessarily strongly supporting Heiko's patch, I think it is important to keep in mind that U-Boot is not a package like any others. We offer version selection for U-Boot, custom Git/Subversion repo selection, which we do not offer for other packages. U-Boot has zillions of forks, support for gazillions platforms each with their own funky requirements. We've already added a BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS to pass custom make options, adding the possibility to add custom dependencies would go in the same direction. I however agree that this kind of option is a good recipe for people to do their own hacks on their side, instead of finding a proper way to do it that can be upstreamed to Buildroot. > Now, there are two situations: > > - the tool is already in Buildroot: add a new _NEEDS_FOO option like > we already have. > > - the tool is in a br2-external tre: this is in my opinion better > served by working on the evaluation-postpone changes Arnou and I > have been suggesting for quite a while now. I think it would make sense to hear about what Heiko's use case exactly is, this might help. Thomas
Hi Thomas, Yann, Am Samstag, 25. April 2020, 23:22:45 CEST schrieb Thomas Petazzoni: > Hello, > > On Sat, 25 Apr 2020 23:13:50 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > My opinion on that patch is that i am definitely not in favour of it. If > > we go that route, then we would have to allow adding any such arbitrary > > dependencies to a wide range of packages. > > Without necessarily strongly supporting Heiko's patch, I think it is > important to keep in mind that U-Boot is not a package like any others. > We offer version selection for U-Boot, custom Git/Subversion repo > selection, which we do not offer for other packages. U-Boot has > zillions of forks, support for gazillions platforms each with their own > funky requirements. > > We've already added a BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS to pass custom > make options, adding the possibility to add custom dependencies would > go in the same direction. > > I however agree that this kind of option is a good recipe for people to > do their own hacks on their side, instead of finding a proper way to do > it that can be upstreamed to Buildroot. > > > Now, there are two situations: > > > > - the tool is already in Buildroot: add a new _NEEDS_FOO option like > > we already have. > > > > - the tool is in a br2-external tre: this is in my opinion better > > served by working on the evaluation-postpone changes Arnou and I > > have been suggesting for quite a while now. > > I think it would make sense to hear about what Heiko's use case exactly > is, this might help. the use-case is: - a u-boot config fragment specifying key-dir and key-hint for signing uboot parts - see [0] - our own package managing these (and other) keys - and thus us wanting to make sure the key package gets "build" before u-boot itself I'm not overly attached to my patch, but it somehow felt in line with also the config-fragment option, that allows including other random fragments into the board config file for the u-boot build. So yes, the required package is in a br2-external tree. What is this "evaluation-postpone" thingy? Thanks Heiko [0] https://patchwork.ozlabs.org/project/uboot/patch/20200421002333.111461-6-heiko@sntech.de/ and other patches in that series
Heiko, All, On 2020-04-25 23:31 +0200, Heiko Stübner spake thusly: > Am Samstag, 25. April 2020, 23:22:45 CEST schrieb Thomas Petazzoni: > > On Sat, 25 Apr 2020 23:13:50 +0200 > > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > My opinion on that patch is that i am definitely not in favour of it. If > > > we go that route, then we would have to allow adding any such arbitrary > > > dependencies to a wide range of packages. > > > > Without necessarily strongly supporting Heiko's patch, I think it is > > important to keep in mind that U-Boot is not a package like any others. > > We offer version selection for U-Boot, custom Git/Subversion repo > > selection, which we do not offer for other packages. U-Boot has > > zillions of forks, support for gazillions platforms each with their own > > funky requirements. > > > > We've already added a BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS to pass custom > > make options, adding the possibility to add custom dependencies would > > go in the same direction. > > > > I however agree that this kind of option is a good recipe for people to > > do their own hacks on their side, instead of finding a proper way to do > > it that can be upstreamed to Buildroot. > > > > > Now, there are two situations: > > > > > > - the tool is already in Buildroot: add a new _NEEDS_FOO option like > > > we already have. > > > > > > - the tool is in a br2-external tre: this is in my opinion better > > > served by working on the evaluation-postpone changes Arnou and I > > > have been suggesting for quite a while now. > > > > I think it would make sense to hear about what Heiko's use case exactly > > is, this might help. > > the use-case is: > - a u-boot config fragment specifying key-dir and key-hint for signing > uboot parts - see [0] > - our own package managing these (and other) keys > - and thus us wanting to make sure the key package gets "build" before > u-boot itself > > I'm not overly attached to my patch, but it somehow felt in line with > also the config-fragment option, that allows including other random > fragments into the board config file for the u-boot build. > > So yes, the required package is in a br2-external tree. What is this > "evaluation-postpone" thingy? The basic idea is to postpone the evaluation of all the *-pacakage macros to after all packages have indeed been parsed. For example (just for the illustration sake, it would probably be a little bit more complex, but not by far): define autotool-package $(1)_PKG_TYPE = autotools $(1)_PKG_NAME := $(pkgname) $(1)_PKG_DIR := $(pkgdir) # A very few other stuff, but really not much, even probably nothing REGISTERED_PACKAGES += $($(1)_PKG_NAME) endef And so on for all the other infras. And eventually, after all the packages are parsed, that is after inclusion of all the br2-external trees, a little codde block that iterates over all registered pacakges: $(foreach pkg, $(REGISTERED_PACKAGES), \ $(eval $(call $(call UPPERCASE,$(pkg))-inner))) Of course, that also need a bit of adaptation in all of the *-inner infras, but that should be manageable. The benefits of that would be for br2-external trees, giving them: - the ability to change the version - the ability to add new dependencies, add new configure options etc... it would even be possible to remove some dependencies or config options... - the ability to change the package infra, use an newer version using meson when we have an older usig autotools (or most probably the other way around... - the ability to even completely override the whole package - ... This would be a single answer to all the problems that have been reported when using br2-external trees. Not that I am very happy to have to provide such a solution, but I prefer a single, generic answer rather than a myriad of hacks every where and ekse... Regards, Yann E. MORIN. > > [0] https://patchwork.ozlabs.org/project/uboot/patch/20200421002333.111461-6-heiko@sntech.de/ > and other patches in that series > > >
On 25/04/2020 23:13, Yann E. MORIN wrote: > Heiko, Thomas, All, > > On 2020-04-25 15:38 +0200, Thomas Petazzoni spake thusly: >> On Sat, 25 Apr 2020 02:06:29 +0200 >> Heiko Stuebner <heiko@sntech.de> wrote: >>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> >>> >>> A custom uboot version my depend on additional unspecified packages >>> to be built before the uboot build is attempted. >>> >>> One example is an additional config fragment referencing things >>> from other packages, so add an option similar to the config fragments >>> where these can be defined. >>> >>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> >> >> Hm, this is interesting. Out of curiosity, what is the specific >> case/issue you had ? >> >> We already have a bunch of BR2_TARGET_UBOOT_NEEDS_xyz options: >> >> config BR2_TARGET_UBOOT_NEEDS_DTC >> config BR2_TARGET_UBOOT_NEEDS_PYLIBFDT >> config BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS >> config BR2_TARGET_UBOOT_NEEDS_OPENSSL >> config BR2_TARGET_UBOOT_NEEDS_LZOP >> >> and I just realized that we will need: >> >> config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYLIBFDT >> config BR2_TARGET_UBOOT_NEEDS_PYTHON3_PYELFTOOLS >> >> indeed, as of U-Boot 2020.01, a number of Python scripts, including the >> pylibfdt stuff, and binman, are now Python 3 only. We already have a >> few build failures in the autobuilders due to this. And >> http://patchwork.ozlabs.org/project/buildroot/patch/20200413171831.12079-1-vincent.stehle@laposte.net/ >> enables host-python3 to get around this, but I don't see how it can >> work as it doesn't guarantee that host-python3 is built before U-Boot. >> >> So perhaps we should get rid of all this craziness and have just this >> BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES ? >> >> Questions are: >> >> - Is this sufficiently user-friendly? It's already not necessary easy >> to know when one has to enable NEEDS_DTC, NEEDS_PYLIBFDT, etc. So if >> instead of that one has to know that host-dtc host-python host-swig, >> etc. have to be enabled. The userfriendlyness is an issue caused by U-Boot itself, not by us. If they would have some formal way to declare their dependencies, then at least we would be able to give a decent error message telling the user what to put in _CUSTOM_DEPENDENCIES. >> >> - How to not break too much backward compatibility. Either we do >> Config.in.legacy handling as we usually do. Or we keep them as >> hidden booleans and have compatibility logic in uboot.mk to add the >> proper dependencies. I would tend to say that we should keep the existing ones, at least for a while, and require new ones to use _CUSTOM_DEPENDENCIES. >> >> Yann, Peter, Arnout, any opinion on this ? > > My opinion on that patch is that i am definitely not in favour of it. If > we go that route, then we would have to allow adding any such arbitrary > dependencies to a wide range of packages. The key difference is that for U-Boot (and kernel) we don't just allow, but *expect* the user to specify some custom version. In that respect we differ wildly from almost all other projects (i.e. OpenWrt and yocto) that choose a single or very few kernel and U-Boot versions to work with. Because of this flexibility, we need to also allow stuff we consider ugly like custom git downloads and a custom directory (which we rejected but for which now I believe we really should have the option). So IMO this custom dependencies ugliness is the inevitable price we have to pay for that flexibility. > This is not acceptable in my > opinion. > > Now, there are two situations: > > - the tool is already in Buildroot: add a new _NEEDS_FOO option like > we already have. > > - the tool is in a br2-external tre: this is in my opinion better > served by working on the evaluation-postpone changes Arnou and I > have been suggesting for quite a while now. > > Yes, the second situation is curently cumbersome for some. but remember > that br-2xternal is just providing a Makefile fragment that is included > in the main Makefiel of Buildrot. As such, you can complement the > internal dependencies in a hackish way: > > $(UBOOT_BUILDDIR)/.spatmp_configred: my-custom-package > > Yes, this is hackish, but the hack is in your br2-external tree, not in > Buildroot. The thin is: maintaining the FAQ entry that explains how to do this is IMO a lot more work than maintaining the fairly simple additional config option. > So, I am definitely not in favour of adding such an option as the > proposed BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES. A few years ago, I was also of the opinion that new Config.in options should be treated with utmost suspicion. The main argument there is maintainability. However, "maintaining" a Config.in option is really not much of an effort. The .mk handling of it may be an effort, but in this case it's pretty simple as well. It would also be a problem if infra support was needed - but that's not the case here either. The only thing that I would like is a test of the functionality, because that's the main maintainance effort IMO: making sure that it doesn't break. In this case, the test is easy: we can just rewrite e.g. asus_tinker_rk3288_defconfig to set: BR2_TARGET_UBOOT_NEEDS_DTC=y BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES="openssl" (which tests the combination of the two approaches). Bottom line: I'm in favour. Regards, Arnout
Hello, On Mon, 27 Apr 2020 10:31:51 +0200 Arnout Vandecappelle <arnout@mind.be> wrote: > > My opinion on that patch is that i am definitely not in favour of it. If > > we go that route, then we would have to allow adding any such arbitrary > > dependencies to a wide range of packages. > > The key difference is that for U-Boot (and kernel) we don't just allow, but > *expect* the user to specify some custom version. In that respect we differ > wildly from almost all other projects (i.e. OpenWrt and yocto) that choose a > single or very few kernel and U-Boot versions to work with. > > Because of this flexibility, we need to also allow stuff we consider ugly like > custom git downloads and a custom directory (which we rejected but for which now > I believe we really should have the option). So IMO this custom dependencies > ugliness is the inevitable price we have to pay for that flexibility. I agree. > > So, I am definitely not in favour of adding such an option as the > > proposed BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES. > > > A few years ago, I was also of the opinion that new Config.in options should be > treated with utmost suspicion. The main argument there is maintainability. > However, "maintaining" a Config.in option is really not much of an effort. The > .mk handling of it may be an effort, but in this case it's pretty simple as > well. It would also be a problem if infra support was needed - but that's not > the case here either. > > The only thing that I would like is a test of the functionality, because that's > the main maintainance effort IMO: making sure that it doesn't break. In this > case, the test is easy: we can just rewrite e.g. asus_tinker_rk3288_defconfig to > set: > > BR2_TARGET_UBOOT_NEEDS_DTC=y > BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES="openssl" Which should in fact be: BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES="host-openssl" so it's really not user-friendy it seems :-) > Bottom line: I'm in favour. Thanks. Should I nevertheless introduce the PYLIBFDT_PYTHON3 and PYELFTOOLS_PYTHON3 options, in addition to this ? Or do we for these new use cases favor using only the CUSTOM_DEPENDENCIES solution ? Thanks, Thomas
On 27/04/2020 17:41, Thomas Petazzoni wrote: > Thanks. Should I nevertheless introduce the PYLIBFDT_PYTHON3 and > PYELFTOOLS_PYTHON3 options, in addition to this ? Or do we for these > new use cases favor using only the CUSTOM_DEPENDENCIES solution ? Given how controversial this new option is, I guess it's better to add the -python3 options. The name of the options is a bit awkward, because the package is host-python3-pylibfdt, not pylibfdt-python3... Regards, Arnout
On Mon, 27 Apr 2020 20:19:45 +0200 Heiko Stuebner <heiko.stuebner@theobroma-systems.com> wrote: > Am Montag, 27. April 2020, 18:59:51 CEST schrieb Arnout Vandecappelle: > > On 27/04/2020 17:41, Thomas Petazzoni wrote: > > > Thanks. Should I nevertheless introduce the PYLIBFDT_PYTHON3 and > > > PYELFTOOLS_PYTHON3 options, in addition to this ? Or do we for these > > > new use cases favor using only the CUSTOM_DEPENDENCIES solution ? > > > > Given how controversial this new option is, I guess it's better to add the > > -python3 options. > > personally I imagined CUSTOM_DEPENDENCIES to _not_ be preferred > over standard options. Indeed, they are not necessarily mutually exclusive. > I.e. things like BR2_TARGET_UBOOT_NEEDS_ATF_BL31 come with their > own special handling for example for including the bl31 binary into the > build. NEEDS_ATF_BL31 indeed does more than adding dependencies. But: ifeq ($(BR2_TARGET_UBOOT_NEEDS_DTC),y) UBOOT_DEPENDENCIES += host-dtc endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_PYLIBFDT),y) UBOOT_DEPENDENCIES += host-python host-swig endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_PYELFTOOLS),y) UBOOT_DEPENDENCIES += host-python-pyelftools endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPENSSL),y) UBOOT_DEPENDENCIES += host-openssl endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_LZOP),y) UBOOT_DEPENDENCIES += host-lzop endif is really all about adding dependencies, and we need two more. But OK, I'll cook a patch to support the Python 3.x situation. Thanks! Thomas
Hello Heiko, On Sat, 25 Apr 2020 02:06:29 +0200 Heiko Stuebner <heiko@sntech.de> wrote: > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > A custom uboot version my depend on additional unspecified packages > to be built before the uboot build is attempted. > > One example is an additional config fragment referencing things > from other packages, so add an option similar to the config fragments > where these can be defined. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > --- > boot/uboot/Config.in | 8 ++++++++ > boot/uboot/uboot.mk | 1 + > 2 files changed, 9 insertions(+) Sorry to come back to you so long after the patch was submitted. We finally had a discussion, and decided that we would prefer to see the actual practical use cases of this, before merging this option. Maybe this option is a good idea, but we really want to hear about specific use cases first. If you have one, we're definitely interested to hear about it! Best regards, Thomas
diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in index 7a6f5053fd..310c6e8a80 100644 --- a/boot/uboot/Config.in +++ b/boot/uboot/Config.in @@ -62,6 +62,14 @@ config BR2_TARGET_UBOOT_CUSTOM_SVN endchoice +config BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES + string "Additional dependencies" + help + Specify here package names that are additionally needed. + This can be for example packages that provide things + referenced by additional config fragments or custom u-boot + sources. + config BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE string "U-Boot version" depends on BR2_TARGET_UBOOT_CUSTOM_VERSION diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 2bfa50779b..639be3e520 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -259,6 +259,7 @@ endif # BR2_TARGET_UBOOT_USE_DEFCONFIG UBOOT_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES)) UBOOT_KCONFIG_EDITORS = menuconfig xconfig gconfig nconfig +UBOOT_DEPENDENCIES += $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_DEPENDENCIES)) # UBOOT_MAKE_OPTS overrides HOSTCC / HOSTLDFLAGS to allow the build to # find our host-openssl. However, this triggers a bug in the kconfig