[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