virtual-packages: the case for multiple providers selected
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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
Hello,
Quoted 10 lines · Yann E. MORIN
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.
Quoted 5 lines
Both of those options have issues.
- going with the choice means that it is no longer possible to add a new provider in
BR2_EXTERNALwithout changing the Buildroot source tree, one of the main selling-point ofBR2_EXTERNALto 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.
Quoted 4 lines
- 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.
Quoted 5 lines
I am rather opposed to this, since I believe allowing providers from
BR2_EXTERNALis a requirement forBR2_EXTERNAL, and we do want to support this situation. After all, I can easily see aBR2_EXTERNALtree with proprietary, non-public providers for 3d and/or video-decoding hardware acceleration.
That use case is indeed true.
Quoted 8 lines
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 someMakefiletrickery.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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Hi,
"Yann E. MORIN" <[email protected]> schreef:
Quoted 30 lines
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.
Quoted 5 lines
Both of those options have issues.
- going with the choice means that it is no longer possible to add a new provider in
BR2_EXTERNALwithout changing the Buildroot source tree, one of the main selling-point ofBR2_EXTERNALto 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.
Quoted 6 lines
- 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.
Quoted 9 lines
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_EXTERNALis a requirement forBR2_EXTERNAL, and we do want to support this situation. After all, I can easily see aBR2_EXTERNALtree with proprietary, non-public providers for 3d and/or video-decoding hardware acceleration.
Agreed.
(...)
Best regards,
Thomas
Dear Thomas De Schampheleire,
(Would be nice to wrap your e-mails!)
Quoted 5 lines · Thomas De Schampheleire
Quoted 3 lines
- going with the choice means that it is no longer possible to add a new provider in
BR2_EXTERNALwithout changing the Buildroot source tree, one of the main selling-point ofBR2_EXTERNALto 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.
Quoted 8 lines
Quoted 5 lines
- 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Quoted 3 lines · Thomas Petazzoni
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?
Quoted 11 lines · Thomas De Schampheleire
Quoted 6 lines
Quoted 4 lines
- going with the choice means that it is no longer possible to add a new provider in
BR2_EXTERNALwithout changing the Buildroot source tree, one of the main selling-point ofBR2_EXTERNALto 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
Config.in +3 -0
@@ -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
support/dummy-external/test.in new file +2 -0
@@ -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.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
- 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.
Quoted 5 lines
Quoted 2 lines
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
Dear Thomas De Schampheleire,
Quoted 23 lines · Thomas De Schampheleire
tdescham at argentina ~/repo/contrib/buildroot-bugs $ hg diff
Config.in +3
-0@@ -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_FILESsupport/dummy-external/test.in new file +2
-0@@ -0,0 +1,2 @@ +config BR2_STRIP_test + bool "test"
Quoted 8 lines
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 inBR2_EXTERNALthat has to be present if you choose to useBR2_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 fromBR2_EXTERNALis 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Quoted 41 lines · Thomas Petazzoni
Dear Thomas De Schampheleire,
Quoted 23 lines · Thomas De Schampheleire
tdescham at argentina ~/repo/contrib/buildroot-bugs $ hg diff
Config.in +3
-0@@ -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_FILESsupport/dummy-external/test.in new file +2
-0@@ -0,0 +1,2 @@ +config BR2_STRIP_test + bool "test"Quoted 8 lines
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 inBR2_EXTERNALthat has to be present if you choose to useBR2_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 fromBR2_EXTERNALis effectively possible.Then it means that as soon as you define a
BR2_EXTERNALvalue, 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.
Quoted 4 lines
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
Dear Thomas De Schampheleire,
Quoted 1 line · Thomas De Schampheleire
Yes indeed. It's not nice, but it's possible.
But makes the BR2_EXTERNAL usage really ugly :/
Quoted 11 lines
By the way, it also means that whenever a new choice is added, every
BR2_EXTERNALin 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 variablesBR2_EXTERNAL_FOR_CHOICE_EGL,BR2_EXTERNAL_FOR_CHOICE_OPENVG, ... where these variables are set to eitherBR2_EXTERNALor 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 infra/pkg-virtual: validate only one provider provides an implementation
In reply to Yann E. MORINFrom: "Yann E. MORIN" <[email protected]>
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 <[email protected]>
Cc: Peter Korsgaard <[email protected]>
Cc: Thomas De Schampheleire <[email protected]>
Cc: Arnout Vandecappelle <[email protected]>
Cc: Samuel Martin <[email protected]>
--- package/pkg-virtual.mk | 9 +++++++++ package/rpi-userland/rpi-userland.mk | 3 +++ 2 files changed, 12 insertions(+)
package/pkg-virtual.mk +9 -0
@@ -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
package/rpi-userland/rpi-userland.mk +3 -0
@@ -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))
Signature · 1 line
1.8.3.2
PATCH 1/1 infra/pkg-virtual: validate only one provider provides an implementation
Dear Yann E. MORIN,
Quoted 17 lines · Yann E. MORIN
package/pkg-virtual.mk +9
-0+# 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 virtualpackage/rpi-userland/rpi-userland.mk +0
-0
package/rpi-userland/rpi-userland.mk define RPI_USERLAND_POST_TARGET_CLEANUP · line 22
Quoted 7 lines
package/rpi-userland/rpi-userland.mk +3
-0@@ -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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 infra/pkg-virtual: validate only one provider provides an implementation
Thomas, All,
On 2014-05-14 00:05 +0200, Thomas Petazzoni spake thusly:
Quoted 24 lines
Dear Yann E. MORIN,
Quoted 20 lines · Yann E. MORIN
package/pkg-virtual.mk +9
-0+# 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 virtualpackage/rpi-userland/rpi-userland.mk +0
-0@@ -16,4 +16,7 @@ define RPI_USERLAND_POST_TARGET_CLEANUP endef RPI_USERLAND_POST_INSTALL_TARGET_HOOKS += RPI_USERLAND_POST_TARGET_CLEANUP
Quoted 7 lines
package/rpi-userland/rpi-userland.mk define RPI_USERLAND_POST_TARGET_CLEANUP · line 22
Quoted 4 lines
package/rpi-userland/rpi-userland.mk +3
-0+# 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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 1/1 infra/pkg-virtual: validate only one provider provides an implementation
In reply to Yann E. MORINQuoted 13 lines · Yann E. MORIN
From: "Yann E. MORIN" <[email protected]>
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.
Quoted 5 lines
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.
Quoted 27 lines
This works by taking advantage that when more than one provider is selected, only one of them will 'win' in setting the
_PROVIDES_FOOoption. 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 <[email protected]>
Cc: Peter Korsgaard <[email protected]>
Cc: Thomas De Schampheleire <[email protected]>
Cc: Arnout Vandecappelle <[email protected]>
Cc: Samuel Martin <[email protected]>--- package/pkg-virtual.mk | 9 +++++++++ package/rpi-userland/rpi-userland.mk | 3 +++ 2 files changed, 12 insertions(+)package/pkg-virtual.mk +0
-0
package/pkg-virtual.mk line 19
Quoted 7 lines
package/pkg-virtual.mk +4
-0@@ -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 aPKG_PROVIDES = ... variable
package/pkg-virtual.mk line 21
Quoted 2 lines
package/pkg-virtual.mk +2
-0+$(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.
package/pkg-virtual.mk line 24
Quoted 3 lines
package/pkg-virtual.mk +3
-0+$$(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
--------------------------------
package/rpi-userland/rpi-userland.mk define RPI_USERLAND_POST_TARGET_CLEANUP · line 20
Quoted 13 lines
package/pkg-virtual.mk +0
-0################################################################################ # inner-virtual-package -- defines the dependency rules of the virtualpackage/rpi-userland/rpi-userland.mk +2
-0@@ -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
Quoted 3 lines
package/rpi-userland/rpi-userland.mk +1
-0+ $(eval $(cmake-package))
Signature · 6 lines
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
PATCH 1/1 infra/pkg-virtual: validate only one provider provides an implementation
Arnout, All,
On 2014-05-14 10:10 +0200, Arnout Vandecappelle spake thusly:
Quoted 14 lines · Yann E. MORIN
Quoted 13 lines
From: "Yann E. MORIN" <[email protected]>
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.
Quoted 7 lines
Quoted 5 lines
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--]
Quoted 8 lines
Quoted 8 lines
package/pkg-virtual.mk +0
-0@@ -13,6 +13,15 @@ # ################################################################################
Quoted 7 lines
package/pkg-virtual.mk line 19
Quoted 4 lines
package/pkg-virtual.mk +4
-0+# 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-providesEven 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.
Quoted 4 lines
package/pkg-virtual.mk line 21
Quoted 2 lines
package/pkg-virtual.mk +2
-0+$(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.
Quoted 47 lines
package/pkg-virtual.mk line 24
Quoted 3 lines
package/pkg-virtual.mk +3
-0+$$(error $(pkgname is trying to override $$(BR2_PACKAGE_PROVIDES_$(p)) to provide $(p)))$(sep)\ +endif$(sep)) +endefHow about (assuming the
PKG_PROVIDESpath):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
--------------------------------package/rpi-userland/rpi-userland.mk define RPI_USERLAND_POST_TARGET_CLEANUP · line 20
Quoted 13 lines
package/pkg-virtual.mk +0
-0################################################################################ # inner-virtual-package -- defines the dependency rules of the virtualpackage/rpi-userland/rpi-userland.mk +2
-0@@ -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
Quoted 8 lines
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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
Quoted 3 lines · Yann E. MORIN
- 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.
Quoted 4 lines
- 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
Signature · 6 lines
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