Re: [Buildroot] [PATCH 1/1] package/mpd: update to version 0.23.2

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello Andreas,

Thanks for this patch. I have some comments below, but more
importantly, I would really like to get the review/approval from Jörg
Krause on this patch, as he has usually been maintaining this package.

On Mon, 25 Oct 2021 11:44:53 +0200
Andreas Ziegler <br015@umbiko.net> wrote:

> In addition to various bug fixes, mpd version 0.23 introduces a new 
> dependency (mft) and a change in the configuration of the UPnP client 
> functionality. 
> 
> Optional new features (openmpt decoder, pipewire and snapcast outputs) are 
> currently not available as Buildroot packages and were not considered. 

We do have pipewire as a package in Buildroot, so that aspect doesn't
seem completely true. If you don't handle those dependencies in mpd.mk
(which is fine), could you disable them explicitly by passing the
appropriate -Dfoo=disabled, if available?

> The change log can be found in [1]
> 
> Introduce new dependency for fmt library. Change configuration of UPnP plugin 
> to submenu, allowing to optionally select libupnp, libnpupnp or disabled. The 
> default setting is 'no UPnP client functionality'. Correct a typo in toolchain 
> dependency comment. Adapt sparc patch to changes in source layout. 

The typo in the toolchain dependency comment should be addressed in a
separate patch, as it is really an unrelated bug fix.


> diff --git a/package/mpd/Config.in b/package/mpd/Config.in
> index 7a2597558b..7b5aeb4bf6 100644
> --- a/package/mpd/Config.in
> +++ b/package/mpd/Config.in
> @@ -8,6 +8,7 @@ menuconfig BR2_PACKAGE_MPD
>  	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_8 # C++17
>  	depends on BR2_HOST_GCC_AT_LEAST_8 # C++17
>  	select BR2_PACKAGE_BOOST
> +	select BR2_PACKAGE_FMT

You're adding this here, but it's not added in MPD_DEPENDENCIES. Is fmt
a build-time or a run-time dependency?

You can check this by doing "make clean && make mpd" and verify that it
does build.

>  	select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE
>  	help
>  	  MPD is a flexible, powerful, server-side application
> @@ -361,7 +362,7 @@ config BR2_PACKAGE_MPD_LIBMPDCLIENT
>  
>  config BR2_PACKAGE_MPD_NEIGHBOR_DISCOVERY_SUPPORT
>  	bool "neighbor discovery support"
> -	depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || BR2_PACKAGE_MPD_UPNP
> +	depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || !BR2_PACKAGE_MPD_UPNP_DISABLED
>  	help
>  	  Enable support for neighbor discovery.
>  	  This option can be used in conjunction with the smbclient
> @@ -380,13 +381,39 @@ config BR2_PACKAGE_MPD_TCP
>  	  You want this on if MPD and the client(s) work
>  	  on different machines (the usual scenario).
>  
> -config BR2_PACKAGE_MPD_UPNP
> -	bool "UPnP"
> +choice

Kind of annoying to have a "choice" here. Is there a good reason to
support both uPNP implementations?

> +	prompt "UPnP"
> +	default BR2_PACKAGE_MPD_UPNP_DISABLED
> +	help
> +	  Enable MPD UPnP client support.
> +
> +config BR2_PACKAGE_MPD_UPNP_PUPNP
> +	bool "pupnp"
>  	select BR2_PACKAGE_EXPAT
>  	select BR2_PACKAGE_LIBUPNP
>  	select BR2_PACKAGE_MPD_CURL
>  	help
> -	  Enable MPD UPnP client support.
> +	  Provides UPnP functionality through libupnp
> +	  (the legacy Portable SDK for UPnP devices).
> +	  
> +	  Introduces least additional dependencies.
> +
> +config BR2_PACKAGE_MPD_UPNP_NPUPNP
> +	bool "npupnp"
> +	select BR2_PACKAGE_LIBNPUPNP
> +	select BR2_PACKAGE_MPD_CURL
> +	help
> +	  Provides UPnP functionality through libnpupnp
> +	  (a C++ reimplementation of the Portable UPnP library).
> +	  
> +	  Prefer this option if you plan to use upmpdcli.
> +
> +config BR2_PACKAGE_MPD_UPNP_DISABLED

I think I would prefer BR2_PACKAGE_MPD_UPNP_NONE.

Thanks a lot!

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