| Message ID | 1487106796-15403-1-git-send-email-yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
On 14-02-17 22:13, Yann E. MORIN wrote: > Some GL providers will provide libegl and libgles, but not libgl, while > other will provide libgl but not libegl not libgles. This can be the > case with: > - mesa3d -> libgl > - rpi-userland -> libegl and libgles The commit message is missing an explanation of why this is bad... Something like "Such a situation leads to conflicting headers and libraries when building a package that uses both libgl and libegl." > > Since we can not protect against this situation in the Config.in files > (especially because providers may be out-of-tree), we can only check for > the validity after the fact. Perhaps it would be possible to create a new opengl virtual package, and that the libgl* providers provide opengl instead of the specific libgl. So libgl etc. would no longer be virtual packages by themselves, but just suboptions of opengl. The libgl* consumers would depend on BR2_PACKAGE_HAS_LIBGL but add opengl to their dependencies instead of libgl. Clearly this alternative is too invasive for 2017.02, so your patch is still valid. > > Fixes: > http://autobuild.buildroot.org/results/6f1/6f197c643972e92f0b27b3afac7da7b4b1115f7e/ > http://autobuild.buildroot.org/results/6c6/6c6e0a236bde7b892a69119a49422119f3efaa97/ > http://autobuild.buildroot.org/results/5a2/5a2a1503b7d19c0ca5b0e9093aa24ed9c020e812/ Does the autobuilder script filter out errors like these? If not, the autobuilder errors will not be fixed, they'll just move... > ... > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > package/opengl/opengl.mk | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/package/opengl/opengl.mk b/package/opengl/opengl.mk > index abf96d5..47902f2 100644 > --- a/package/opengl/opengl.mk > +++ b/package/opengl/opengl.mk > @@ -1 +1,30 @@ > include $(sort $(wildcard package/opengl/*/*.mk)) > + > +# Ensure consistency betwenn the various GL providers: > + > +# If we have a libgl provider, then the libegl provider must be the same > +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBEGL),yy) > +# No need to qstrip, both are quoted > +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBEGL)) > +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ > + as for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL))) > +endif > +endif Shouldn't all this be protected by BR_BUILDING? Regards, Arnout > + > +# If we have a libgl provider, then the libgles provider must be the same > +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBGLES),yy) > +# No need to qstrip, both are quoted > +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBGLES)) > +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ > + as for libgles ($(BR2_PACKAGE_PROVIDES_LIBGLES))) > +endif > +endif > + > +# If we have a libegl provider, then the libgles provider must be the same > +ifeq ($(BR2_PACKAGE_HAS_LIBEGL)$(BR2_PACKAGE_HAS_LIBGLES),yy) > +# No need to qstrip, both are quoted > +ifneq ($(BR2_PACKAGE_PROVIDES_LIBEGL),$(BR2_PACKAGE_PROVIDES_LIBGLES)) > +$(error Provider for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL)) is not the same \ > + as for libgles ($(BR2_PACKAGE_PROVIDES_LIBGLES))) > +endif > +endif >
Arnout, All, On 2017-02-16 18:46 +0100, Arnout Vandecappelle spake thusly: > On 14-02-17 22:13, Yann E. MORIN wrote: > > Some GL providers will provide libegl and libgles, but not libgl, while > > other will provide libgl but not libegl not libgles. This can be the > > case with: > > - mesa3d -> libgl > > - rpi-userland -> libegl and libgles > > The commit message is missing an explanation of why this is bad... Something > like "Such a situation leads to conflicting headers and libraries when building > a package that uses both libgl and libegl." OK, I can fix that. > > Since we can not protect against this situation in the Config.in files > > (especially because providers may be out-of-tree), we can only check for > > the validity after the fact. > > Perhaps it would be possible to create a new opengl virtual package, and that > the libgl* providers provide opengl instead of the specific libgl. So libgl etc. > would no longer be virtual packages by themselves, but just suboptions of > opengl. The libgl* consumers would depend on BR2_PACKAGE_HAS_LIBGL but add > opengl to their dependencies instead of libgl. I don't like it, because it means that it is no longer possible to have out-of-tree providers (e.g. in a br2-external tree). (If I understood correctly what you are proposing.) > Clearly this alternative is too invasive for 2017.02, so your patch is still valid. > > > > > Fixes: > > http://autobuild.buildroot.org/results/6f1/6f197c643972e92f0b27b3afac7da7b4b1115f7e/ > > http://autobuild.buildroot.org/results/6c6/6c6e0a236bde7b892a69119a49422119f3efaa97/ > > http://autobuild.buildroot.org/results/5a2/5a2a1503b7d19c0ca5b0e9093aa24ed9c020e812/ > > Does the autobuilder script filter out errors like these? If not, the > autobuilder errors will not be fixed, they'll just move... Clearly, the autobuilder scripts will have to learn to catch this, as they are currently catching the multiple-providers case. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > > package/opengl/opengl.mk | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/package/opengl/opengl.mk b/package/opengl/opengl.mk > > index abf96d5..47902f2 100644 > > --- a/package/opengl/opengl.mk > > +++ b/package/opengl/opengl.mk > > @@ -1 +1,30 @@ > > include $(sort $(wildcard package/opengl/*/*.mk)) > > + > > +# Ensure consistency betwenn the various GL providers: > > + > > +# If we have a libgl provider, then the libegl provider must be the same > > +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBEGL),yy) > > +# No need to qstrip, both are quoted > > +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBEGL)) > > +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ > > + as for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL))) > > +endif > > +endif > > Shouldn't all this be protected by BR_BUILDING? I was wondering if we should have it, so that we could re-enter the menuconfig in case of error, but it seems that it was not needed. And we do not want to allow 'make source' either, sonce the configuration is invalid. What are you trying to protect against (here) with BR_BUILDING? Regards, Yann E. MORIN.
On 16-02-17 18:56, Yann E. MORIN wrote: > Arnout, All, > > On 2017-02-16 18:46 +0100, Arnout Vandecappelle spake thusly: >> On 14-02-17 22:13, Yann E. MORIN wrote: [snip] >>> Since we can not protect against this situation in the Config.in files >>> (especially because providers may be out-of-tree), we can only check for >>> the validity after the fact. >> >> Perhaps it would be possible to create a new opengl virtual package, and that >> the libgl* providers provide opengl instead of the specific libgl. So libgl etc. >> would no longer be virtual packages by themselves, but just suboptions of >> opengl. The libgl* consumers would depend on BR2_PACKAGE_HAS_LIBGL but add >> opengl to their dependencies instead of libgl. > > I don't like it, because it means that it is no longer possible to have > out-of-tree providers (e.g. in a br2-external tree). > > (If I understood correctly what you are proposing.) Probably not :-) My proposal would break current out-of-tree providers (and consumers), but new providers would still be possible. The idea is that libgl, libgles and libegl would no longer be virtual packages. Instead, opengl would be a virtual package, with three suboptions BR2_PACKAGE_HAS_LIBGL, BR2_PACKAGE_HAS_LIBGLES and BR2_PACKAGE_HAS_LIBEGL, each of which would select BR2_PACKAGE_HAS_OPENGL. A provider would select one of the suboptions (which implicitly selects HAS_OPENGL), and would set only BR2_PACKAGE_PROVIDES_OPENGL to itself. A consumer would depend on one of the suboptions in the Config.in file, and add opengl to its DEPENDENCIES in the .mk file. BR2_PACKAGE_HAS_OPENGL itself would not be used except by the virtual-package infra. Basically, it boils down to reusing the existing infra for detecting conflicting providers of virtual packages. >> Clearly this alternative is too invasive for 2017.02, so your patch is still valid. >> >>> >>> Fixes: >>> http://autobuild.buildroot.org/results/6f1/6f197c643972e92f0b27b3afac7da7b4b1115f7e/ >>> http://autobuild.buildroot.org/results/6c6/6c6e0a236bde7b892a69119a49422119f3efaa97/ >>> http://autobuild.buildroot.org/results/5a2/5a2a1503b7d19c0ca5b0e9093aa24ed9c020e812/ >> >> Does the autobuilder script filter out errors like these? If not, the >> autobuilder errors will not be fixed, they'll just move... > > Clearly, the autobuilder scripts will have to learn to catch this, as > they are currently catching the multiple-providers case. > >>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> >>> --- >>> package/opengl/opengl.mk | 29 +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/package/opengl/opengl.mk b/package/opengl/opengl.mk >>> index abf96d5..47902f2 100644 >>> --- a/package/opengl/opengl.mk >>> +++ b/package/opengl/opengl.mk >>> @@ -1 +1,30 @@ >>> include $(sort $(wildcard package/opengl/*/*.mk)) >>> + >>> +# Ensure consistency betwenn the various GL providers: >>> + >>> +# If we have a libgl provider, then the libegl provider must be the same >>> +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBEGL),yy) >>> +# No need to qstrip, both are quoted >>> +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBEGL)) >>> +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ >>> + as for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL))) >>> +endif >>> +endif >> >> Shouldn't all this be protected by BR_BUILDING? > > I was wondering if we should have it, so that we could re-enter the > menuconfig in case of error, but it seems that it was not needed. > > And we do not want to allow 'make source' either, sonce the > configuration is invalid. > > What are you trying to protect against (here) with BR_BUILDING? I didn't thing really hard about it, that's why it was a question :-) It's just that most of this kind of stuff is protected by BR_BUILDING. But indeed, the check in virtual-package isn't, and that's what we try to emulate here, so it's OK. Regards, Arnout
Hello Yann, On Thursday 16 February 2017 18:46:10 CET Arnout Vandecappelle wrote: > > On 14-02-17 22:13, Yann E. MORIN wrote: > > Some GL providers will provide libegl and libgles, but not libgl, while > > other will provide libgl but not libegl not libgles. This can be the > > case with: > > - mesa3d -> libgl > > - rpi-userland -> libegl and libgles Unfortunately, as far as I remember, this kind of configuration is necessary to build xdriver_xf86-video-imx-viv. Mesa is necessary to build GLX support but, EGL/GLes have to be provided by imx-gpu-viv. However, maybe your idea is correct and xdriver_xf86-video-imx-viv should be reworked (but it is not a trivial work). > The commit message is missing an explanation of why this is bad... Something > like "Such a situation leads to conflicting headers and libraries when building > a package that uses both libgl and libegl."
Hello, On Fri, 17 Feb 2017 09:34:38 +0100, Jérôme Pouiller wrote: > On Thursday 16 February 2017 18:46:10 CET Arnout Vandecappelle wrote: > > > > On 14-02-17 22:13, Yann E. MORIN wrote: > > > Some GL providers will provide libegl and libgles, but not libgl, while > > > other will provide libgl but not libegl not libgles. This can be the > > > case with: > > > - mesa3d -> libgl > > > - rpi-userland -> libegl and libgles > > Unfortunately, as far as I remember, this kind of configuration is > necessary to build xdriver_xf86-video-imx-viv. Mesa is necessary to > build GLX support but, EGL/GLes have to be provided by imx-gpu-viv. Nope, it's not actually needed. Even with xdriver_xf86-video-imx-viv, the OpenGL provider is imx-gpu-viv, as it's ultimately the one that provides libGL.so. However, it is true that imx-gpu-viv is a special case as it provides a libGL.so that itself uses libraries provided by Mesa. So the hack I've done so far is: - select BR2_PACKAGE_HAS_LIBGL if BR2_PACKAGE_XORG7 + select BR2_PACKAGE_HAS_LIBGL if BR2_PACKAGE_XORG7 && !BR2_PACKAGE_IMX_GPU_VIV_OUTPUT_X11 and: - default "mesa3d" if BR2_PACKAGE_XORG7 + default "mesa3d" if BR2_PACKAGE_XORG7 && !BR2_PACKAGE_IMX_GPU_VIV_OUTPUT_X11 This is a quick hack, but it allows mesa3d to *NOT* be seen as an OpenGL provider if imx-gpu-viv X11 support is enabled. So I still think the rule should be: we support only one OpenGL provider. And the rest should be solved by exposing mesa3d as an OpenGL provider or not depending on Config.in options. Best regards, Thomas
On Friday 17 February 2017 11:04:36 CET Thomas Petazzoni wrote: > Hello, > > On Fri, 17 Feb 2017 09:34:38 +0100, Jérôme Pouiller wrote: > > > On Thursday 16 February 2017 18:46:10 CET Arnout Vandecappelle wrote: > > > > > > On 14-02-17 22:13, Yann E. MORIN wrote: > > > > Some GL providers will provide libegl and libgles, but not libgl, while > > > > other will provide libgl but not libegl not libgles. This can be the > > > > case with: > > > > - mesa3d -> libgl > > > > - rpi-userland -> libegl and libgles > > > > Unfortunately, as far as I remember, this kind of configuration is > > necessary to build xdriver_xf86-video-imx-viv. Mesa is necessary to > > build GLX support but, EGL/GLes have to be provided by imx-gpu-viv. > > Nope, it's not actually needed. Even with xdriver_xf86-video-imx-viv, > the OpenGL provider is imx-gpu-viv, as it's ultimately the one that > provides libGL.so. Yes, replace "necessary" by "currently used" in my sentence. > However, it is true that imx-gpu-viv is a special case as it provides a > libGL.so that itself uses libraries provided by Mesa. > > So the hack I've done so far is: > > - select BR2_PACKAGE_HAS_LIBGL if BR2_PACKAGE_XORG7 > + select BR2_PACKAGE_HAS_LIBGL if BR2_PACKAGE_XORG7 && !BR2_PACKAGE_IMX_GPU_VIV_OUTPUT_X11 > > and: > > - default "mesa3d" if BR2_PACKAGE_XORG7 > + default "mesa3d" if BR2_PACKAGE_XORG7 && !BR2_PACKAGE_IMX_GPU_VIV_OUTPUT_X11 > > This is a quick hack, but it allows mesa3d to *NOT* be seen as an > OpenGL provider if imx-gpu-viv X11 support is enabled. > > So I still think the rule should be: we support only one OpenGL > provider. And the rest should be solved by exposing mesa3d as an OpenGL > provider or not depending on Config.in options. I agree. Give me a few days to prepare a patch that do that.
Arnout, All, On 2017-02-17 08:56 +0100, Arnout Vandecappelle spake thusly: > On 16-02-17 18:56, Yann E. MORIN wrote: > > Arnout, All, > > > > On 2017-02-16 18:46 +0100, Arnout Vandecappelle spake thusly: > >> On 14-02-17 22:13, Yann E. MORIN wrote: > [snip] > >>> Since we can not protect against this situation in the Config.in files > >>> (especially because providers may be out-of-tree), we can only check for > >>> the validity after the fact. > >> > >> Perhaps it would be possible to create a new opengl virtual package, and that > >> the libgl* providers provide opengl instead of the specific libgl. So libgl etc. > >> would no longer be virtual packages by themselves, but just suboptions of > >> opengl. The libgl* consumers would depend on BR2_PACKAGE_HAS_LIBGL but add > >> opengl to their dependencies instead of libgl. > > > > I don't like it, because it means that it is no longer possible to have > > out-of-tree providers (e.g. in a br2-external tree). > > > > (If I understood correctly what you are proposing.) > > Probably not :-) Damn... ;-) > My proposal would break current out-of-tree providers (and > consumers), but new providers would still be possible. > > The idea is that libgl, libgles and libegl would no longer be virtual packages. > Instead, opengl would be a virtual package, with three suboptions > BR2_PACKAGE_HAS_LIBGL, BR2_PACKAGE_HAS_LIBGLES and BR2_PACKAGE_HAS_LIBEGL, each > of which would select BR2_PACKAGE_HAS_OPENGL. A provider would select one of the > suboptions (which implicitly selects HAS_OPENGL), and would set only > BR2_PACKAGE_PROVIDES_OPENGL to itself. A consumer would depend on one of the > suboptions in the Config.in file, and add opengl to its DEPENDENCIES in the .mk > file. BR2_PACKAGE_HAS_OPENGL itself would not be used except by the > virtual-package infra. > > Basically, it boils down to reusing the existing infra for detecting > conflicting providers of virtual packages. Ah, I see. Yes, this is clever. Yes, I would think this would be acceptable. But clearly this would not be not for 2017.02. [--SNIP--] > >> Shouldn't all this be protected by BR_BUILDING? > > > > I was wondering if we should have it, so that we could re-enter the > > menuconfig in case of error, but it seems that it was not needed. > > > > And we do not want to allow 'make source' either, sonce the > > configuration is invalid. > > > > What are you trying to protect against (here) with BR_BUILDING? > > I didn't thing really hard about it, that's why it was a question :-) It's just > that most of this kind of stuff is protected by BR_BUILDING. But indeed, the > check in virtual-package isn't, and that's what we try to emulate here, so it's OK. I will double-check. Regards, Yann E. MORIN.
Hello, On Tue, 14 Feb 2017 22:13:16 +0100, Yann E. MORIN wrote: > Some GL providers will provide libegl and libgles, but not libgl, while > other will provide libgl but not libegl not libgles. This can be the > case with: > - mesa3d -> libgl > - rpi-userland -> libegl and libgles > > Since we can not protect against this situation in the Config.in files > (especially because providers may be out-of-tree), we can only check for > the validity after the fact. > > Fixes: > http://autobuild.buildroot.org/results/6f1/6f197c643972e92f0b27b3afac7da7b4b1115f7e/ > http://autobuild.buildroot.org/results/6c6/6c6e0a236bde7b892a69119a49422119f3efaa97/ > http://autobuild.buildroot.org/results/5a2/5a2a1503b7d19c0ca5b0e9093aa24ed9c020e812/ > ... > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > package/opengl/opengl.mk | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) This patch has received numerous comments, so I'll mark it as Changes Requested in patchwork. Thanks! Thomas
diff --git a/package/opengl/opengl.mk b/package/opengl/opengl.mk index abf96d5..47902f2 100644 --- a/package/opengl/opengl.mk +++ b/package/opengl/opengl.mk @@ -1 +1,30 @@ include $(sort $(wildcard package/opengl/*/*.mk)) + +# Ensure consistency betwenn the various GL providers: + +# If we have a libgl provider, then the libegl provider must be the same +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBEGL),yy) +# No need to qstrip, both are quoted +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBEGL)) +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ + as for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL))) +endif +endif + +# If we have a libgl provider, then the libgles provider must be the same +ifeq ($(BR2_PACKAGE_HAS_LIBGL)$(BR2_PACKAGE_HAS_LIBGLES),yy) +# No need to qstrip, both are quoted +ifneq ($(BR2_PACKAGE_PROVIDES_LIBGL),$(BR2_PACKAGE_PROVIDES_LIBGLES)) +$(error Provider for libgl ($(BR2_PACKAGE_PROVIDES_LIBGL)) is not the same \ + as for libgles ($(BR2_PACKAGE_PROVIDES_LIBGLES))) +endif +endif + +# If we have a libegl provider, then the libgles provider must be the same +ifeq ($(BR2_PACKAGE_HAS_LIBEGL)$(BR2_PACKAGE_HAS_LIBGLES),yy) +# No need to qstrip, both are quoted +ifneq ($(BR2_PACKAGE_PROVIDES_LIBEGL),$(BR2_PACKAGE_PROVIDES_LIBGLES)) +$(error Provider for libegl ($(BR2_PACKAGE_PROVIDES_LIBEGL)) is not the same \ + as for libgles ($(BR2_PACKAGE_PROVIDES_LIBGLES))) +endif +endif
Some GL providers will provide libegl and libgles, but not libgl, while other will provide libgl but not libegl not libgles. This can be the case with: - mesa3d -> libgl - rpi-userland -> libegl and libgles Since we can not protect against this situation in the Config.in files (especially because providers may be out-of-tree), we can only check for the validity after the fact. Fixes: http://autobuild.buildroot.org/results/6f1/6f197c643972e92f0b27b3afac7da7b4b1115f7e/ http://autobuild.buildroot.org/results/6c6/6c6e0a236bde7b892a69119a49422119f3efaa97/ http://autobuild.buildroot.org/results/5a2/5a2a1503b7d19c0ca5b0e9093aa24ed9c020e812/ ... Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- package/opengl/opengl.mk | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)