[Buildroot] virtual-packages: the case for multiple providers selected

[Buildroot] virtual-packages: the case for multiple providers selected

[Yann E. MORIN]

Hello All!

We have recently identified the reasons for some weird autobuilder
failures.

The failures happen when two or more providers of the same virtual
package are enabled at once in the same config.

Although this seems like a minor issue, as a carefull user will probably
never generate such a config on purpose, we're still exposed to this
isue for two reasons:

  - the autobuilders do random configurations to stress-test the
    packages combinations will continue to generate such configurations;

  - a user who does a configuration mistake that is thus hard to debug
    and understand.

So, we need to find away to avoid this situation.

Two options have been proposed by Thomas P. on IRC:

  - add a choice to select one and only provider at a time, and make all
    the providers prompt-less config options so it is not possible to
    choose more than one at a time;

  - add a pre-build check that verifies that not two providers of the
    same feature are selected, and bail out early in the build if that
    is the case.

Both of those options have issues.

  - going with the choice means that it is no longer possible to add a
    new provider in BR2_EXTERNAL without changing the Buildroot source
    tree, one of the main selling-point of BR2_EXTERNAL to begin with,

  - going with the check means that it will still possible to generate
    such configurations, which means we'd still get autobuild failures
    for those (unless the autobuilders are tweaked to recognise this,)
    while it would be a minimal annoyance to the user.

Thomas P. suggested that providers for our virtual packages would not be
supported in BR2_EXTERNAL. This would allow us to go with the choice
option.

I am rather opposed to this, since I believe allowing providers from
BR2_EXTERNAL is a requirement for BR2_EXTERNAL, and we do want to
support this situation. After all, I can easily see a BR2_EXTERNAL tree
with proprietary, non-public providers for 3d and/or video-decoding
hardware acceleration.

On the other hand, kconfig does not expose the number of config item
that select another one, e.g. it is not possible to know how many
packages did 'select HAS_EGL', we'd have to resort to the .mk to do the
calculations and the check. I think this should be doable in a
relatively non-invasive way, with some Makefile trickery.

So, what do you guys think of these two proposals? Do you have an
alternate solution to propose?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas Petazzoni]

Hello,

On Tue, 13 May 2014 20:09:45 +0200, Yann E. MORIN wrote:

> Two options have been proposed by Thomas P. on IRC:
> 
>   - add a choice to select one and only provider at a time, and make all
>     the providers prompt-less config options so it is not possible to
>     choose more than one at a time;
> 
>   - add a pre-build check that verifies that not two providers of the
>     same feature are selected, and bail out early in the build if that
>     is the case.
> 
> Both of those options have issues.
> 
>   - going with the choice means that it is no longer possible to add a
>     new provider in BR2_EXTERNAL without changing the Buildroot source
>     tree, one of the main selling-point of BR2_EXTERNAL to begin with,

I don't agree that it's the main selling point of BR2_EXTERNAL. The
main selling point was the ability to have package .mk files outside of
the main Buildroot tree. Which remains entirely possible. The only
limitation that a choice introduces is that such an out-of-tree
package .mk file cannot implement a provider for an in-tree virtual
package, without making an in-tree change. It is certainly less nice
than it was, but it clearly doesn't make BR2_EXTERNAL useless at all.

>   - going with the check means that it will still possible to generate
>     such configurations, which means we'd still get autobuild failures
>     for those (unless the autobuilders are tweaked to recognise this,)
>     while it would be a minimal annoyance to the user.

Right, for the autobuilders, it would be a bit annoying. We could also
decide to ignore the problem completely, and have the autobuilder
script reject random configurations that have multiple providers
selected for a given virtual package. I however don't really see how to
implement that in a generic fashion, so we would have to have an
explicit list of all possible providers for all virtual packages.

> I am rather opposed to this, since I believe allowing providers from
> BR2_EXTERNAL is a requirement for BR2_EXTERNAL, and we do want to
> support this situation. After all, I can easily see a BR2_EXTERNAL tree
> with proprietary, non-public providers for 3d and/or video-decoding
> hardware acceleration.

That use case is indeed true.

> On the other hand, kconfig does not expose the number of config item
> that select another one, e.g. it is not possible to know how many
> packages did 'select HAS_EGL', we'd have to resort to the .mk to do the
> calculations and the check. I think this should be doable in a
> relatively non-invasive way, with some Makefile trickery.
> 
> So, what do you guys think of these two proposals? Do you have an
> alternate solution to propose?

The third option is the one I exposed earlier: do nothing, and work
around the problem in the autobuilder scripts.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas De Schampheleire]

Hi,

"Yann E. MORIN" <yann.morin.1998@free.fr> schreef:
>Hello All!
>
>We have recently identified the reasons for some weird autobuilder
>failures.
>
>The failures happen when two or more providers of the same virtual
>package are enabled at once in the same config.
>
>Although this seems like a minor issue, as a carefull user will probably
>never generate such a config on purpose, we're still exposed to this
>isue for two reasons:
>
>  - the autobuilders do random configurations to stress-test the
>    packages combinations will continue to generate such configurations;
>
>  - a user who does a configuration mistake that is thus hard to debug
>    and understand.
>
>So, we need to find away to avoid this situation.
>
>Two options have been proposed by Thomas P. on IRC:
>
>  - add a choice to select one and only provider at a time, and make all
>    the providers prompt-less config options so it is not possible to
>    choose more than one at a time;
>
>  - add a pre-build check that verifies that not two providers of the
>    same feature are selected, and bail out early in the build if that
>    is the case.
>
>Both of those options have issues.
>
>  - going with the choice means that it is no longer possible to add a
>    new provider in BR2_EXTERNAL without changing the Buildroot source
>    tree, one of the main selling-point of BR2_EXTERNAL to begin with,

I guess it's not possible to put an 'include' statement inside the choice? The included file would then just contain the external options.


>
>  - going with the check means that it will still possible to generate
>    such configurations, which means we'd still get autobuild failures
>    for those (unless the autobuilders are tweaked to recognise this,)
>    while it would be a minimal annoyance to the user.
>

It doesn't look too difficult to me to handle such prebuild checks in a generic way in the autobuilders.
Suppose that a magic return code is used if a prebuild check fails (also for the kernel headers check for example), then the autobuilders can check for this magic code and then simply ignore the configuration and generate a new one.


>Thomas P. suggested that providers for our virtual packages would not be
>supported in BR2_EXTERNAL. This would allow us to go with the choice
>option.
>
>I am rather opposed to this, since I believe allowing providers from
>BR2_EXTERNAL is a requirement for BR2_EXTERNAL, and we do want to
>support this situation. After all, I can easily see a BR2_EXTERNAL tree
>with proprietary, non-public providers for 3d and/or video-decoding
>hardware acceleration.

Agreed.

(...)

Best regards,
Thomas

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas Petazzoni]

Dear Thomas De Schampheleire,

(Would be nice to wrap your e-mails!)

On Tue, 13 May 2014 22:12:59 +0200, Thomas De Schampheleire wrote:

> >  - going with the choice means that it is no longer possible to add a
> >    new provider in BR2_EXTERNAL without changing the Buildroot source
> >    tree, one of the main selling-point of BR2_EXTERNAL to begin with,
> 
> I guess it's not possible to put an 'include' statement inside the choice? The included file would then just contain the external options.

What would it include? Remember that include statements in kconfig fail
if the target file doesn't exist.

> >  - going with the check means that it will still possible to generate
> >    such configurations, which means we'd still get autobuild failures
> >    for those (unless the autobuilders are tweaked to recognise this,)
> >    while it would be a minimal annoyance to the user.
> >
> 
> It doesn't look too difficult to me to handle such prebuild checks in a generic way in the autobuilders.
> Suppose that a magic return code is used if a prebuild check fails (also for the kernel headers check for example), then the autobuilders can check for this magic code and then simply ignore the configuration and generate a new one.

Or we could have a specific make target to allow the autobuilders to
check a configuration before starting the build.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] infra/pkg-virtual: validate only one provider provides an implementation

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

BIG FAT NOTICE:
    This is *not* tested.
    This is only a track I'd like to further explore to fix the issue.

Currently, it is possible that more than one provider of a virtual package
is selected in the menuconfig.

This leads to autobuild failures, and we do not protect the user from
making a mistake in the configuration. The failure is then hard to
troubleshoot in any case.

We can't use kconfig constructs to prevent this, sicne kconfig does not
tell how many options did a select on another option.

This patch adds a new function that providers *must* call in their .mk
to ensure at most one provider is selected.

This works by taking advantage that when more than one provider is
selected, only one of them will 'win' in setting the _PROVIDES_FOO
option. Thus any provider just have to check they are indeed the declared
provider. If not, it means that one or more other provider is selected.

This gives the opportunity to the user to change its configuration, and
we can match the error message in the autobuilders to skip those failures
(we can skip them instead of reporting them, since they are obviously
configuration errors that should not happen in the first place.)

NOT Signed-off-by on purpose.
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Peter Korsgaard <jacmet@uclibc.org>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Samuel Martin <s.martin49@gmail.com>
---
 package/pkg-virtual.mk               | 9 +++++++++
 package/rpi-userland/rpi-userland.mk | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/package/pkg-virtual.mk b/package/pkg-virtual.mk
index 617e5f2..66537bf 100644
--- a/package/pkg-virtual.mk
+++ b/package/pkg-virtual.mk
@@ -13,6 +13,15 @@
 #
 ################################################################################
 
+# Providers shall call this function with all the FEATURES they provide
+# 	$(eval $(call virt-provides,FEATURE[ FEATURE ...]))
+# where FEATURE has a corresponding BR2_PACKAGE_HAS_FEATURE
+define virt-provides
+$(foreach p,$(1),\
+ifneq ($$(BR2_PACKAGE_PROVIDES_$(p)),$(pkgname))$(sep)\
+$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\
+endif$(sep))
+endef
 
 ################################################################################
 # inner-virtual-package -- defines the dependency rules of the virtual
diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
index f6e4443..86125f2 100644
--- a/package/rpi-userland/rpi-userland.mk
+++ b/package/rpi-userland/rpi-userland.mk
@@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP
 endef
 RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
 
+# rpi-userland is a provider for those features:
+$(eval $(call virt-provides,LIBEGL LIBGLES OPENVG OPENMAX))
+
 $(eval $(cmake-package))
-- 
1.8.3.2

[Buildroot] [PATCH] infra/pkg-virtual: validate only one provider provides an implementation

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Tue, 13 May 2014 23:47:50 +0200, Yann E. MORIN wrote:

> +# Providers shall call this function with all the FEATURES they provide
> +# 	$(eval $(call virt-provides,FEATURE[ FEATURE ...]))
> +# where FEATURE has a corresponding BR2_PACKAGE_HAS_FEATURE
> +define virt-provides
> +$(foreach p,$(1),\
> +ifneq ($$(BR2_PACKAGE_PROVIDES_$(p)),$(pkgname))$(sep)\
> +$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\
> +endif$(sep))
> +endef
>  
>  ################################################################################
>  # inner-virtual-package -- defines the dependency rules of the virtual
> diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> index f6e4443..86125f2 100644
> --- a/package/rpi-userland/rpi-userland.mk
> +++ b/package/rpi-userland/rpi-userland.mk
> @@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP
>  endef
>  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
>  
> +# rpi-userland is a provider for those features:
> +$(eval $(call virt-provides,LIBEGL LIBGLES OPENVG OPENMAX))
> +
>  $(eval $(cmake-package))

Just thinking out loud: isn't it possible to instead check if
<pkg>_DEPENDENCIES for each virtual package contains only one word?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] infra/pkg-virtual: validate only one provider provides an implementation

[Yann E. MORIN]

Thomas, All,

On 2014-05-14 00:05 +0200, Thomas Petazzoni spake thusly:
> Dear Yann E. MORIN,
> 
> On Tue, 13 May 2014 23:47:50 +0200, Yann E. MORIN wrote:
> 
> > +# Providers shall call this function with all the FEATURES they provide
> > +# 	$(eval $(call virt-provides,FEATURE[ FEATURE ...]))
> > +# where FEATURE has a corresponding BR2_PACKAGE_HAS_FEATURE
> > +define virt-provides
> > +$(foreach p,$(1),\
> > +ifneq ($$(BR2_PACKAGE_PROVIDES_$(p)),$(pkgname))$(sep)\
> > +$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\
> > +endif$(sep))
> > +endef
> >  
> >  ################################################################################
> >  # inner-virtual-package -- defines the dependency rules of the virtual
> > diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> > index f6e4443..86125f2 100644
> > --- a/package/rpi-userland/rpi-userland.mk
> > +++ b/package/rpi-userland/rpi-userland.mk
> > @@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP
> >  endef
> >  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
> >  
> > +# rpi-userland is a provider for those features:
> > +$(eval $(call virt-provides,LIBEGL LIBGLES OPENVG OPENMAX))
> > +
> >  $(eval $(cmake-package))
> 
> Just thinking out loud: isn't it possible to instead check if
> <pkg>_DEPENDENCIES for each virtual package contains only one word?

No, because FOO_DEPENDENCIES is set from _PROVIDES_FOO, which as a
kconfig option, can be only one word to begin with.

I've tried to remove _PROVIDES_FOO for the Config.in and move it into
the .mk, but then, as it is used to set FOO_DEPENDENCIES, it could be
empty at the time we define the virtual package, which would be
depndency-less, when the provider sorts after the virtual package.

Unless Thomas DS' patchset would allow to have depndencies evaluated as
second-expansion, in which case we could then define _PROVIDES_FOO in
the .mk instead of the Config.in (but we would still need to checit is
not already set, as this patch does.)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas De Schampheleire]

On Tue, May 13, 2014 at 10:18 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas De Schampheleire,
>
> (Would be nice to wrap your e-mails!)

Yes sorry, it's the darn K9 mail app on Android that does not offer a
wrap option (or wraps the entire mail including the mail you reply
on).
Who makes a decent mailing list friendly app for Android?

>
> On Tue, 13 May 2014 22:12:59 +0200, Thomas De Schampheleire wrote:
>
>> >  - going with the choice means that it is no longer possible to add a
>> >    new provider in BR2_EXTERNAL without changing the Buildroot source
>> >    tree, one of the main selling-point of BR2_EXTERNAL to begin with,
>>
>> I guess it's not possible to put an 'include' statement inside the choice? The included file would then just contain the external options.
>
> What would it include? Remember that include statements in kconfig fail
> if the target file doesn't exist.

Something like this (tested):
tdescham at argentina ~/repo/contrib/buildroot-bugs $ hg diff
diff --git a/Config.in b/Config.in
--- a/Config.in
+++ b/Config.in
@@ -369,6 +369,9 @@ config BR2_STRIP_none
        help
          Do not strip binaries and libraries in the target
          filesystem.
+
+source "$BR2_EXTERNAL/test.in"
+
 endchoice

 config BR2_STRIP_EXCLUDE_FILES
diff --git a/support/dummy-external/test.in b/support/dummy-external/test.in
new file mode 100644
--- /dev/null
+++ b/support/dummy-external/test.in
@@ -0,0 +1,2 @@
+config BR2_STRIP_test
+       bool "test"


test.in would either exist in dummy-external, or in BR2_EXTERNAL, so
there is no problem of missing files.
It requires a separate file in BR2_EXTERNAL that has to be present if
you choose to use BR2_EXTERNAL, but can be empty. Moreover, you need
one such file for each choice you want to expand this way.
I'm leaning more towards the preflight check, but I just want to
highlight that expanding a choice from BR2_EXTERNAL is effectively
possible.

>
>> >  - going with the check means that it will still possible to generate
>> >    such configurations, which means we'd still get autobuild failures
>> >    for those (unless the autobuilders are tweaked to recognise this,)
>> >    while it would be a minimal annoyance to the user.
>> >
>>
>> It doesn't look too difficult to me to handle such prebuild checks in a generic way in the autobuilders.
>> Suppose that a magic return code is used if a prebuild check fails (also for the kernel headers check for example), then the autobuilders can check for this magic code and then simply ignore the configuration and generate a new one.
>
> Or we could have a specific make target to allow the autobuilders to
> check a configuration before starting the build.

Yes, that sounds very good too.

Best regards,
Thomas

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas Petazzoni]

Dear Thomas De Schampheleire,

On Wed, 14 May 2014 09:21:33 +0200, Thomas De Schampheleire wrote:

> tdescham at argentina ~/repo/contrib/buildroot-bugs $ hg diff
> diff --git a/Config.in b/Config.in
> --- a/Config.in
> +++ b/Config.in
> @@ -369,6 +369,9 @@ config BR2_STRIP_none
>         help
>           Do not strip binaries and libraries in the target
>           filesystem.
> +
> +source "$BR2_EXTERNAL/test.in"
> +
>  endchoice
> 
>  config BR2_STRIP_EXCLUDE_FILES
> diff --git a/support/dummy-external/test.in b/support/dummy-external/test.in
> new file mode 100644
> --- /dev/null
> +++ b/support/dummy-external/test.in
> @@ -0,0 +1,2 @@
> +config BR2_STRIP_test
> +       bool "test"
> 
> 
> test.in would either exist in dummy-external, or in BR2_EXTERNAL, so
> there is no problem of missing files.
> It requires a separate file in BR2_EXTERNAL that has to be present if
> you choose to use BR2_EXTERNAL, but can be empty. Moreover, you need
> one such file for each choice you want to expand this way.
> I'm leaning more towards the preflight check, but I just want to
> highlight that expanding a choice from BR2_EXTERNAL is effectively
> possible.

Then it means that as soon as you define a BR2_EXTERNAL value, it
*must* point to a directory containing all the "<foo>.in" files for all
possible virtual packages that Buildroot has. This really doesn't seem
nice.

Remember, we're not talking about *one* choice option here, we're
talking about one *per* virtual package: one for EGL, one for OpenVG,
one for OpenGLES, one for OpenGL, one for ..., one for ...

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas De Schampheleire]

On Wed, May 14, 2014 at 9:30 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas De Schampheleire,
>
> On Wed, 14 May 2014 09:21:33 +0200, Thomas De Schampheleire wrote:
>
>> tdescham at argentina ~/repo/contrib/buildroot-bugs $ hg diff
>> diff --git a/Config.in b/Config.in
>> --- a/Config.in
>> +++ b/Config.in
>> @@ -369,6 +369,9 @@ config BR2_STRIP_none
>>         help
>>           Do not strip binaries and libraries in the target
>>           filesystem.
>> +
>> +source "$BR2_EXTERNAL/test.in"
>> +
>>  endchoice
>>
>>  config BR2_STRIP_EXCLUDE_FILES
>> diff --git a/support/dummy-external/test.in b/support/dummy-external/test.in
>> new file mode 100644
>> --- /dev/null
>> +++ b/support/dummy-external/test.in
>> @@ -0,0 +1,2 @@
>> +config BR2_STRIP_test
>> +       bool "test"
>>
>>
>> test.in would either exist in dummy-external, or in BR2_EXTERNAL, so
>> there is no problem of missing files.
>> It requires a separate file in BR2_EXTERNAL that has to be present if
>> you choose to use BR2_EXTERNAL, but can be empty. Moreover, you need
>> one such file for each choice you want to expand this way.
>> I'm leaning more towards the preflight check, but I just want to
>> highlight that expanding a choice from BR2_EXTERNAL is effectively
>> possible.
>
> Then it means that as soon as you define a BR2_EXTERNAL value, it
> *must* point to a directory containing all the "<foo>.in" files for all
> possible virtual packages that Buildroot has. This really doesn't seem
> nice.
>
> Remember, we're not talking about *one* choice option here, we're
> talking about one *per* virtual package: one for EGL, one for OpenVG,
> one for OpenGLES, one for OpenGL, one for ..., one for ...
>

Yes indeed. It's not nice, but it's possible.
By the way, it also means that whenever a new choice is added, every
BR2_EXTERNAL in the world needs to be updated in order to work
correctly.

This could be solved if it were possible to merge a set of dummy such
files with the actual BR2_EXTERNAL. Or by using different env
variables BR2_EXTERNAL_FOR_CHOICE_EGL, BR2_EXTERNAL_FOR_CHOICE_OPENVG,
... where these variables are set to either BR2_EXTERNAL or
dummy-external, depending on the existence of the corresponding file.
These variables would be set by buildroot, before calling menuconfig.
But as you can see, it becomes very ugly.

Best regards,
Thomas

[Buildroot] virtual-packages: the case for multiple providers selected

[Thomas Petazzoni]

Dear Thomas De Schampheleire,

On Wed, 14 May 2014 09:34:43 +0200, Thomas De Schampheleire wrote:

> Yes indeed. It's not nice, but it's possible.

But makes the BR2_EXTERNAL usage really ugly :/

> By the way, it also means that whenever a new choice is added, every
> BR2_EXTERNAL in the world needs to be updated in order to work
> correctly.
> 
> This could be solved if it were possible to merge a set of dummy such
> files with the actual BR2_EXTERNAL. Or by using different env
> variables BR2_EXTERNAL_FOR_CHOICE_EGL, BR2_EXTERNAL_FOR_CHOICE_OPENVG,
> ... where these variables are set to either BR2_EXTERNAL or
> dummy-external, depending on the existence of the corresponding file.
> These variables would be set by buildroot, before calling menuconfig.
> But as you can see, it becomes very ugly.

Yes it does :/

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] infra/pkg-virtual: validate only one provider provides an implementation

[Arnout Vandecappelle]

On 13/05/14 23:47, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> BIG FAT NOTICE:
>     This is *not* tested.
>     This is only a track I'd like to further explore to fix the issue.
> 
> Currently, it is possible that more than one provider of a virtual package
> is selected in the menuconfig.
> 
> This leads to autobuild failures, and we do not protect the user from
> making a mistake in the configuration. The failure is then hard to
> troubleshoot in any case.
> 
> We can't use kconfig constructs to prevent this, sicne kconfig does not
> tell how many options did a select on another option.
> 
> This patch adds a new function that providers *must* call in their .mk
> to ensure at most one provider is selected.

 I like it!

 It's really simple, it's easy to understand. And since it has to be called
explicitly, it doesn't block the possibility for virtual packages with multiple
providers that _don't_ conflict (although admittedly those would still have a
reproducibility problem, but anyway we're not looking at that issue now).

 It's also really easy to check for in review.

> 
> This works by taking advantage that when more than one provider is
> selected, only one of them will 'win' in setting the _PROVIDES_FOO
> option. Thus any provider just have to check they are indeed the declared
> provider. If not, it means that one or more other provider is selected.
> 
> This gives the opportunity to the user to change its configuration, and
> we can match the error message in the autobuilders to skip those failures
> (we can skip them instead of reporting them, since they are obviously
> configuration errors that should not happen in the first place.)
> 
> NOT Signed-off-by on purpose.
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <jacmet@uclibc.org>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Samuel Martin <s.martin49@gmail.com>
> ---
>  package/pkg-virtual.mk               | 9 +++++++++
>  package/rpi-userland/rpi-userland.mk | 3 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/package/pkg-virtual.mk b/package/pkg-virtual.mk
> index 617e5f2..66537bf 100644
> --- a/package/pkg-virtual.mk
> +++ b/package/pkg-virtual.mk
> @@ -13,6 +13,15 @@
>  #
>  ################################################################################
>  
> +# Providers shall call this function with all the FEATURES they provide
> +# 	$(eval $(call virt-provides,FEATURE[ FEATURE ...]))
> +# where FEATURE has a corresponding BR2_PACKAGE_HAS_FEATURE
> +define virt-provides

 Even better would be to make this part of generic-package, and add a
PKG_PROVIDES = ... variable

> +$(foreach p,$(1),\
> +ifneq ($$(BR2_PACKAGE_PROVIDES_$(p)),$(pkgname))$(sep)\

 We could throw in an UPPERCASE here so the caller can put it in lowercase.

> +$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\
> +endif$(sep))
> +endef

 How about (assuming the PKG_PROVIDES path):

Before inner-generic-package:

--------------------------------
# virt-provides-single <virt-pkg> <VIRT_PKG> <provider-pkg>
define virt-provides-single
ifneq ($$(BR2_PACKAGE_PROVIDES_$(2)),$(3))
$$(error Configuration error: $(3) and $$(BR2_PACKAGE_PROVIDES_$(2))$(sep)\
	are both selected as providers for virtual package $(1).$(sep)\
	Only one of them should be selected. Please adapt your configuration.)
endif
endef
--------------------------------


Within inner-generic-package:

--------------------------------
ifneq ($$($(2)_PROVIDES),)
$$(foreach pkg,$$($(2)_PROVIDES),\
	$$(call virt-provides-single,$$(pkg),$$(call UPPERCASE,$$(pkg)),$(1)))
endif
--------------------------------

>  
>  ################################################################################
>  # inner-virtual-package -- defines the dependency rules of the virtual
> diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> index f6e4443..86125f2 100644
> --- a/package/rpi-userland/rpi-userland.mk
> +++ b/package/rpi-userland/rpi-userland.mk
> @@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP
>  endef
>  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
>  
> +# rpi-userland is a provider for those features:
> +$(eval $(call virt-provides,LIBEGL LIBGLES OPENVG OPENMAX))

 So this would become

RPI_USERLAND_PROVIDES = libegl libegles openvg openmax



 I'm thinking, with this approach, it may even be possible to remove the
Config.in part of the providers. Ah, no it's not possible, because then you have
the ordering problem that Yann already mentioned in another mail. Maybe it would
be possible somehow to force the virtual packages to be evaluated only at the
end, but I think that that's just going to complicate stuff unnecessarily.


 Again, I'm pretty happy with what we have here.


 Regards,
 Arnout

> +
>  $(eval $(cmake-package))
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] virtual-packages: the case for multiple providers selected

[Arnout Vandecappelle]

On 13/05/14 20:09, Yann E. MORIN wrote:
>   - add a choice to select one and only provider at a time, and make all
>     the providers prompt-less config options so it is not possible to
>     choose more than one at a time;

 I'm really against this. One of the great things about the current virtual
package infrastructure is that it makes it possible to avoid a choice construct.
And it still is possible to have the choice construct as well. Just perfect.


> 
>   - add a pre-build check that verifies that not two providers of the
>     same feature are selected, and bail out early in the build if that
>     is the case.

 With Yann's proposed patch I think this is really feasible.


 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH] infra/pkg-virtual: validate only one provider provides an implementation

[Yann E. MORIN]

Arnout, All,

On 2014-05-14 10:10 +0200, Arnout Vandecappelle spake thusly:
> On 13/05/14 23:47, Yann E. MORIN wrote:
> > From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > 
> > BIG FAT NOTICE:
> >     This is *not* tested.
> >     This is only a track I'd like to further explore to fix the issue.
> > 
> > Currently, it is possible that more than one provider of a virtual package
> > is selected in the menuconfig.
> > 
> > This leads to autobuild failures, and we do not protect the user from
> > making a mistake in the configuration. The failure is then hard to
> > troubleshoot in any case.
> > 
> > We can't use kconfig constructs to prevent this, sicne kconfig does not
> > tell how many options did a select on another option.
> > 
> > This patch adds a new function that providers *must* call in their .mk
> > to ensure at most one provider is selected.
> 
>  I like it!

Hehe! :-)

[--SNIP--]
> > diff --git a/package/pkg-virtual.mk b/package/pkg-virtual.mk
> > index 617e5f2..66537bf 100644
> > --- a/package/pkg-virtual.mk
> > +++ b/package/pkg-virtual.mk
> > @@ -13,6 +13,15 @@
> >  #
> >  ################################################################################
> >  
> > +# Providers shall call this function with all the FEATURES they provide
> > +# 	$(eval $(call virt-provides,FEATURE[ FEATURE ...]))
> > +# where FEATURE has a corresponding BR2_PACKAGE_HAS_FEATURE
> > +define virt-provides
> 
>  Even better would be to make this part of generic-package, and add a
> PKG_PROVIDES = ... variable

Yes, that's what I was gonna go for in the end, since it is even easier
to write than the eval+call.

> > +$(foreach p,$(1),\
> > +ifneq ($$(BR2_PACKAGE_PROVIDES_$(p)),$(pkgname))$(sep)\
> 
>  We could throw in an UPPERCASE here so the caller can put it in lowercase.

Yes, good idea. This way, there would be consistency between the menu
prompt, and the content of FOO_PROVIDES.

> > +$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\
> > +endif$(sep))
> > +endef
> 
>  How about (assuming the PKG_PROVIDES path):
> 
> Before inner-generic-package:
> 
> --------------------------------
> # virt-provides-single <virt-pkg> <VIRT_PKG> <provider-pkg>
> define virt-provides-single
> ifneq ($$(BR2_PACKAGE_PROVIDES_$(2)),$(3))
> $$(error Configuration error: $(3) and $$(BR2_PACKAGE_PROVIDES_$(2))$(sep)\
> 	are both selected as providers for virtual package $(1).$(sep)\
> 	Only one of them should be selected. Please adapt your configuration.)
> endif
> endef
> --------------------------------
> 
> 
> Within inner-generic-package:
> 
> --------------------------------
> ifneq ($$($(2)_PROVIDES),)
> $$(foreach pkg,$$($(2)_PROVIDES),\
> 	$$(call virt-provides-single,$$(pkg),$$(call UPPERCASE,$$(pkg)),$(1)))
> endif
> --------------------------------
> 
> >  
> >  ################################################################################
> >  # inner-virtual-package -- defines the dependency rules of the virtual
> > diff --git a/package/rpi-userland/rpi-userland.mk b/package/rpi-userland/rpi-userland.mk
> > index f6e4443..86125f2 100644
> > --- a/package/rpi-userland/rpi-userland.mk
> > +++ b/package/rpi-userland/rpi-userland.mk
> > @@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP
> >  endef
> >  RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
> >  
> > +# rpi-userland is a provider for those features:
> > +$(eval $(call virt-provides,LIBEGL LIBGLES OPENVG OPENMAX))
> 
>  So this would become
> 
> RPI_USERLAND_PROVIDES = libegl libegles openvg openmax
> 
>  I'm thinking, with this approach, it may even be possible to remove the
> Config.in part of the providers. Ah, no it's not possible, because then you have
> the ordering problem that Yann already mentioned in another mail. Maybe it would
> be possible somehow to force the virtual packages to be evaluated only at the
> end, but I think that that's just going to complicate stuff unnecessarily.
> 
> 
>  Again, I'm pretty happy with what we have here.

OK, I'll go with that. Thanks for the suggestions! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'