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

[Andreas Ziegler]

Hi Thomas,

thank you for taking a look at this patch. Some answers to your remarks 
below; I will also prepare a v2 of the change.

I believe a legacy entry for BR2_PACKAGE_MPD_UPNP is also necessary ...

On 2021-11-14 15:08, Thomas Petazzoni wrote:
> 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 at 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?
> 

package/pipewire is the server component - unfortunately also the only 
provider of libpipewire, which is needed here. Installing the complete 
package would probably satisfy the dependency, but in an embedded 
context it would be better to break pipewire into two components, 
libpipewire and pipewired. Since I have no use for this feature, I would 
prefer to disable it; I will also re-phrase the commit message.

>> 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.
> 

OK

>> 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.
> 

It is both, a build- and a runtime dependency. Probably works only 
because of alphabetical sorting of packages ...

>>  	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?
> 

Currently just one: there is a choice, there is no 'correct' option, so 
defer the decision to the user. libnpupnp is the more modern 
implementation, but impossible to configure: config options exist, but 
break the build, because they are not completely implemented. Buildroot 
therefore does a full-featured install, which uses more space than 
libpupnp and also brings features not needed in a client context (e.g. a 
web server). Since this raises additional security concerns, I would 
prefer the older library (which is client-only) - unless npupnp solves 
an issue or another package installs it anyway; currently  upmpdcli does 
this. On the other hand, having both UPnP front-end and back-end 
integration is not really a valid use case ...

At that point I gave up and implemented the choice. I will put some more 
information into the Config.in help texts.

>> +	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.

Good solution :-D The help texts are too technical, I will try to 
provide some more information here.

Kind regards,
Andreas

> 
> Thanks a lot!
> 
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com