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

[Masahiro Yamada <masahiroy@kernel.org>]

On Tue, Sep 28, 2021 at 1:11 AM Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> 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.


Agree.
Host tools should not depend on any CONFIG option.

In Linux kernel, most host tools are target-agnostic.
(It is true there are some exceptions such as objtool,
modpost, etc. but in theory we can make all of them
target-agnostic)

In fact, U-Boot made much more tools target-dependent.
They must be rebuilt every time you change the configuration.

For example, mkimage in U-Boot supports many boards
and depends on the configuration.
It makes less sense to support all sort of board-specific
firmware in a huge single host tool.


In contrast, Barebox splits it into small programs,
zynq_mkimage, socfpga_mkimage, etc.,
each of which is target-agnostic.  [1]
Makefile decides only whether each program should be built or not.

[1] https://github.com/saschahauer/barebox/blob/v2021.07.0/scripts/Makefile#L22



>
> 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',
> >



-- 
Best Regards
Masahiro Yamada