Re: [Buildroot] [PATCH v1 1/1] package/x11r7/xwayland: new package

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Raphael Pavlidis <raphael.pavlidis@gmail.com>
Cc: Bernd Kuhls <bernd.kuhls@t-online.de>,
	Eric Le Bihan <eric.le.bihan.dev@free.fr>,
	Adrian Perez de Castro <aperez@igalia.com>,
	"Yann E . MORIN" <yann.morin.1998@free.fr>,
	buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH v1 1/1] package/x11r7/xwayland: new package
Date: Wed, 15 Feb 2023 21:22:09 +0100	[thread overview]
Message-ID: <20230215212209.269cd96d@windsurf> (raw)
In-Reply-To: <20230215143534.19834-1-raphael.pavlidis@gmail.com>

Hello 

On Wed, 15 Feb 2023 15:35:34 +0100
Raphael Pavlidis <raphael.pavlidis@gmail.com> wrote:

> XServer to run X clients under wayland
> 
> This XServer is required to run the X clients under Wayland compositor,
> for example weston or wlroots based compositors.
> 
> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>

Thanks for the patch! I have a few questions/suggestions below.

>  DEVELOPERS                           |  1 +
>  package/weston/Config.in             |  8 +++
>  package/weston/weston.mk             |  2 +-
>  package/wlroots/Config.in            | 12 ++++
>  package/wlroots/wlroots.mk           |  6 +-
>  package/x11r7/Config.in              |  1 +
>  package/x11r7/xwayland/Config.in     | 26 ++++++++
>  package/x11r7/xwayland/xwayland.hash |  6 ++
>  package/x11r7/xwayland/xwayland.mk   | 94 ++++++++++++++++++++++++++++
>  9 files changed, 154 insertions(+), 2 deletions(-)

This should probably be split into 3 patches: one adding the xwayland
package, one tweaking the existing weston package, and one tweaking the
wlroots package.

> diff --git a/package/weston/Config.in b/package/weston/Config.in
> index 3b4e932b7d..db12afc43a 100644
> --- a/package/weston/Config.in
> +++ b/package/weston/Config.in
> @@ -111,14 +111,22 @@ config BR2_PACKAGE_WESTON_XWAYLAND
>  	bool "XWayland support"
>  	depends on BR2_PACKAGE_XORG7
>  	depends on BR2_PACKAGE_LIBEPOXY
> +	depends on BR2_USE_MMU # xwayland
> +	depends on !BR2_STATIC_LIBS # xwayland
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # xwayland
>  	select BR2_PACKAGE_CAIRO
>  	select BR2_PACKAGE_LIBXCB
>  	select BR2_PACKAGE_XLIB_LIBX11
>  	select BR2_PACKAGE_XLIB_LIBXCURSOR
> +	select BR2_PACKAGE_XWAYLAND

So does that means that the current BR2_PACKAGE_WESTON_XWAYLAND option
is broken?

I am rather confused by how XWayland works. Isn't XWayland a normal
Wayland client? Why does Weston or wlroots, as Wayland compositors,
need to have some support/knowledge for XWayland?

> +config BR2_PACKAGE_WLROOTS_XWAYLAND
> +	bool "XWayland support"
> +	depends on BR2_PACKAGE_XORG7 # libxcb, xcb-util-wm, xwayland
> +	select BR2_PACKAGE_LIBXCB
> +	select BR2_PACKAGE_XCB_UTIL_WM
> +	select BR2_PACKAGE_XWAYLAND

Since you're selecting BR2_PACKAGE_XWAYLAND, you need to replicate:

	depends on BR2_USE_MMU # fork()
	depends on !BR2_STATIC_LIBS # wayland
	depends on BR2_TOOLCHAIN_HAS_THREADS # wayland

and add the corresponding Config.in comment.

> diff --git a/package/wlroots/wlroots.mk b/package/wlroots/wlroots.mk
> index 4f14d20b11..5b930a51be 100644
> --- a/package/wlroots/wlroots.mk
> +++ b/package/wlroots/wlroots.mk
> @@ -30,8 +30,12 @@ WLROOTS_BACKENDS = libinput drm
>  
>  ifeq ($(BR2_PACKAGE_WLROOTS_X11),y)
>  WLROOTS_BACKENDS += x11
> -WLROOTS_CONF_OPTS += -Dxwayland=enabled

So this is also a bug in the current package?

What is WLROOTS_X11 controlling then? Running wlroots as a X11 client?

Otherwise, the xwayland package looks fine from a pure review point of
view (I did not do any testing).

Thanks!

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