| Message ID | cover.1387495624.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Yann E. MORIN, On Fri, 20 Dec 2013 00:43:23 +0100, Yann E. MORIN wrote: > This series is a third attempt at trying to fix the BR2_EXTERNAL issue, > in that packages in BR2_EXTERNAL can't be providers of an implementation > for a virtual package, such as libegl. I must say I really like this proposal! It both makes virtual packages better, and solves the BR2_EXTERNAL problem without making invasive changes in the package infrastructure. A few minor questions: *) Shouldn't we use 'qstrip' when doing: POWERVR_DEPENDENCIES = $(BR2_PACKAGE_PROVIDES_POWERVR) *) The jpeg virtual package should also be converted in the same way. *) In PATCH 1, in package/rpi-userland/Config.in, you're adding an if BR2_PACKAGE_RPI_USERLAND ... endif block, but *before* the comment related to the toolchain dependencies of rpi-userland. I never remember if that is the situation causing indentation problem of the comment in menuconfig or not. Would be good to check this. Thanks for doing this! Thomas
Thomas, All, On Friday 20 December 2013 10:31:43 Thomas Petazzoni wrote: > On Fri, 20 Dec 2013 00:43:23 +0100, Yann E. MORIN wrote: > > > This series is a third attempt at trying to fix the BR2_EXTERNAL issue, > > in that packages in BR2_EXTERNAL can't be providers of an implementation > > for a virtual package, such as libegl. > > I must say I really like this proposal! It both makes virtual packages > better, and solves the BR2_EXTERNAL problem without making invasive > changes in the package infrastructure. Note that I still intend to work on the post-pone patch, as this would allow more integration of BR2_EXTERNAL. > A few minor questions: > > *) Shouldn't we use 'qstrip' when doing: > > POWERVR_DEPENDENCIES = $(BR2_PACKAGE_PROVIDES_POWERVR) Probably. I must say I forgot to mark this PR as an RFC. I did not actually test it, besides checking the Kconfig variable was properly set in .config. I'll conduct more thourough tests tonight and this WE. So: this was an RFC. ;-) > *) The jpeg virtual package should also be converted in the same way. I've looked at the jpeg and cryptodev packages, but they do not follow the "standard" virtual packages scheme (or, as Gustavo put it on IRC yesterday: "they are not real virtual packages"). > *) In PATCH 1, in package/rpi-userland/Config.in, you're adding an if > BR2_PACKAGE_RPI_USERLAND ... endif block, but *before* the comment > related to the toolchain dependencies of rpi-userland. I never > remember if that is the situation causing indentation problem of the > comment in menuconfig or not. Would be good to check this. I'll look into that, too. Regards, Yann E. MORIN.
Dear Yann E. MORIN, On Fri, 20 Dec 2013 10:43:01 +0100, Yann E. MORIN wrote: > > I must say I really like this proposal! It both makes virtual packages > > better, and solves the BR2_EXTERNAL problem without making invasive > > changes in the package infrastructure. > > Note that I still intend to work on the post-pone patch, as this would > allow more integration of BR2_EXTERNAL. Like what? > > A few minor questions: > > > > *) Shouldn't we use 'qstrip' when doing: > > > > POWERVR_DEPENDENCIES = $(BR2_PACKAGE_PROVIDES_POWERVR) > > Probably. I must say I forgot to mark this PR as an RFC. I did not > actually test it, besides checking the Kconfig variable was properly > set in .config. > > I'll conduct more thourough tests tonight and this WE. > > So: this was an RFC. ;-) Yeah, no problem. But it clearly does look nice. > > *) The jpeg virtual package should also be converted in the same way. > > I've looked at the jpeg and cryptodev packages, but they do not follow > the "standard" virtual packages scheme (or, as Gustavo put it on IRC > yesterday: "they are not real virtual packages"). Don't know about cryptodev, but jpeg is really a virtual package, but it's true that it is handled in a different way than the OpenGL packages, in that jpeg/Config.in provides a choice between the different providers. Not sure what we want to do here. Maybe nothing, I don't know. > > *) In PATCH 1, in package/rpi-userland/Config.in, you're adding an if > > BR2_PACKAGE_RPI_USERLAND ... endif block, but *before* the comment > > related to the toolchain dependencies of rpi-userland. I never > > remember if that is the situation causing indentation problem of the > > comment in menuconfig or not. Would be good to check this. > > I'll look into that, too. Thanks! Thomas
Thomas, All, On 2013-12-20 10:31 +0100, Thomas Petazzoni spake thusly: > On Fri, 20 Dec 2013 00:43:23 +0100, Yann E. MORIN wrote: > > > This series is a third attempt at trying to fix the BR2_EXTERNAL issue, > > in that packages in BR2_EXTERNAL can't be providers of an implementation > > for a virtual package, such as libegl. [--SNIP--] > A few minor questions: [--SNIP--] > *) The jpeg virtual package should also be converted in the same way. As you said, jpeg is also a virtual package. But since it is not written the same way the others are, the change will be more involved than the one done in this series. So, I'd prefer this series (which is relatively simple) stays as-is, and that once it is applied, we take care of jpeg (and cryptodev), by turning them into "real virtual" packages (can't help but use that phrase of yours, Gustavo! :-p). > *) In PATCH 1, in package/rpi-userland/Config.in, you're adding an if > BR2_PACKAGE_RPI_USERLAND ... endif block, but *before* the comment > related to the toolchain dependencies of rpi-userland. I never > remember if that is the situation causing indentation problem of the > comment in menuconfig or not. Would be good to check this. Nope, since we're in a 'menu', not a 'menuconfig'. The menu layout is still correct. And even with a menuconfig, what is busted is if one option or a comment is in the middle of other dependent options, such as: menuconfig FOO bool "foo" config BAR bool "bar" depends on FOO comment "FOO deps busted" config BUZ bool "buz" depends on FOO Regards, Yann E. MORIN.
Thomas, All, On 2013-12-20 10:49 +0100, Thomas Petazzoni spake thusly: > On Fri, 20 Dec 2013 10:43:01 +0100, Yann E. MORIN wrote: > > > I must say I really like this proposal! It both makes virtual packages > > > better, and solves the BR2_EXTERNAL problem without making invasive > > > changes in the package infrastructure. > > > > Note that I still intend to work on the post-pone patch, as this would > > allow more integration of BR2_EXTERNAL. > > Like what? I just have to clear a case where a new external package enables a feature for a built-in package. So, br2-external could provide extra ./configure options for a built-in package. But that has deep implications, especially if Buildroot is then distributed as part of a compliance effort: only providing Buildroot and not br2-external would not be compliant. So, in fact, I don't think we should make that easy. > > > A few minor questions: > > > > > > *) Shouldn't we use 'qstrip' when doing: > > > > > > POWERVR_DEPENDENCIES = $(BR2_PACKAGE_PROVIDES_POWERVR) OK, I've fixed that now. > > > *) The jpeg virtual package should also be converted in the same way. > Don't know about cryptodev, cryptodev is like jpeg: it provides a choice. > but jpeg is really a virtual package, I've now converted both of them, too. > but > it's true that it is handled in a different way than the OpenGL > packages, in that jpeg/Config.in provides a choice between the > different providers. Not sure what we want to do here. Maybe nothing, I > don't know. I think we should just convert them over to how other virtual packages are implemented: there is nothing to prevent, eg. rpi-userland and ti-gfx (both providers of EGL) to be selected at the same time. OK, those are a bit special, since they are tied to the hardware, when neither jpeg nor cryptodev are. But still, I think we should have a single way of doing virtual packages. Peter, what's your opinion? Regards, Yann E. MORIN.
From: "Yann E. MORIN" <yann.morin.1998@free.fr> Hello All! This series is a third attempt at trying to fix the BR2_EXTERNAL issue, in that packages in BR2_EXTERNAL can't be providers of an implementation for a virtual package, such as libegl. The basic idea behind this series is that Arnout found it ugly that virtual packages had to each be aware of all their possible providers. For example, libegl.mk had four conditional dependencies, one for each of the builtin providers (freescale's vivante, rpi, TI's gfx, or Allwinner's sunxi mali). This would prohibit a package in BR2_EXTERNAL to provide another implementation (eg. with an in-house proprietary, non-public package), since the virtual package's rules were evaluated even before we had a chance to parse BR2_EXTERNAL, and thus either requiring the new dependency to be added to the virtual package (hence defeating one of the selling-point of BR2_EXTERNAL0, or not allow external packages to provides implementations (hence killing that same selling-point of BR2_EXTERNAL as well). So, Arnout suggested (and I was already working on it) that packages would declare themselves as providers, all in a single Kconfig variable, so the virtual package would just have to depend on that single, Kconfig-set variable. This is what this series does for each virtual packages: - moves the virtual package definition to its own Config.in - introduces the string option BR2_PACKAGE_PROVIDES_xxxx - changes all providers to default that variable to thier own name for each of: opengl-egl, opengl-es, openmax, openvg, and powervr. If anyone is feeling like writing a blurb in the manual, just feel free to go ahead and add a section about virtual packages! ;-p Regards, Yann E. MORIN. The following changes since commit cbb6cdc69cb3f0cfb8783c1fef7e878a43da8567: libcurl: security bump to version 7.34.0 (2013-12-19 23:17:25 +0100) are available in the git repository at: git://gitorious.org/buildroot/buildroot.git yem/providers for you to fetch changes up to e3300b74a954f422626703e3402206636cf35043: package/powervr: switch to package-defined providers (2013-12-20 00:01:42 +0100) ---------------------------------------------------------------- Yann E. MORIN (5): package/opengl/libegl: switch to package-defined providers package/opengl/libgles: switch to package-defined providers package/opengl/libopenvg: switch to package-defined providers package/opengl/libopenmax: switch to package-defined providers package/powervr: switch to package-defined providers package/bellagio/Config.in | 7 +++++++ package/freescale-imx/gpu-viv-bin-mx6q/Config.in | 6 ++++++ package/opengl/Config.in | 15 ++++----------- package/opengl/libegl/Config.in | 6 ++++++ package/opengl/libegl/libegl.mk | 17 +---------------- package/opengl/libgles/Config.in | 6 ++++++ package/opengl/libgles/libgles.mk | 17 +---------------- package/opengl/libopenmax/Config.in | 6 ++++++ package/opengl/libopenmax/libopenmax.mk | 9 +-------- package/opengl/libopenvg/Config.in | 6 ++++++ package/opengl/libopenvg/libopenvg.mk | 5 +---- package/powervr/Config.in | 4 ++++ package/powervr/powervr.mk | 5 +---- package/rpi-userland/Config.in | 16 ++++++++++++++++ package/sunxi-mali/Config.in | 6 ++++++ package/ti-gfx/Config.in | 9 +++++++++ 16 files changed, 81 insertions(+), 59 deletions(-) create mode 100644 package/opengl/libegl/Config.in create mode 100644 package/opengl/libgles/Config.in create mode 100644 package/opengl/libopenmax/Config.in create mode 100644 package/opengl/libopenvg/Config.in