Re: [Buildroot] [PATCH v2 2/4] package/cog: depend on wpebackend-fdo only if needed

[Adrian Perez de Castro <aperez@igalia.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6351 bytes --]

Hello,

On Fri, 02 Aug 2024 23:06:44 +0200 Thomas Petazzoni via buildroot <buildroot@buildroot.org> wrote:
 
> On Thu, 27 Jun 2024 22:33:30 +0300
> Adrian Perez de Castro <aperez@igalia.com> wrote:
> 
> > Make the Cog headless platform plug-in selectable, allowing to configure
> > the build without any plug-in at all. When all plug-ins are disabled,
> > Cog does not require wpebackend-fdo at build time, and it is still
> > able to use its built-in "fallback" support to load other WPE backends
> > like wpebackend-rdk.
> 
> This seems to makes sense to me, but I completely fail to understand
> the complicated implementation you are proposing below.

Oops O:-)

> There is in fact something already super wrong in package/cog: it has
> wpebackend-fdo in COG_DEPENDENCIES, but it doesn't select it. It works
> because it depends on wpewebkit, which itself selects wpebackend-fdo,
> but that's really wrong. This should be fixed in a first patch.

You're right!
 
> > Signed-off-by: Adrian Perez de Castro <aperez@igalia.com>
> > ---
> >  package/cog/Config.in | 28 ++++++++++++++++++++++++++--
> >  package/cog/cog.mk    | 13 ++++++++++---
> >  2 files changed, 36 insertions(+), 5 deletions(-)
> > 
> > ---
> > v1 -> v2:
> > - Unchanged.
> > 
> > diff --git a/package/cog/Config.in b/package/cog/Config.in
> > index d2a910f9b89..d706b045b15 100644
> > --- a/package/cog/Config.in
> > +++ b/package/cog/Config.in
> > @@ -19,6 +19,10 @@ config BR2_PACKAGE_COG
> >  
> >  if BR2_PACKAGE_COG
> >  
> > +config BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO
> > +	bool
> > +	default n
> 
> I believe this is not needed.
> 
> >  config BR2_PACKAGE_COG_PROGRAMS_HOME_URI
> >  	string "home uri"
> >  	default "https://wpewebkit.org"
> > @@ -30,6 +34,8 @@ config BR2_PACKAGE_COG_PROGRAMS_HOME_URI
> >  config BR2_PACKAGE_COG_PLATFORM_FDO
> >  	bool "Wayland backend"
> >  	default y
> > +	depends on BR2_PACKAGE_WPEBACKEND_FDO
> > +	select BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO
> 
> If the Wayland backend needs wpebackend-fdo, just select it:
> 
> 	select BR2_PACKAGE_WPEBACKEND_FDO
> 
> >  	select BR2_PACKAGE_LIBXKBCOMMON
> >  	select BR2_PACKAGE_WAYLAND_PROTOCOLS
> >  	help
> > @@ -43,6 +49,8 @@ config BR2_PACKAGE_COG_PLATFORM_DRM
> >  	depends on BR2_PACKAGE_HAS_LIBGBM
> >  	depends on BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF
> >  	depends on BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT
> > +	depends on BR2_PACKAGE_WPEBACKEND_FDO
> > +	select BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO
> 
> If the DRM backend needs wpebackend-fdo, just select it:
> 
> 	select BR2_PACKAGE_WPEBACKEND_FDO
> 
> >  	select BR2_PACKAGE_LIBDRM
> >  	select BR2_PACKAGE_LIBINPUT
> >  	help
> > @@ -50,16 +58,32 @@ config BR2_PACKAGE_COG_PLATFORM_DRM
> >  	  with video drivers that support kernel mode-setting (KMS)
> >  	  via the DRM user-space API.
> >  
> > +config BR2_PACKAGE_COG_PLATFORM_HEADLESS
> > +	bool "Headless backend"
> > +	depends on BR2_PACKAGE_WPEBACKEND_FDO
> > +	select BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO
> 
> Same, just select wpebackend-fdo.
> 
> > +	help
> > +	  Enable the headless platform backend.
> > +
> >  config BR2_PACKAGE_COG_USE_SYSTEM_DBUS
> >  	bool "expose system D-Bus control interface"
> >  	help
> >  	  Expose remote control interface on system bus
> >  
> > -comment "DRM platform needs EGL and GBM"
> > +comment "Headless platform needs wpebackend-fdo"
> > +	depends on \
> > +		!BR2_PACKAGE_WPEBACKEND_FDO
> > +
> > +comment "DRM platform needs EGL, GBM, wpebackend-fdo"
> >  	depends on \
> >  		!BR2_PACKAGE_HAS_LIBEGL || \
> >  		!BR2_PACKAGE_HAS_LIBGBM || \
> >  		!BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF || \
> > -		!BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT
> > +		!BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT || \
> > +		!BR2_PACKAGE_WPEBACKEND_FDO
> > +
> > +comment "Wayland platform needs wpebackend-fdo"
> > +	depends on \
> > +		!BR2_PACKAGE_WPEBACKEND_FDO
> 
> Drop all those changes.
> 
> >  endif
> > diff --git a/package/cog/cog.mk b/package/cog/cog.mk
> > index 7f680bb7005..166f095118b 100644
> > --- a/package/cog/cog.mk
> > +++ b/package/cog/cog.mk
> > @@ -8,7 +8,7 @@ COG_VERSION = 0.18.4
> >  COG_SITE = https://wpewebkit.org/releases
> >  COG_SOURCE = cog-$(COG_VERSION).tar.xz
> >  COG_INSTALL_STAGING = YES
> > -COG_DEPENDENCIES = dbus wpewebkit wpebackend-fdo wayland
> > +COG_DEPENDENCIES = dbus wpewebkit wayland
> >  COG_LICENSE = MIT
> >  COG_LICENSE_FILES = COPYING
> >  COG_CONF_OPTS = \
> > @@ -19,8 +19,6 @@ COG_CONF_OPTS = \
> >  	-Dcog_home_uri='$(call qstrip,$(BR2_PACKAGE_COG_PROGRAMS_HOME_URI))' \
> >  	-Dplatforms='$(subst $(space),$(comma),$(strip $(COG_PLATFORMS_LIST)))'
> >  
> > -COG_PLATFORMS_LIST = headless
> > -
> >  ifeq ($(BR2_PACKAGE_WESTON),y)
> >  COG_CONF_OPTS += -Dwayland_weston_direct_display=true
> >  COG_DEPENDENCIES += weston
> > @@ -28,6 +26,11 @@ else
> >  COG_CONF_OPTS += -Dwayland_weston_direct_display=false
> >  endif
> >  
> > +ifeq ($(BR2_PACKAGE_COG_PLATFORM_HEADLESS),y)
> > +COG_PLATFORMS_LIST += headless
> > +COG_DEPENDENCIES += wpebackend-fdo
> > +endif
> > +
> >  ifeq ($(BR2_PACKAGE_COG_PLATFORM_FDO),y)
> >  COG_PLATFORMS_LIST += wayland
> >  COG_DEPENDENCIES += libxkbcommon wayland-protocols
> > @@ -48,4 +51,8 @@ ifeq ($(BR2_PACKAGE_LIBMANETTE),y)
> >  COG_DEPENDENCIES += libmanette
> >  endif
> >  
> > +ifeq ($(BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO),y)
> > +COG_DEPENDENCIES += wpebackend-fdo
> > +endif
> 
> Drop this, and instead do:
> 
> ifeq ($(BR2_PACKAGE_COG_PLATFORM_HEADLESS),y)
> COG_PLATFORMS_LIST += headless
> COG_DEPENDENCIES += wpebackend-fdo
> endif
> 
> ifeq ($(BR2_PACKAGE_COG_PLATFORM_FDO),y)
> COG_PLATFORMS_LIST += wayland
> COG_DEPENDENCIES += libxkbcommon wayland-protocols wpebackend-fdo
> endif
> 
> ifeq ($(BR2_PACKAGE_COG_PLATFORM_DRM),y)
> COG_PLATFORMS_LIST += drm
> COG_DEPENDENCIES += libdrm libinput libgbm libegl udev wpebackend-fdo
> endif

With the additional BR2_PACKAGE_COG_NEEDS_WPEBACKEND_FDO configuration
symbol I was trying to avoid adding wpebackend-fdo more than once to
COG_DEPENDENCIES. If it's fine to have it there (as it seems to be the
case) then yes, I am aboard for all the simplifications.
 
> Could you rework a v3 according to those suggestions?

Yes, will re-spin this into a v3 at some point. Thanks for all the
recommendations!

Cheers,
—Adrián

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot