Re: [Buildroot] [PATCH] package/sdl2: support building for Wayland

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Alexander Bau <dev@abau.org>
Cc: Michael Fischer <mf@go-sys.de>, buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH] package/sdl2: support building for Wayland
Date: Sat, 29 Jul 2023 22:50:08 +0200	[thread overview]
Message-ID: <20230729225008.131ad59a@windsurf> (raw)
In-Reply-To: <20230522101438.1026434-1-dev@abau.org>

Hello Alexander,

Thanks for your patch, and sorry for the long delay in providing this
review. See below some comments.

On Mon, 22 May 2023 12:14:37 +0200
Alexander Bau <dev@abau.org> wrote:

Your commit log needs to have a Signed-off-by line with your name and
e-mail. And if needed some extra explanation about the change.

> ---
>  package/sdl2/Config.in | 7 +++++++
>  package/sdl2/sdl2.mk   | 7 ++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> index 7c8258fe73..85d7c09385 100644
> --- a/package/sdl2/Config.in
> +++ b/package/sdl2/Config.in
> @@ -29,6 +29,13 @@ comment "X11 video driver needs X.org"
>  	depends on !BR2_PACKAGE_XORG7
>  	depends on BR2_USE_MMU
>  
> +config BR2_PACKAGE_SDL2_WAYLAND
> +	bool "Wayland video driver"
> +	depends on BR2_PACKAGE_WAYLAND
> +	depends on BR2_PACKAGE_HAS_LIBEGL
> +	depends on BR2_PACKAGE_SDL2_OPENGLES
> +	depends on BR2_PACKAGE_LIBXKBCOMMON

I don't think we want to use "depends on" for all those dependencies. I
think we probably want something like this:

	depends on BR2_PACKAGE_WAYLAND
	depends on BR2_PACKAGE_HAS_LIBEGL
	depends on BR2_PACKAGE_HAS_LIBGLES
	select BR2_PACKAGE_SDL2_OPENGLES
	select BR2_PACKAGE_LIBXKBCOMMON

with also:

comment "wayland video driver needs Wayland, OpenGLES+EGL"
	depends on !BR2_PACKAGE_WAYLAND || !BR2_PACKAGE_HAS_LIBEGL || !BR2_PACKAGE_HAS_LIBGLES

> +ifeq ($(BR2_PACKAGE_SDL2_WAYLAND),y)
> +SDL2_CONF_OPTS += --enable-video-wayland

No build dependencies? This looks odd. I'm pretty you need to depend on
host-pkgconf (for pkg-config), on wayland, and libxkbcommon.

Could you have a look at those comments and if possible, send an
updated version that takes them into account?

See
https://buildroot.org/downloads/manual/manual.html#submitting-patches
for some guidelines on submitting patches if needed.

Thanks a lot for your contribution!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot