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

From: Simon Glass <sjg@chromium.org>
To: "Alex G." <mr.nuke.me@gmail.com>
Cc: U-Boot Mailing List <u-boot@lists.denx.de>,
	Andre Przywara <andre.przywara@arm.com>,
	 Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
	Robert Marko <robert.marko@sartura.hr>,
	 Masahiro Yamada <masahiroy@kernel.org>,
	Tom Rini <trini@konsulko.com>,
	 Masahiro Yamada <yamada.masahiro@socionext.com>
Subject: Re: [PATCH v5 02/29] kconfig: Add tools support to CONFIG_IS_ENABLED()
Date: Mon, 27 Sep 2021 14:17:50 -0600	[thread overview]
Message-ID: <CAPnjgZ0N-9nsQELhT+nS-3wRSQAwQBPyKcgKo=06Y965Yhyv_Q@mail.gmail.com> (raw)
In-Reply-To: <cada774e-1f9c-d92c-34eb-b1840208b1c5@gmail.com>

Hi Alex,

On Mon, 27 Sept 2021 at 10:11, 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.
>
> 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?

nit: if() not #if

Most of the changes in this series are to remove #if and #idef

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

The key files to look at are those related to images in common/ which
are built by tools. It's actually a bit hard to pass judgement right
now since there is so much #ifdefing going on. You'll notice that one
patch creates image-board.c to try to avoid compiling code
unnecessarily for tools. I am sure there is more oppty for that. But
Rome wasn't built in a day.

One fundamental question is driver model. Of course it isn't supported
in tools and I think it makes no sense to add it, since it just
complicates the code. But as more of the feature become more diverse
(e.g. hashing, crypto, other accelerators) we have to deal with making
the software version of these features available in tools and also in
the board.

Anyway I think this series takes things forward and there is a path to
do another pass to drop more #ifdefs as well.

As to your question about using CONFIG_IS_ENABLED() for tools, I see
it as a convenience for now, but I would not bank on it being needed
in the fullness of time. It's just hard to know until we get closer to
cleaning this up.

Regards,
Simon