Re: [PATCH] Documentation: kbuild: explain handling optional dependencies

[Jani Nikula]

On Fri, 15 Sep 2023, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:
> On Thu, Sep 14, 2023, at 19:23, Masahiro Yamada wrote:
>> On Thu, Sep 14, 2023 at 11:57 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>> On Thu, Sep 14, 2023, at 15:42, Jani Nikula wrote:
>>
>> It is unclear why WIREGUARD must be entirely disabled
>> just because of the optional feature being modular.
>
> I don't think anyone is asking for that, and the current
> "depends on IPV6 || !IPV6" seems fine here, and is consistent
> with dozens of other symbols.
>
>> My preference is to use IS_REACHABLE(CONFIG_IPV6)
>> instead of IS_ENABLED(CONFIG_IPV6)
>> under drivers/net/wireguard, then
>> get rid of "depends on IPV6 || !IPV6)
>
> My feeling is that this would be significantly worse from a
> usability point of view even if it made it a little easier
> for maintainers:
>
> When a user selects both IPV6 and WIREGUARD, they expect
> to be able to use them together, and a normal user setting
> WIREGUARD=y would have a hard time figuring out why that
> leads it becoming IPv4-only.

I think IS_REACHABLE() is in most cases just plain wrong, and should
only be used as the last resort.

I think the kconfig should express what the dependencies are, and you
should get kconfig or build errors if you get it wrong, and not paper
over them with IS_REACHABLE().

Configuring the kernel is hard enough, and IS_REACHABLE() silences the
issues with no messages to the user at any point. If the user gets it
wrong, it just doesn't work like they expect, they have no clues why,
and they have to peruse the kernel source to figure it out. (Or, more
likely, file a bug and waste the kernel developer/maintainer time to get
the configuration right.)

IS_REACHABLE() considered harmful.


BR,
Jani.


>
>> If you want to make it clearer on the Kconfig level,
>> perhaps the following is also possible.
>>
>>
>> config WIREGUARD
>>        tristate "WireGuard"
>>
>> config WIREGUARD_IPV6
>>        def_bool y
>>        depends on WIREGUARD
>>        depends on IPV6 >= WIREGUARD
>>
>> config IPV6
>>        tristate "IPV6"
>
> That has the same downside, with the added problem
> of also confusing kernel developers with the '>='
> Kconfig syntax, which IMHO makes no sense unless one
> knows way too much about Kconfig internals.
>
>       Arnd

-- 
Jani Nikula, Intel