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

Sat Apr 9 16:09:21 UTC 2022

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