Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()

Alex G.
2021-09-27T16:11:33Z

On 9/25/21 8:43 PM, Simon Glass wrote:
> At present we must separately test for the host build for many options,
> since we force them to be enabled. For example, CONFIG_FIT is always
> enabled in the host tools, even if CONFIG_FIT is not enabled by the
> board itself.
>
> It would be more convenient if we could use, for example,
> CONFIG_IS_ENABLED(FIT) and get CONFIG_HOST_FIT, when building for the
> host. Add support for this.
>
> With this and the tools_build() function, we should be able to remove all
> the #ifdefs currently needed in code that is build by tools and targets.
>
> This will be even nicer when we move to using CONFIG(xxx) everywhere,
> since all the #ifdef and IS_ENABLED/CONFIG_IS_ENABLED stuff will go away.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> # b4f73886
> ---

In my vision, the host tools are target-agostic. They always support the
same feature set: all features. From that point of view, it doesn't make
sense to have options which enable or disable tools features.

I understand the motivation is to use CONFIG_IS_ENABLED(), which has the
appearance of making the code cleaner. We have to continue using #if
CONFIG_IS_ENABLED() -- is that an improvement over old-school #ifdefs?

So I question whether this new direction makes sense for the long term,
as opposed to taking a deeper look at the underlying code. The polite
thing from me would be to propose alternatives, which I don't have the
bandwidth these days. I won't be adding opposition to this series other
than these final thoughts. We can fix the code later, and then remove
the HOST configs.

Alex

>
> Changes in v5:
> - Update commit message
> - Use TOOLS_ instead of HOST_
>
> Changes in v2:
> - Correct comment about USE_HOSTCC being undefined in CONFIG_VAL()
> - Fix up comment to put an underscore after every CONFIG
>
> include/linux/kconfig.h | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
> index d109ed3119e..a1d1a298426 100644
> --- a/include/linux/kconfig.h
> +++ b/include/linux/kconfig.h
> @@ -31,11 +31,14 @@
> (config_enabled(option))
>
> /*
> - * U-Boot add-on: Helper macros to reference to different macros
> - * (CONFIG_ or CONFIG_SPL_ prefixed), depending on the build context.
> + * U-Boot add-on: Helper macros to reference to different macros (prefixed by
> + * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build
> + * context.
> */
>
> -#if defined(CONFIG_TPL_BUILD)
> +#ifdef USE_HOSTCC
> +#define _CONFIG_PREFIX TOOLS_
> +#elif defined(CONFIG_TPL_BUILD)
> #define _CONFIG_PREFIX TPL_
> #elif defined(CONFIG_SPL_BUILD)
> #define _CONFIG_PREFIX SPL_
> @@ -49,6 +52,7 @@
>
> /*
> * CONFIG_VAL(FOO) evaluates to the value of
> + * CONFIG_TOOLS_FOO if USE_HOSTCC is defined,
> * CONFIG_FOO if CONFIG_SPL_BUILD is undefined,
> * CONFIG_SPL_FOO if CONFIG_SPL_BUILD is defined.
> * CONFIG_TPL_FOO if CONFIG_TPL_BUILD is defined.
> @@ -76,18 +80,21 @@
>
> /*
> * CONFIG_IS_ENABLED(FOO) expands to
> + * 1 if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y',
> * 1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
> * 1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
> * 1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
> * 0 otherwise.
> *
> * CONFIG_IS_ENABLED(FOO, (abc)) expands to
> + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y',
> * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
> * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
> * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
> * nothing otherwise.
> *
> * CONFIG_IS_ENABLED(FOO, (abc), (def)) expands to
> + * abc if USE_HOSTCC is defined and CONFIG_TOOLS_FOO is set to 'y',
> * abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
> * abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
> * abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
>