Re: [Buildroot] [PATCH v2 2/4] package/mpd: add/enhance (kconfig + code) comments

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Andreas,

On Wed,  5 Oct 2022 11:10:30 +0200
Andreas Ziegler <br015@umbiko.net> wrote:

> Align kconfig comments with descriptions in the mpd manual.
> Add a kconfig comment to highlight the impact of ogg/vorbis selection.
> Add comments to makefile to explain remaining multiple dependency creation.
> 
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
> ---
> Changes v1 -> v2:
>  - make this a separate patch 
> 
>  package/mpd/Config.in | 9 ++++++---
>  package/mpd/mpd.mk    | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/package/mpd/Config.in b/package/mpd/Config.in
> index 8f0af7b2d3..2606008e90 100644
> --- a/package/mpd/Config.in
> +++ b/package/mpd/Config.in
> @@ -33,7 +33,7 @@ config BR2_PACKAGE_MPD_SQLITE
>  	select BR2_PACKAGE_SQLITE
>  	help
>  	  Enable sqlite database support.
> -	  If you don't use sqlite it will use an ASCII database.

Why is this removed?

> +	  This is mandatory for the sticker database.

Not sure what the sticker database is :-)

>  
>  config BR2_PACKAGE_MPD_ZZIP
>  	bool "zzip"
> @@ -81,8 +81,8 @@ comment "Decoder plugins"
>  config BR2_PACKAGE_MPD_DSD
>  	bool "dsd"
>  	help
> -	  Enable Digital Speech Decoder (DSD) support to play audio
> -	  files encoded in a digital speech format.
> +	  Direct Stream Digital (DSD) support to play audio
> +	  files encoded in single bit format.

Is this change really relevant ?

>  
>  config BR2_PACKAGE_MPD_FAAD2
>  	bool "faad2"
> @@ -210,6 +210,9 @@ config BR2_PACKAGE_MPD_TWOLAME
>  	help
>  	  Enable TwoLAME mp2 encoding.
>  
> +comment "for ogg/vorbis encoding enable vorbis decoder"
> +	depends on !BR2_PACKAGE_MPD_VORBIS

I don't understand why this comment is needed. We usually don't put
comments about all dependencies on other packages. Why for this one?

If you want to play Ogg/Vorbis files, it's quite obvious that the
"vorbis" option needs to be enabled.

> +# opus needs to be encapsulated in a container format, here ogg

Yes, and?

>  ifeq ($(BR2_PACKAGE_MPD_OPUS),y)
>  MPD_DEPENDENCIES += opus libogg
>  MPD_CONF_OPTS += -Dopus=enabled
> @@ -317,6 +318,7 @@ else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
>  MPD_CONF_OPTS += -Dupnp=disabled
>  endif
>  
> +# handle decoder and encoder simultaneously

Yes, and?

Sorry, but I don't really understand the value of the changes proposed
in this commit. I'll mark it as Changes Requested for the time being.
Feel free to resubmit with more details if needed. Also, look at other
Buildroot packages, we try to do things consistently, so doing
something "special" in MPD is unlikely to be accepted.

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