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 <br015@umbiko.net>]

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