| Message ID | 20200621131832.1036346-1-b.bilas@grinn-global.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] package/cegui: select libglu when libgl is detected | expand |
On Sun, 21 Jun 2020 15:18:32 +0200 Bartosz Bilas <b.bilas@grinn-global.com> wrote: > diff --git a/package/cegui/Config.in b/package/cegui/Config.in > index f917be0cc5..0c7932098c 100644 > --- a/package/cegui/Config.in > +++ b/package/cegui/Config.in > @@ -9,6 +9,7 @@ config BR2_PACKAGE_CEGUI > depends on BR2_TOOLCHAIN_HAS_THREADS > depends on BR2_USE_WCHAR > select BR2_PACKAGE_GLM > + select BR2_PACKAGE_LIBGLU if BR2_PACKAGE_HAS_LIBGL This doesn't seem correct. According to the CMakeLists.txt, cegui can use either libepoxy *or* glew as an "OpenGL loader" (whatever that means). So with your patch, we would select libglu even if it is not needed because we have libepoxy available. See the CMakeLists.txt: if (GLEW_FOUND OR EPOXY_FOUND) set (OPENGL_LOADER_FOUND TRUE) else () set (OPENGL_LOADER_FOUND FALSE) endif () This is confirmed by ./cegui/include/CEGUI/RendererModules/OpenGL/GL.h, which contains: #if defined CEGUI_USE_EPOXY #include <epoxy/gl.h> #elif defined CEGUI_USE_GLEW #include <GL/glew.h> // When using GLEW, there's no need to "#include" the OpenGL headers. #ifndef __APPLE__ [...] #else # include <OpenGL/glu.h> #endif #else #error Either "CEGUI_USE_EPOXY" or "CEGUI_USE_GLEW" must be defined. Defining both or none is invalid. #endif So, shouldn't we make things explicit: ifeq ($(BR2_PACKAGE_GLEW)$(BR2_PACKAGE_GLU),yy) CEGUI_CONF_OPTS += -DCEGUI_USE_GLEW=ON else ifeq ($(BR2_PACKAGE_LIBEPOXY),y) CEGUI_CONF_OPTS += -DCEGUI_USE_EPOXY=ON endif or something like that ? Thomas
Hi Thomas, On 22.06.2020 21:10, Thomas Petazzoni wrote: > On Sun, 21 Jun 2020 15:18:32 +0200 > Bartosz Bilas <b.bilas@grinn-global.com> wrote: > >> diff --git a/package/cegui/Config.in b/package/cegui/Config.in >> index f917be0cc5..0c7932098c 100644 >> --- a/package/cegui/Config.in >> +++ b/package/cegui/Config.in >> @@ -9,6 +9,7 @@ config BR2_PACKAGE_CEGUI >> depends on BR2_TOOLCHAIN_HAS_THREADS >> depends on BR2_USE_WCHAR >> select BR2_PACKAGE_GLM >> + select BR2_PACKAGE_LIBGLU if BR2_PACKAGE_HAS_LIBGL > This doesn't seem correct. According to the CMakeLists.txt, cegui can > use either libepoxy *or* glew as an "OpenGL loader" (whatever that > means). So with your patch, we would select libglu even if it is not > needed because we have libepoxy available. No, we wouldn't because libglu is selected only when we have BR2_PACKAGE_HAS_LIBGL option set. Libepoxy doesn't provide it so even if we use glew instead of libepoxy we have to select libglu because cegui directly includes GL/glu.h here -> https://github.com/cegui/cegui/blob/v0-8-7/cegui/include/CEGUI/RendererModules/OpenGL/GL.h#L45 > > See the CMakeLists.txt: > > if (GLEW_FOUND OR EPOXY_FOUND) > set (OPENGL_LOADER_FOUND TRUE) > else () > set (OPENGL_LOADER_FOUND FALSE) > endif () > > This is confirmed by ./cegui/include/CEGUI/RendererModules/OpenGL/GL.h, > which contains: > > #if defined CEGUI_USE_EPOXY > > #include <epoxy/gl.h> > > #elif defined CEGUI_USE_GLEW > > #include <GL/glew.h> > > // When using GLEW, there's no need to "#include" the OpenGL headers. > #ifndef __APPLE__ > [...] > #else > # include <OpenGL/glu.h> > #endif > > #else > #error Either "CEGUI_USE_EPOXY" or "CEGUI_USE_GLEW" must be defined. Defining both or none is invalid. > #endif > > So, shouldn't we make things explicit: > > ifeq ($(BR2_PACKAGE_GLEW)$(BR2_PACKAGE_GLU),yy) > CEGUI_CONF_OPTS += -DCEGUI_USE_GLEW=ON > else ifeq ($(BR2_PACKAGE_LIBEPOXY),y) > CEGUI_CONF_OPTS += -DCEGUI_USE_EPOXY=ON > endif > > or something like that ? > > Thomas Best Bartek
On Mon, 22 Jun 2020 21:37:15 +0200 Bartosz Bilas <b.bilas@grinn-global.com> wrote: > No, we wouldn't because libglu is selected only when we have > BR2_PACKAGE_HAS_LIBGL option set. libepoxy is a library for handling the load of OpenGL library, and has: depends on BR2_PACKAGE_HAS_LIBEGL || BR2_PACKAGE_HAS_LIBGL So basically, when you have HAS_LIBGL=y, you can have either libepoxy *OR* libglew/libglu used by Irrlicht. > Libepoxy doesn't provide it so even if > we use glew instead of libepoxy we have to select libglu because cegui > directly includes GL/glu.h here -> > https://github.com/cegui/cegui/blob/v0-8-7/cegui/include/CEGUI/RendererModules/OpenGL/GL.h#L45 That is exactly the code I have pasted below, please see my comments inline. > > This is confirmed by ./cegui/include/CEGUI/RendererModules/OpenGL/GL.h, > > which contains: > > > > #if defined CEGUI_USE_EPOXY So if the OpenGL loader is libepoxy, we are here... > > > > #include <epoxy/gl.h> > > > > #elif defined CEGUI_USE_GLEW *OR* if the OpenGL loader is glew, we are here, and we indeed include GLU unconditionally. So I stand to my position: according to the code, if you're using libepoxy as the OpenGL loader, you don't need Glew, and you don't need GLU. Thomas
On 22.06.2020 22:01, Thomas Petazzoni wrote: > On Mon, 22 Jun 2020 21:37:15 +0200 > Bartosz Bilas <b.bilas@grinn-global.com> wrote: > >> No, we wouldn't because libglu is selected only when we have >> BR2_PACKAGE_HAS_LIBGL option set. > libepoxy is a library for handling the load of OpenGL library, and has: > > depends on BR2_PACKAGE_HAS_LIBEGL || BR2_PACKAGE_HAS_LIBGL > > So basically, when you have HAS_LIBGL=y, you can have either libepoxy > *OR* libglew/libglu used by Irrlicht. Oh right, somehow I hadn't seen this line before... >> Libepoxy doesn't provide it so even if >> we use glew instead of libepoxy we have to select libglu because cegui >> directly includes GL/glu.h here -> >> https://github.com/cegui/cegui/blob/v0-8-7/cegui/include/CEGUI/RendererModules/OpenGL/GL.h#L45 > That is exactly the code I have pasted below, please see my comments inline. > >>> This is confirmed by ./cegui/include/CEGUI/RendererModules/OpenGL/GL.h, >>> which contains: >>> >>> #if defined CEGUI_USE_EPOXY > So if the OpenGL loader is libepoxy, we are here... > >>> #include <epoxy/gl.h> >>> >>> #elif defined CEGUI_USE_GLEW > *OR* if the OpenGL loader is glew, we are here, and we indeed include > GLU unconditionally. > > So I stand to my position: according to the code, if you're using > libepoxy as the OpenGL loader, you don't need Glew, and you don't need > GLU. I agree with you now, so basically we have to select glu only when glew is used and that should solve the problem discussed... > > Thomas Best Bartek
On Mon, 22 Jun 2020 22:11:31 +0200 Bartosz Bilas <b.bilas@grinn-global.com> wrote: > I agree with you now, so basically we have to select glu only when glew > is used and that should solve the problem discussed... See the solution I have proposed: to be more explicit about which OpenGL loader is used. See my initial reply. Thanks! Thomas
diff --git a/package/cegui/Config.in b/package/cegui/Config.in index f917be0cc5..0c7932098c 100644 --- a/package/cegui/Config.in +++ b/package/cegui/Config.in @@ -9,6 +9,7 @@ config BR2_PACKAGE_CEGUI depends on BR2_TOOLCHAIN_HAS_THREADS depends on BR2_USE_WCHAR select BR2_PACKAGE_GLM + select BR2_PACKAGE_LIBGLU if BR2_PACKAGE_HAS_LIBGL select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE help Crazy Eddie's GUI System is a free library providing windowing diff --git a/package/cegui/cegui.mk b/package/cegui/cegui.mk index 0e3d015948..4aaa065818 100644 --- a/package/cegui/cegui.mk +++ b/package/cegui/cegui.mk @@ -16,7 +16,8 @@ CEGUI_DEPENDENCIES = glm \ $(if $(BR2_PACKAGE_HAS_LIBGL),libgl) \ $(if $(BR2_PACKAGE_HAS_LIBGLES),libgles) \ $(if $(BR2_PACKAGE_LIBGLEW),libglew) \ - $(if $(BR2_PACKAGE_LIBICONV),libiconv) + $(if $(BR2_PACKAGE_LIBICONV),libiconv) \ + $(if $(BR2_PACKAGE_LIBGLU),libglu) ifeq ($(BR2_PACKAGE_LIBEPOXY)$(BR2_PACKAGE_LIBGLEW),y) CEGUI_DEPENDENCIES += libepoxy
Fixes: http://autobuild.buildroot.net/results/1a8/1a866e8722fe111297e4a99b376cf9975ee92ace Signed-off-by: Bartosz Bilas <b.bilas@grinn-global.com> Reviewed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de> --- Changes v1 -> v2: - added reviewed by Bernd Kuhls package/cegui/Config.in | 1 + package/cegui/cegui.mk | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)