[Buildroot] [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages

[Buildroot] [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages

[Andreas Ziegler]

Background
During configuration, the meson build system tries to determine the 
availability of optional dependencies using (host) pkgconfig and cmake in that
order. If a library does not exist on the target, pkg-config will fail, but
cmake sometimes finds and reports libraries that exist as host packages. This
has been observed for host-expat (cmake dependency) and host-zlib. The link 
step subsequently fails, because necessary files are not present in the target 
architecture.

Unconditionally disable optional features often found in host binaries and
modify the menu selection processing in mpd.mk to re-enable them where
necessary. Currently this concerns expat and zlib only. 

This fixes the following build errors:

[expat]
/home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: warning: libc.so.6, needed by /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try using -rpath or -rpath-link)
/home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined reference to `__errno_location@GLIBC_2.2.5'

[zlib]
http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/

Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
 package/mpd/mpd.mk | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
index 12da36098f..4e67c9428c 100644
--- a/package/mpd/mpd.mk
+++ b/package/mpd/mpd.mk
@@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING
 # these refer to the FreeBSD PPP daemon
 MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466
 MPD_SELINUX_MODULES = mpd
+# These features are either unwanted or not selectable via the Buildroot menu
 MPD_CONF_OPTS = \
 	-Daudiofile=disabled \
 	-Ddocumentation=disabled \
@@ -21,6 +22,12 @@ MPD_CONF_OPTS = \
 	-Dpipewire=disabled \
 	-Dsnapcast=false
 
+# Explicitly disable features where meson's dependency detection picks up host
+# libraries. These settings can be overridden through menu options later
+MPD_CONF_OPTS += \
+	-Dexpat=disabled \
+	-Dzlib=disabled
+
 # Zeroconf support depends on libdns_sd from avahi.
 ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y)
 MPD_DEPENDENCIES += avahi
@@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y)
 MPD_DEPENDENCIES += \
 	expat \
 	libupnp
-MPD_CONF_OPTS += -Dupnp=pupnp
+MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled
 else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y)
 MPD_DEPENDENCIES += \
 	libnpupnp
-MPD_CONF_OPTS += -Dupnp=npupnp
+MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled
 else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
 MPD_CONF_OPTS += -Dupnp=disabled
 endif
-- 
2.34.1

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

Re: [Buildroot] [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages

[Arnout Vandecappelle]

On 31/03/2022 15:22, Andreas Ziegler wrote:
> Background
> During configuration, the meson build system tries to determine the
> availability of optional dependencies using (host) pkgconfig and cmake in that
> order. If a library does not exist on the target, pkg-config will fail, but
> cmake sometimes finds and reports libraries that exist as host packages. This
> has been observed for host-expat (cmake dependency) and host-zlib. The link
> step subsequently fails, because necessary files are not present in the target
> architecture.
> 
> Unconditionally disable optional features often found in host binaries and
> modify the menu selection processing in mpd.mk to re-enable them where
> necessary. Currently this concerns expat and zlib only.
> 
> This fixes the following build errors:
> 
> [expat]
> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: warning: libc.so.6, needed by /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try using -rpath or -rpath-link)
> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined reference to `__errno_location@GLIBC_2.2.5'
> 
> [zlib]
> http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/
> 
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
> ---
>   package/mpd/mpd.mk | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
> index 12da36098f..4e67c9428c 100644
> --- a/package/mpd/mpd.mk
> +++ b/package/mpd/mpd.mk
> @@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING
>   # these refer to the FreeBSD PPP daemon
>   MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466
>   MPD_SELINUX_MODULES = mpd
> +# These features are either unwanted or not selectable via the Buildroot menu
>   MPD_CONF_OPTS = \
>   	-Daudiofile=disabled \
>   	-Ddocumentation=disabled \
> @@ -21,6 +22,12 @@ MPD_CONF_OPTS = \
>   	-Dpipewire=disabled \
>   	-Dsnapcast=false
>   
> +# Explicitly disable features where meson's dependency detection picks up host
> +# libraries. These settings can be overridden through menu options later
> +MPD_CONF_OPTS += \
> +	-Dexpat=disabled \
> +	-Dzlib=disabled

  It would be better to make these automatic conditional dependencies, like it's 
done for ICU.

> +
>   # Zeroconf support depends on libdns_sd from avahi.
>   ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y)
>   MPD_DEPENDENCIES += avahi
> @@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y)
>   MPD_DEPENDENCIES += \
>   	expat \

  That would also allow to remove this (it will be done implicitly since expat 
is selected in Config.in and the automatic dependency will add it).

>   	libupnp
> -MPD_CONF_OPTS += -Dupnp=pupnp
> +MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled
>   else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y)
>   MPD_DEPENDENCIES += \
>   	libnpupnp
> -MPD_CONF_OPTS += -Dupnp=npupnp
> +MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled

  This is wrong: no dependency on expat is added so expat may not even be 
selected or built at this point.

  Marked as Changes Requested.

  Regards,
  Arnout

>   else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
>   MPD_CONF_OPTS += -Dupnp=disabled
>   endif
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages

[Andreas Ziegler]

Hi Arnout,

Thank you for taking a look.

On 2022-04-04 17:38, Arnout Vandecappelle wrote:
> On 31/03/2022 15:22, Andreas Ziegler wrote:
>> Background
>> During configuration, the meson build system tries to determine the
>> availability of optional dependencies using (host) pkgconfig and cmake 
>> in that
>> order. If a library does not exist on the target, pkg-config will 
>> fail, but
>> cmake sometimes finds and reports libraries that exist as host 
>> packages. This
>> has been observed for host-expat (cmake dependency) and host-zlib. The 
>> link
>> step subsequently fails, because necessary files are not present in 
>> the target
>> architecture.
>> 
>> Unconditionally disable optional features often found in host binaries 
>> and
>> modify the menu selection processing in mpd.mk to re-enable them where
>> necessary. Currently this concerns expat and zlib only.
>> 
>> This fixes the following build errors:
>> 
>> [expat]
>> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: 
>> warning: libc.so.6, needed by 
>> /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try 
>> using -rpath or -rpath-link)
>> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: 
>> /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined 
>> reference to `__errno_location@GLIBC_2.2.5'
>> 
>> [zlib]
>> http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/
>> 
>> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
>> ---
>>   package/mpd/mpd.mk | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
>> index 12da36098f..4e67c9428c 100644
>> --- a/package/mpd/mpd.mk
>> +++ b/package/mpd/mpd.mk
>> @@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING
>>   # these refer to the FreeBSD PPP daemon
>>   MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466
>>   MPD_SELINUX_MODULES = mpd
>> +# These features are either unwanted or not selectable via the 
>> Buildroot menu
>>   MPD_CONF_OPTS = \
>>   	-Daudiofile=disabled \
>>   	-Ddocumentation=disabled \
>> @@ -21,6 +22,12 @@ MPD_CONF_OPTS = \
>>   	-Dpipewire=disabled \
>>   	-Dsnapcast=false
>>   +# Explicitly disable features where meson's dependency detection 
>> picks up host
>> +# libraries. These settings can be overridden through menu options 
>> later
>> +MPD_CONF_OPTS += \
>> +	-Dexpat=disabled \
>> +	-Dzlib=disabled
> 
>  It would be better to make these automatic conditional dependencies,
> like it's done for ICU.

Good idea, but inclusion of ICU should be initiated by Config.in, like 
every other feature.

>> +
>>   # Zeroconf support depends on libdns_sd from avahi.
>>   ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y)
>>   MPD_DEPENDENCIES += avahi
>> @@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y)
>>   MPD_DEPENDENCIES += \
>>   	expat \
> 
>  That would also allow to remove this (it will be done implicitly
> since expat is selected in Config.in and the automatic dependency will
> add it).
> 
>>   	libupnp
>> -MPD_CONF_OPTS += -Dupnp=pupnp
>> +MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled
>>   else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y)
>>   MPD_DEPENDENCIES += \
>>   	libnpupnp
>> -MPD_CONF_OPTS += -Dupnp=npupnp
>> +MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled
> 
>  This is wrong: no dependency on expat is added so expat may not even
> be selected or built at this point.

Actually, the dependency is inherited from libnpupnp, but you are right, 
this handling is not intuitive.

>  Marked as Changes Requested.

Outline of the proposed next version of this change:

[I did not want to make so many changes, but you are right, it makes 
sense.]

Separate logic from implementation: Remove feature dependencies from 
mpd.mk and put selection logic into Config.in exclusively.

In the makefile, a feature is responsible only to select or deselect its 
configure option and, if this feature relies on a library, add a 
dependency on this.

Duplicated selection logic in mpd.mk will be eliminated. A build 
dependency will only be added if it is present in the mpd configuration, 
and not be inherited accidentally from concurrent packages.

This impacts the following features: expat, id3tag, yajl. id3tag already 
is half implemented, but not used consistently (has an option entry in 
Config.in, but does not select this, but handles dependencies 
individually). expat and yajl would be created as hidden options without 
visibility (no user interaction necessary).

Separate vorbis decoder and encoder options; currently the decoder 
feature BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and 
ogg/vorbis encoding, which is probably not intended.

Create entry 'unicode' for ICU in Config.in.

Set zlib to disabled (not used currently).

Thoughts?

Kind regards,
Andreas

>  Regards,
>  Arnout
> 
>>   else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
>>   MPD_CONF_OPTS += -Dupnp=disabled
>>   endif
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] User-visible Config.in feature sub-options [was: Re: [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages]

[Arnout Vandecappelle]

  [Putting the other maintainers in Cc here because there's a bit of a 
policy/philosophy decision.]

On 08/04/2022 15:22, Andreas Ziegler wrote:
> Hi Arnout,
> 
>
[snip]
> Outline of the proposed next version of this change:
> 
> [I did not want to make so many changes, but you are right, it makes sense.]
> 
> Separate logic from implementation: Remove feature dependencies from mpd.mk and 
> put selection logic into Config.in exclusively.

  As you can see from the description above: this is making things quite 
complicated. And one of the tenets of Buildroot is to keep things simple.

  That is the reason why we generally want to avoid sub-options in Config.in. 
Basically we only want sub-options in the following cases:

- It makes a big difference in the final target installed size if the option is 
kept disabled. E.g. BR2_PACKAGE_AVAHI_DAEMON.

- There are multiple possibilities and depending on the circumstances, one can 
be more appropriate than the other. E.g. libcurl SSL/TLS library to use.

- It is not obvious which other package needs to be enabled to enable a feature. 
E.g. BR2_PACKAGE_BLUEZ_ALSA_HCITOP.

- The package has many sub-features with various dependencies, it is typically a 
major top-level package (i.e. something that the user wants explicitly and is 
not pulled in by something else), and the sub-features are user visible.

The mpd sub-options fall in the latter category. It's the most tricky one to 
evaluate because it pulls in complexity with a difficult trade-off. It's also 
the one I dislike most, because it's very subjective. A way to make it more 
objective is to make a user-visible option for each config option that the 
package provides. However, I think that easily leads to an unnecessary explosion 
of Config.in options with no benefit for the user and possibly causing 
confusion. And it's not necessarily that simple either, cfr. mesa3d (though in 
that case it's going to be complicated however it's approached).

  We don't have a consistent policy for this case, but I believe the policy 
should be:

- Add Config.in options only for features that are important, meaningful for the 
user (e.g. codec support).

- Add Config.in options only for features that have a size impact (usually due 
to the dependencies they pull in).

- In the .mk file, don't use the Config.in options but instead use automatic 
dependencies only.

That way, the .mk file is kept simple (no problematic cases like the 
libupnp/expat interaction in this patch). The Config.in is a bit complicated, 
but it doesn't explode.

  Bottom line: I think it's actually the reverse that needs to be done.


> In the makefile, a feature is responsible only to select or deselect its 
> configure option and, if this feature relies on a library, add a dependency on 
> this.

  So I think the .mk file should simply contain things like:

ifeq ($(BR2_PACKAGE_LIBVORBIS),y)
MPD_DEPENDENCIES += libvorbis
MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
else
MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled
endif

and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting 
BR2_PACKAGE_LIBVORBIS.


> Duplicated selection logic in mpd.mk will be eliminated. A build dependency will 
> only be added if it is present in the mpd configuration, and not be inherited 
> accidentally from concurrent packages.

  So this is exactly the reverse of what I'd want. I think it makes the .mk file 
harder to maintain for no practical gain.


> This impacts the following features: expat, id3tag, yajl. id3tag already is half 
> implemented, but not used consistently (has an option entry in Config.in, but 
> does not select this, but handles dependencies individually). expat and yajl 
> would be created as hidden options without visibility (no user interaction 
> necessary).

  Yes, that would indeed be an alternative to keep the .mk file simpler. I take 
it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets selected by 
the other MPD options that rely on expat. This indeed simplifies things, but it 
is still a bit more complicated than what I propose.

> Separate vorbis decoder and encoder options; currently the decoder feature 
> BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis encoding, 
> which is probably not intended.

  I don't see a reason why you would want only one or the other. It has 
virtually no impact on size.


  In summary, I think we have the following three options for packages where we 
decide we want user-visible sub-options.

- 1-to-1 mapping with the package configure options. Interaction between the 
options is expressed with select/depends in Config.in. The .mk file simply maps 
the Config.in options. If it's really not relevant for the user, a Config.in 
option can be made blind. This is what Andreas proposes.

- Only automatic dependencies in the .mk file, except in cases where it has an 
important size or behaviour impact. Add Config.in options only in case it is 
relevant. This is what I propose.

- Add Config.in options for important features. Express interdependencies 
between package configure options in the .mk file. This is the current situation 
for mpd.


  Thinking more about it, Andreas' proposal does have an attractive kind of 
elegance about it, which gives it simplicity even though it is more lines of code.

  So let's see what the other maintainers think. If you (or the other 
maintainers) don't agree, we can compromise and go back to the original patch 
(with just the npupnp/expat situation resolved).

  Ideally I'd like the maintainers (and anybody else, really) to come to a 
decision here and eventually formalize it in the manual. Because it's not the 
first time this discussion crops up.


  Regards,
  Arnout


> Create entry 'unicode' for ICU in Config.in.
> 
> Set zlib to disabled (not used currently).
> 
> Thoughts?
> 
> Kind regards,
> Andreas
> 
>>  Regards,
>>  Arnout
>>
>>>   else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
>>>   MPD_CONF_OPTS += -Dupnp=disabled
>>>   endif
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] User-visible Config.in feature sub-options [was: Re: [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages]

[Yann E. MORIN]

Arnout, Andreas, All,

On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly:
> On 08/04/2022 15:22, Andreas Ziegler wrote:
> >Outline of the proposed next version of this change:
> >[I did not want to make so many changes, but you are right, it makes sense.]
> >Separate logic from implementation: Remove feature dependencies from
> >mpd.mk and put selection logic into Config.in exclusively.
>  As you can see from the description above: this is making things quite
> complicated. And one of the tenets of Buildroot is to keep things simple.
[--SNIP--]
>  We don't have a consistent policy for this case, but I believe the policy
> should be:
> 
> - Add Config.in options only for features that are important, meaningful for
> the user (e.g. codec support).
> 
> - Add Config.in options only for features that have a size impact (usually
> due to the dependencies they pull in).
> 
> - In the .mk file, don't use the Config.in options but instead use automatic
> dependencies only.

That would be very confusign from a user perspective: they would not
enable a feature of a package, yet the package would have that feature
enabled if th3e depedency is enabled.

Besides the technical surprise, this could also lead to licensing issues
if the combination of the two packages require special handling (e.g.
because the library license propagates top the package).

So, the settings from Config.in must be abode by.

> That way, the .mk file is kept simple (no problematic cases like the
> libupnp/expat interaction in this patch). The Config.in is a bit
> complicated, but it doesn't explode.
> 
>  Bottom line: I think it's actually the reverse that needs to be done.

Err. I don;'t understand what you meant here... :-(

> >In the makefile, a feature is responsible only to select or deselect its
> >configure option and, if this feature relies on a library, add a
> >dependency on this.
> 
>  So I think the .mk file should simply contain things like:
> 
> ifeq ($(BR2_PACKAGE_LIBVORBIS),y)
> MPD_DEPENDENCIES += libvorbis
> MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
> else
> MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled
> endif
> 
> and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting
> BR2_PACKAGE_LIBVORBIS.

I highly disagree; the conditional should be on the package setting, not
the dependency.

> >Duplicated selection logic in mpd.mk will be eliminated. A build
> >dependency will only be added if it is present in the mpd configuration,
> >and not be inherited accidentally from concurrent packages.
>  So this is exactly the reverse of what I'd want. I think it makes the .mk
> file harder to maintain for no practical gain.

Yet, the proposal by Andreas is I believe the correct way to handle the
situation.

> >This impacts the following features: expat, id3tag, yajl. id3tag already
> >is half implemented, but not used consistently (has an option entry in
> >Config.in, but does not select this, but handles dependencies
> >individually). expat and yajl would be created as hidden options without
> >visibility (no user interaction necessary).
>  Yes, that would indeed be an alternative to keep the .mk file simpler. I
> take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets
> selected by the other MPD options that rely on expat. This indeed simplifies
> things, but it is still a bit more complicated than what I propose.

But more correct.

> >Separate vorbis decoder and encoder options; currently the decoder feature
> >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis
> >encoding, which is probably not intended.
>  I don't see a reason why you would want only one or the other. It has
> virtually no impact on size.

Indeed here, vorbis support should just enable both the encoder/decoder.

The only reason that we might want to be able to chose, is if enabling
one or the other have different requirements: extra size requirements,
extra dependencies, legal issues in your jurisdiction, etc...

>  In summary, I think we have the following three options for packages where
> we decide we want user-visible sub-options.
> 
> - 1-to-1 mapping with the package configure options. Interaction between the
> options is expressed with select/depends in Config.in. The .mk file simply
> maps the Config.in options. If it's really not relevant for the user, a
> Config.in option can be made blind. This is what Andreas proposes.

I am OK with that.

> - Only automatic dependencies in the .mk file, except in cases where it has
> an important size or behaviour impact. Add Config.in options only in case it
> is relevant. This is what I propose.

I am OK with the principle, but this does not look like what you
proposed above, as the build-dependencies would be on the dependency
being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs.
BR2_PACKAGE_LIBVORBIS as you showed above).

> - Add Config.in options for important features. Express interdependencies
> between package configure options in the .mk file. This is the current
> situation for mpd.

I don;t see a difference here: "Add Config.in options only in case it is
relevant" and "Add Config.in options for important features" are exactly
the same in mny eyes...

>  So let's see what the other maintainers think. If you (or the other
> maintainers) don't agree, we can compromise and go back to the original
> patch (with just the npupnp/expat situation resolved).

So, I'll summariser my position:

  - add Config.in options when it makes sense:
    - important size delta
    - legal issues
    those options select the appropriate packages

  - in the .mk:
      - add conditional blocks based on those options, add build
        dependencies as appropriate;
      - add conditoinal dependencies on package for automatic
        dependencies

>  Ideally I'd like the maintainers (and anybody else, really) to come to a
> decision here and eventually formalize it in the manual. Because it's not
> the first time this discussion crops up.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] User-visible Config.in feature sub-options [was: Re: [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages]

[Andreas Ziegler]

Hi Yann, Arnout, and whoever else is interested,

two additional thoughts from my side below:

On 2022-04-09 16:09, Yann E. MORIN wrote:
> Arnout, Andreas, All,
> 
> On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly:
>> On 08/04/2022 15:22, Andreas Ziegler wrote:
>> >Outline of the proposed next version of this change:
>> >[I did not want to make so many changes, but you are right, it makes sense.]
>> >Separate logic from implementation: Remove feature dependencies from
>> >mpd.mk and put selection logic into Config.in exclusively.
>>  As you can see from the description above: this is making things 
>> quite
>> complicated. And one of the tenets of Buildroot is to keep things 
>> simple.
> [--SNIP--]
>>  We don't have a consistent policy for this case, but I believe the 
>> policy
>> should be:
>> 
>> - Add Config.in options only for features that are important, 
>> meaningful for
>> the user (e.g. codec support).
>> 
>> - Add Config.in options only for features that have a size impact 
>> (usually
>> due to the dependencies they pull in).
>> 
>> - In the .mk file, don't use the Config.in options but instead use 
>> automatic
>> dependencies only.
> 
> That would be very confusign from a user perspective: they would not
> enable a feature of a package, yet the package would have that feature
> enabled if th3e depedency is enabled.
> 
> Besides the technical surprise, this could also lead to licensing 
> issues
> if the combination of the two packages require special handling (e.g.
> because the library license propagates top the package).

The security aspect increasingly becomes more important: every line of 
unused code is a potential attack vector. Better disable what you do not 
need.

> So, the settings from Config.in must be abode by.
> 
>> That way, the .mk file is kept simple (no problematic cases like the
>> libupnp/expat interaction in this patch). The Config.in is a bit
>> complicated, but it doesn't explode.
>> 
>>  Bottom line: I think it's actually the reverse that needs to be done.
> 
> Err. I don;'t understand what you meant here... :-(
> 
>> >In the makefile, a feature is responsible only to select or deselect its
>> >configure option and, if this feature relies on a library, add a
>> >dependency on this.
>> 
>>  So I think the .mk file should simply contain things like:
>> 
>> ifeq ($(BR2_PACKAGE_LIBVORBIS),y)
>> MPD_DEPENDENCIES += libvorbis
>> MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
>> else
>> MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled
>> endif
>> 
>> and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting
>> BR2_PACKAGE_LIBVORBIS.
> 
> I highly disagree; the conditional should be on the package setting, 
> not
> the dependency.
> 
>> >Duplicated selection logic in mpd.mk will be eliminated. A build
>> >dependency will only be added if it is present in the mpd configuration,
>> >and not be inherited accidentally from concurrent packages.
>>  So this is exactly the reverse of what I'd want. I think it makes the 
>> .mk
>> file harder to maintain for no practical gain.
> 
> Yet, the proposal by Andreas is I believe the correct way to handle the
> situation.
> 
>> >This impacts the following features: expat, id3tag, yajl. id3tag already
>> >is half implemented, but not used consistently (has an option entry in
>> >Config.in, but does not select this, but handles dependencies
>> >individually). expat and yajl would be created as hidden options without
>> >visibility (no user interaction necessary).
>>  Yes, that would indeed be an alternative to keep the .mk file 
>> simpler. I
>> take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets
>> selected by the other MPD options that rely on expat. This indeed 
>> simplifies
>> things, but it is still a bit more complicated than what I propose.
> 
> But more correct.
> 
>> >Separate vorbis decoder and encoder options; currently the decoder feature
>> >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis
>> >encoding, which is probably not intended.
>>  I don't see a reason why you would want only one or the other. It has
>> virtually no impact on size.
> 
> Indeed here, vorbis support should just enable both the 
> encoder/decoder.
> 
> The only reason that we might want to be able to chose, is if enabling
> one or the other have different requirements: extra size requirements,
> extra dependencies, legal issues in your jurisdiction, etc...

This behavior is not evident from the menu. Here you just have an option 
to enable the decoder; the encoder is not mentioned anywhere. If the 
configuration stays, at least grouping and help message of the current 
item need an update.

>>  In summary, I think we have the following three options for packages 
>> where
>> we decide we want user-visible sub-options.
>> 
>> - 1-to-1 mapping with the package configure options. Interaction 
>> between the
>> options is expressed with select/depends in Config.in. The .mk file 
>> simply
>> maps the Config.in options. If it's really not relevant for the user, 
>> a
>> Config.in option can be made blind. This is what Andreas proposes.
> 
> I am OK with that.
> 
>> - Only automatic dependencies in the .mk file, except in cases where 
>> it has
>> an important size or behaviour impact. Add Config.in options only in 
>> case it
>> is relevant. This is what I propose.
> 
> I am OK with the principle, but this does not look like what you
> proposed above, as the build-dependencies would be on the dependency
> being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs.
> BR2_PACKAGE_LIBVORBIS as you showed above).
> 
>> - Add Config.in options for important features. Express 
>> interdependencies
>> between package configure options in the .mk file. This is the current
>> situation for mpd.
> 
> I don;t see a difference here: "Add Config.in options only in case it 
> is
> relevant" and "Add Config.in options for important features" are 
> exactly
> the same in mny eyes...
> 
>>  So let's see what the other maintainers think. If you (or the other
>> maintainers) don't agree, we can compromise and go back to the 
>> original
>> patch (with just the npupnp/expat situation resolved).
> 
> So, I'll summariser my position:
> 
>   - add Config.in options when it makes sense:
>     - important size delta
>     - legal issues
>     those options select the appropriate packages
> 
>   - in the .mk:
>       - add conditional blocks based on those options, add build
>         dependencies as appropriate;
>       - add conditoinal dependencies on package for automatic
>         dependencies
> 
>>  Ideally I'd like the maintainers (and anybody else, really) to come 
>> to a
>> decision here and eventually formalize it in the manual. Because it's 
>> not
>> the first time this discussion crops up.

Kind regards,
Andreas

> Regards,
> Yann E. MORIN.
> 
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' 
> conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___        
>        |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There 
> is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   
> conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] User-visible Config.in feature sub-options [was: Re: [PATCH 1/1] package/mpd: explicitly disable features to avoid collision with host packages]

[Andreas Ziegler]

Hi Yann, Arnout, *,

I would like to postpone this change for the moment; the root cause for 
the addressed build failures lies within the meson build system (patch 
coming) and fixing this will make some of the proposed modifications 
unnecessary.

I would still like to implement the selection logic changes, because I 
started to notice some inconsistencies and also missing features.

Kind regards,
Andreas


On 2022-04-09 16:09, Yann E. MORIN wrote:
> Arnout, Andreas, All,
> 
> On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly:
>> On 08/04/2022 15:22, Andreas Ziegler wrote:
>> >Outline of the proposed next version of this change:
>> >[I did not want to make so many changes, but you are right, it makes sense.]
>> >Separate logic from implementation: Remove feature dependencies from
>> >mpd.mk and put selection logic into Config.in exclusively.
>>  As you can see from the description above: this is making things 
>> quite
>> complicated. And one of the tenets of Buildroot is to keep things 
>> simple.
> [--SNIP--]
>>  We don't have a consistent policy for this case, but I believe the 
>> policy
>> should be:
>> 
>> - Add Config.in options only for features that are important, 
>> meaningful for
>> the user (e.g. codec support).
>> 
>> - Add Config.in options only for features that have a size impact 
>> (usually
>> due to the dependencies they pull in).
>> 
>> - In the .mk file, don't use the Config.in options but instead use 
>> automatic
>> dependencies only.
> 
> That would be very confusign from a user perspective: they would not
> enable a feature of a package, yet the package would have that feature
> enabled if th3e depedency is enabled.
> 
> Besides the technical surprise, this could also lead to licensing 
> issues
> if the combination of the two packages require special handling (e.g.
> because the library license propagates top the package).
> 
> So, the settings from Config.in must be abode by.
> 
>> That way, the .mk file is kept simple (no problematic cases like the
>> libupnp/expat interaction in this patch). The Config.in is a bit
>> complicated, but it doesn't explode.
>> 
>>  Bottom line: I think it's actually the reverse that needs to be done.
> 
> Err. I don;'t understand what you meant here... :-(
> 
>> >In the makefile, a feature is responsible only to select or deselect its
>> >configure option and, if this feature relies on a library, add a
>> >dependency on this.
>> 
>>  So I think the .mk file should simply contain things like:
>> 
>> ifeq ($(BR2_PACKAGE_LIBVORBIS),y)
>> MPD_DEPENDENCIES += libvorbis
>> MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
>> else
>> MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled
>> endif
>> 
>> and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting
>> BR2_PACKAGE_LIBVORBIS.
> 
> I highly disagree; the conditional should be on the package setting, 
> not
> the dependency.
> 
>> >Duplicated selection logic in mpd.mk will be eliminated. A build
>> >dependency will only be added if it is present in the mpd configuration,
>> >and not be inherited accidentally from concurrent packages.
>>  So this is exactly the reverse of what I'd want. I think it makes the 
>> .mk
>> file harder to maintain for no practical gain.
> 
> Yet, the proposal by Andreas is I believe the correct way to handle the
> situation.
> 
>> >This impacts the following features: expat, id3tag, yajl. id3tag already
>> >is half implemented, but not used consistently (has an option entry in
>> >Config.in, but does not select this, but handles dependencies
>> >individually). expat and yajl would be created as hidden options without
>> >visibility (no user interaction necessary).
>>  Yes, that would indeed be an alternative to keep the .mk file 
>> simpler. I
>> take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets
>> selected by the other MPD options that rely on expat. This indeed 
>> simplifies
>> things, but it is still a bit more complicated than what I propose.
> 
> But more correct.
> 
>> >Separate vorbis decoder and encoder options; currently the decoder feature
>> >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis
>> >encoding, which is probably not intended.
>>  I don't see a reason why you would want only one or the other. It has
>> virtually no impact on size.
> 
> Indeed here, vorbis support should just enable both the 
> encoder/decoder.
> 
> The only reason that we might want to be able to chose, is if enabling
> one or the other have different requirements: extra size requirements,
> extra dependencies, legal issues in your jurisdiction, etc...
> 
>>  In summary, I think we have the following three options for packages 
>> where
>> we decide we want user-visible sub-options.
>> 
>> - 1-to-1 mapping with the package configure options. Interaction 
>> between the
>> options is expressed with select/depends in Config.in. The .mk file 
>> simply
>> maps the Config.in options. If it's really not relevant for the user, 
>> a
>> Config.in option can be made blind. This is what Andreas proposes.
> 
> I am OK with that.
> 
>> - Only automatic dependencies in the .mk file, except in cases where 
>> it has
>> an important size or behaviour impact. Add Config.in options only in 
>> case it
>> is relevant. This is what I propose.
> 
> I am OK with the principle, but this does not look like what you
> proposed above, as the build-dependencies would be on the dependency
> being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs.
> BR2_PACKAGE_LIBVORBIS as you showed above).
> 
>> - Add Config.in options for important features. Express 
>> interdependencies
>> between package configure options in the .mk file. This is the current
>> situation for mpd.
> 
> I don;t see a difference here: "Add Config.in options only in case it 
> is
> relevant" and "Add Config.in options for important features" are 
> exactly
> the same in mny eyes...
> 
>>  So let's see what the other maintainers think. If you (or the other
>> maintainers) don't agree, we can compromise and go back to the 
>> original
>> patch (with just the npupnp/expat situation resolved).
> 
> So, I'll summariser my position:
> 
>   - add Config.in options when it makes sense:
>     - important size delta
>     - legal issues
>     those options select the appropriate packages
> 
>   - in the .mk:
>       - add conditional blocks based on those options, add build
>         dependencies as appropriate;
>       - add conditoinal dependencies on package for automatic
>         dependencies
> 
>>  Ideally I'd like the maintainers (and anybody else, really) to come 
>> to a
>> decision here and eventually formalize it in the manual. Because it's 
>> not
>> the first time this discussion crops up.
> 
> Regards,
> Yann E. MORIN.
> 
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' 
> conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___        
>        |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There 
> is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   
> conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH v2 0/4] User-visible Config.in feature sub-options

[Andreas Ziegler]

As indicated, this is the patch based on our discussion. I gave it the 
version number 2, even if the content has little to do with the original 
patch. I also split it into a series, to allow for partial integration. 

#1 just a bug fix
#2 add /modify comments -- mostly captures additional info, collected when
   looking up features in meson_options.txt and the mpd manual
#3 id3tag is a necessary sub-feature for mp3 decoders, so make them select
   the (already existing) sub-option, instead of doing their own selection
   and omitting id3tag
#4 introduce the sub-option logic for expat and yajl also; this cleans up
   upnp configuration also

I would appreciate integration of 1-3. #3 actually solves a possible con-
figuration error: if you configure mp3 playback, but forget id3tag, mp3 
files will not be recognized by mpd. 

I am open for discussion on #4. I can live with automatic dependencies based 
on libraries, aka

	ifeq ($(BR2_PACKAGE_FOO),y)
		MPD_DEPENDENCIES += libfoo
		MPD_CONF_OPTS += -Dfoo=enabled
	else
		MPD_CONF_OPTS += -Dfoo=disabled
	endif

(works for me), but would prefer dependencies based on Config.in sub-options. 

libicu handling remains unchanged. libicu adds approx. 40 MB to the compressed
image; whoever needs this in his firmware is welcome to the additional feature 
in mpd. It has no discernable size impact here.

Andreas Ziegler (4):
  package/mpd: fix reversed logic in tcp disable
  package/mpd: add/enhance (kconfig + code) comments
  package/mpd: introduce id3tag feature dependency
  package/mpd: introduce sub-options for expat and yajl

 package/mpd/Config.in | 30 ++++++++++++++++++++++--------
 package/mpd/mpd.mk    | 33 +++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 18 deletions(-)

-- 
2.34.1

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

[Buildroot] [PATCH v2 1/4] package/mpd: fix reversed logic in tcp disable

[Andreas Ziegler]

Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
Changes v1 -> v2:
 - make this a separate patch 

 package/mpd/mpd.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
index 5c15953984..4d73e6de03 100644
--- a/package/mpd/mpd.mk
+++ b/package/mpd/mpd.mk
@@ -285,8 +285,9 @@ else
 MPD_CONF_OPTS += -Dsqlite=disabled
 endif
 
+# default setting is 'true'
 ifneq ($(BR2_PACKAGE_MPD_TCP),y)
-MPD_CONF_OPTS += -Dtcp=true
+MPD_CONF_OPTS += -Dtcp=false
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_TREMOR),y)
-- 
2.34.1

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

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

[Andreas Ziegler]

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.
+	  This is mandatory for the sticker database.
 
 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.
 
 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
+
 comment "Input plugins"
 
 config BR2_PACKAGE_MPD_CDIO_PARANOIA
diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
index 4d73e6de03..feab894f0f 100644
--- a/package/mpd/mpd.mk
+++ b/package/mpd/mpd.mk
@@ -230,6 +230,7 @@ else
 MPD_CONF_OPTS += -Dopenal=disabled
 endif
 
+# opus needs to be encapsulated in a container format, here ogg
 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
 ifeq ($(BR2_PACKAGE_MPD_VORBIS),y)
 MPD_DEPENDENCIES += libvorbis
 MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled
-- 
2.34.1

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

[Buildroot] [PATCH v2 3/4] package/mpd: introduce id3tag feature dependency

[Andreas Ziegler]

id3tag is a sub-feature that is needed to extract information from mp3 files.
It selects the corresponding library and handles config settings. Two other
features need this sub-feature, but handle all library selections themselves
and omit enabling the id3tag feature. In consequence, users have to remember
to select both mp3 library and id3tag, otherwise the mpd executable will not
process mp3 files.

Reflect feature dependency in mpd Config.in to make id3tag selection automatic.

Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
Changes v1 -> v2:
 - make this a separate patch 

 package/mpd/Config.in | 4 ++--
 package/mpd/mpd.mk    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/package/mpd/Config.in b/package/mpd/Config.in
index 2606008e90..daf1469cae 100644
--- a/package/mpd/Config.in
+++ b/package/mpd/Config.in
@@ -127,7 +127,7 @@ config BR2_PACKAGE_MPD_LIBSNDFILE
 config BR2_PACKAGE_MPD_MAD
 	bool "mad"
 	default y
-	select BR2_PACKAGE_LIBID3TAG
+	select BR2_PACKAGE_MPD_ID3TAG
 	select BR2_PACKAGE_LIBMAD
 	help
 	  Enable mad input support.
@@ -141,7 +141,7 @@ config BR2_PACKAGE_MPD_MODPLUG
 
 config BR2_PACKAGE_MPD_MPG123
 	bool "mpg123"
-	select BR2_PACKAGE_LIBID3TAG
+	select BR2_PACKAGE_MPD_ID3TAG
 	select BR2_PACKAGE_MPG123
 	help
 	  Enable mpg123 input support.
diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
index feab894f0f..1269b90661 100644
--- a/package/mpd/mpd.mk
+++ b/package/mpd/mpd.mk
@@ -190,7 +190,7 @@ MPD_CONF_OPTS += -Dsoxr=disabled
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_MAD),y)
-MPD_DEPENDENCIES += libid3tag libmad
+MPD_DEPENDENCIES += libmad
 MPD_CONF_OPTS += -Dmad=enabled
 else
 MPD_CONF_OPTS += -Dmad=disabled
@@ -204,7 +204,7 @@ MPD_CONF_OPTS += -Dmodplug=disabled
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_MPG123),y)
-MPD_DEPENDENCIES += libid3tag mpg123
+MPD_DEPENDENCIES += mpg123
 MPD_CONF_OPTS += -Dmpg123=enabled
 else
 MPD_CONF_OPTS += -Dmpg123=disabled
-- 
2.34.1

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

[Buildroot] [PATCH v2 4/4] package/mpd: introduce sub-options for expat and yajl

[Andreas Ziegler]

Make inclusion of expat and yajl libraries in the mpd build dependent on
user selection, not presence of the libraries in the target filesystem.

Signed-off-by: Andreas Ziegler <br015@umbiko.net>
---
Changes v1 -> v2:
 - based on a discussion w/ Arnout Vandecapelle, Yann Morin

 package/mpd/Config.in | 17 ++++++++++++++---
 package/mpd/mpd.mk    | 24 +++++++++++++++++-------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/package/mpd/Config.in b/package/mpd/Config.in
index daf1469cae..fe4f22201a 100644
--- a/package/mpd/Config.in
+++ b/package/mpd/Config.in
@@ -20,6 +20,16 @@ menuconfig BR2_PACKAGE_MPD
 
 if BR2_PACKAGE_MPD
 
+# mpd internal feature dependencies
+
+config BR2_PACKAGE_MPD_EXPAT
+	bool
+	select BR2_PACKAGE_EXPAT
+
+config BR2_PACKAGE_MPD_YAJL
+	bool
+	select BR2_PACKAGE_YAJL
+
 comment "Archive plugins"
 
 config BR2_PACKAGE_MPD_BZIP2
@@ -48,14 +58,14 @@ config BR2_PACKAGE_MPD_QOBUZ
 	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS
 	select BR2_PACKAGE_MPD_CURL
 	select BR2_PACKAGE_LIBGCRYPT
-	select BR2_PACKAGE_YAJL
+	select BR2_PACKAGE_MPD_YAJL
 	help
 	  Play songs from the commercial streaming service Qobuz.
 
 config BR2_PACKAGE_MPD_SOUNDCLOUD
 	bool "soundcloud"
 	select BR2_PACKAGE_MPD_CURL
-	select BR2_PACKAGE_YAJL
+	select BR2_PACKAGE_MPD_YAJL
 	help
 	  Enable soundcloud.com playlist support.
 
@@ -399,9 +409,9 @@ choice
 
 config BR2_PACKAGE_MPD_UPNP_PUPNP
 	bool "pupnp"
-	select BR2_PACKAGE_EXPAT
 	select BR2_PACKAGE_LIBUPNP
 	select BR2_PACKAGE_MPD_CURL
+	select BR2_PACKAGE_MPD_EXPAT
 	help
 	  Provides UPnP database access through libupnp
 	  (the legacy Portable SDK for UPnP devices).
@@ -412,6 +422,7 @@ config BR2_PACKAGE_MPD_UPNP_NPUPNP
 	bool "npupnp"
 	select BR2_PACKAGE_LIBNPUPNP
 	select BR2_PACKAGE_MPD_CURL
+	select BR2_PACKAGE_MPD_EXPAT
 	help
 	  Provides UPnP database access through libnpupnp
 	  (a C++ reimplementation of the Portable UPnP library).
diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk
index 1269b90661..1c3b659220 100644
--- a/package/mpd/mpd.mk
+++ b/package/mpd/mpd.mk
@@ -78,6 +78,13 @@ else
 MPD_CONF_OPTS += -Ddsd=false
 endif
 
+ifeq ($(BR2_PACKAGE_MPD_EXPAT),y)
+MPD_DEPENDENCIES += expat
+MPD_CONF_OPTS += -Dexpat=enabled
+else
+MPD_CONF_OPTS += -Dexpat=disabled
+endif
+
 ifeq ($(BR2_PACKAGE_MPD_FAAD2),y)
 MPD_DEPENDENCIES += faad2
 MPD_CONF_OPTS += -Dfaad=enabled
@@ -252,7 +259,7 @@ MPD_CONF_OPTS += -Dpulse=disabled
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_QOBUZ),y)
-MPD_DEPENDENCIES += libgcrypt yajl
+MPD_DEPENDENCIES += libgcrypt
 MPD_CONF_OPTS += -Dqobuz=enabled
 else
 MPD_CONF_OPTS += -Dqobuz=disabled
@@ -273,7 +280,6 @@ MPD_CONF_OPTS += -Dsidplay=disabled
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_SOUNDCLOUD),y)
-MPD_DEPENDENCIES += yajl
 MPD_CONF_OPTS += -Dsoundcloud=enabled
 else
 MPD_CONF_OPTS += -Dsoundcloud=disabled
@@ -306,13 +312,10 @@ MPD_CONF_OPTS += -Dtwolame=disabled
 endif
 
 ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y)
-MPD_DEPENDENCIES += \
-	expat \
-	libupnp
+MPD_DEPENDENCIES += libupnp
 MPD_CONF_OPTS += -Dupnp=pupnp
 else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y)
-MPD_DEPENDENCIES += \
-	libnpupnp
+MPD_DEPENDENCIES += libnpupnp
 MPD_CONF_OPTS += -Dupnp=npupnp
 else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y)
 MPD_CONF_OPTS += -Dupnp=disabled
@@ -333,6 +336,13 @@ else
 MPD_CONF_OPTS += -Dwavpack=disabled
 endif
 
+ifeq ($(BR2_PACKAGE_MPD_YAJL),y)
+MPD_DEPENDENCIES += yajl
+MPD_CONF_OPTS += -Dyajl=enabled
+else
+MPD_CONF_OPTS += -Dyajl=disabled
+endif
+
 ifeq ($(BR2_PACKAGE_MPD_ZZIP),y)
 MPD_DEPENDENCIES += zziplib
 MPD_CONF_OPTS += -Dzzip=enabled
-- 
2.34.1

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

Re: [Buildroot] [PATCH v2 1/4] package/mpd: fix reversed logic in tcp disable

[Thomas Petazzoni via buildroot]

Hello Andreas,

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

> Signed-off-by: Andreas Ziegler <br015@umbiko.net>

I've extended this explanation with some details on when the issue was
introduced:

    package/mpd: fix reversed logic in tcp disable
    
    In commit 54b9008d482923131191da75f09966483f4ecac1 ("package/mpd: bump
    to version 0.21.11"), mpd was migrated from using the autotools build
    system to the meson build system.
    
    As part of this, the BR2_PACKAGE_MPD_TCP was incorrectly modified,
    leading BR2_PACKAGE_MPD_TCP disabled to actually enable TCP, and
    BR2_PACKAGE_MPD_TCP enabled to not explicitly enable TCP support.
    
    This commit fixes that by handling this option in the common way.
    
    Signed-off-by: Andreas Ziegler <br015@umbiko.net>
    Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

> +# default setting is 'true'
>  ifneq ($(BR2_PACKAGE_MPD_TCP),y)
> -MPD_CONF_OPTS += -Dtcp=true
> +MPD_CONF_OPTS += -Dtcp=false
>  endif

Changed to:

ifeq ($(BR2_PACKAGE_MPD_TCP),y)
MPD_CONF_OPTS += -Dtcp=true
else
MPD_CONF_OPTS += -Dtcp=false
endif

Applied to master with those changes.

Best regards,

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

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

[Thomas Petazzoni via buildroot]

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

Re: [Buildroot] [PATCH v2 3/4] package/mpd: introduce id3tag feature dependency

[Thomas Petazzoni via buildroot]

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

> id3tag is a sub-feature that is needed to extract information from mp3 files.
> It selects the corresponding library and handles config settings. Two other
> features need this sub-feature, but handle all library selections themselves
> and omit enabling the id3tag feature. In consequence, users have to remember
> to select both mp3 library and id3tag, otherwise the mpd executable will not
> process mp3 files.
> 
> Reflect feature dependency in mpd Config.in to make id3tag selection automatic.
> 
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>
> ---
> Changes v1 -> v2:
>  - make this a separate patch 

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 4/4] package/mpd: introduce sub-options for expat and yajl

[Thomas Petazzoni via buildroot]

Hello Andreas,

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

> Make inclusion of expat and yajl libraries in the mpd build dependent on
> user selection, not presence of the libraries in the target filesystem.
> 
> Signed-off-by: Andreas Ziegler <br015@umbiko.net>

So I handled this differently:

- First, one patch that adds the "missing" dependency of
  BR2_PACKAGE_MPD_UPNP_NPUPNP on expat:

  https://gitlab.com/buildroot.org/buildroot/-/commit/6cdb48a0482a093dcdf73d0d748f5435262515ab

- Second, a patch that takes care of expat and yajl, but without the
  intermediate options: as soon as expat or yajl are available in
  Buildroot, their support will enabled in mpq:

  https://gitlab.com/buildroot.org/buildroot/-/commit/3693462a1f8bd328c1a4f8fb68f00ce9127c31cd

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

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

[Andreas Ziegler]

Hi Thomas, *,

Thank you for picking this up.

On 2023-08-06 19:45, Thomas Petazzoni wrote:
> 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?

It raises false hopes. Mpd uses an in-memory database that is persisted 
in a proprietary file format. The format may change between releases and 
there is no way to use a SQL database for storage. All that the 
sqlite=enabled feature does is enable the sticker database.

> 
>> +	  This is mandatory for the sticker database.
> 
> Not sure what the sticker database is :-)

I will add more information from the mpd manual:
+	  Support for the sticker database.  “Stickers” are pieces of
+	  information attached to songs. Some clients use them to store
+	  ratings and other volatile data.

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

The Digital Speech [1] format exists, but this is not the format 
referred to here. MPD plays SA-CD rips [2]

Maybe:
+	  Direct Stream Digital (DSD) support to play delta-sigma
+	  encoded files in formats DSDIFF and DSF.
+
+	  This is the sample format used on Super Audio CDs.

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

Options are structured by function. Since ogg/vorbis codecs must be 
selected simultaneously (according to the Buildroot standard), a feature 
that is located under *decoding* must be enabled for *encoding*. Breaks 
up the UI a little bit, therefore a comment.

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

This is a different use case. The encoder is used for streaming 
protocols, similar to internet radio stations.

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

Most mpd options have exactly one dependency. Feature name and library 
name match; easy to see that everything is as it should be. So I added 
comments whenever there was a deviation, just to show that this is not 
an error, but intended.

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

Again, a deviation from the one feature /one selection pattern.

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

Some packages - for example mpd - are rather difficult to configure 
correctly. So why not add a bit more information, especially in the user 
interface ;-)

I expect your comments and will follow up with a new version.

Kind regards,
Andreas

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

[1] https://en.wikipedia.org/wiki/Digital_Speech_Standard
[2] https://en.wikipedia.org/wiki/Direct_Stream_Digital
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/4] package/mpd: fix reversed logic in tcp disable

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni via buildroot <buildroot@buildroot.org> writes:

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

 >> Signed-off-by: Andreas Ziegler <br015@umbiko.net>

 > I've extended this explanation with some details on when the issue was
 > introduced:

 >     package/mpd: fix reversed logic in tcp disable
    
 >     In commit 54b9008d482923131191da75f09966483f4ecac1 ("package/mpd: bump
 >     to version 0.21.11"), mpd was migrated from using the autotools build
 >     system to the meson build system.
    
 >     As part of this, the BR2_PACKAGE_MPD_TCP was incorrectly modified,
 >     leading BR2_PACKAGE_MPD_TCP disabled to actually enable TCP, and
 >     BR2_PACKAGE_MPD_TCP enabled to not explicitly enable TCP support.
    
 >     This commit fixes that by handling this option in the common way.
    
 >     Signed-off-by: Andreas Ziegler <br015@umbiko.net>
 >     Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2023.02.x and 2023.05.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 4/4] package/mpd: introduce sub-options for expat and yajl

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni via buildroot <buildroot@buildroot.org> writes:

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

 >> Make inclusion of expat and yajl libraries in the mpd build dependent on
 >> user selection, not presence of the libraries in the target filesystem.
 >> 
 >> Signed-off-by: Andreas Ziegler <br015@umbiko.net>

 > So I handled this differently:

 > - First, one patch that adds the "missing" dependency of
 >   BR2_PACKAGE_MPD_UPNP_NPUPNP on expat:

 >   https://gitlab.com/buildroot.org/buildroot/-/commit/6cdb48a0482a093dcdf73d0d748f5435262515ab

 > - Second, a patch that takes care of expat and yajl, but without the
 >   intermediate options: as soon as expat or yajl are available in
 >   Buildroot, their support will enabled in mpq:

 >   https://gitlab.com/buildroot.org/buildroot/-/commit/3693462a1f8bd328c1a4f8fb68f00ce9127c31cd

Committed to 2023.02.x and 2023.05.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 3/4] package/mpd: introduce id3tag feature dependency

[Peter Korsgaard]

>>>>> "Andreas" == Andreas Ziegler <br015@umbiko.net> writes:

 > id3tag is a sub-feature that is needed to extract information from mp3 files.
 > It selects the corresponding library and handles config settings. Two other
 > features need this sub-feature, but handle all library selections themselves
 > and omit enabling the id3tag feature. In consequence, users have to remember
 > to select both mp3 library and id3tag, otherwise the mpd executable will not
 > process mp3 files.

 > Reflect feature dependency in mpd Config.in to make id3tag selection automatic.

 > Signed-off-by: Andreas Ziegler <br015@umbiko.net>
 > ---
 > Changes v1 -> v2:
 >  - make this a separate patch 

Committed to 2023.02.x and 2023.05.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot