Re: [Buildroot] [PATCH] boot/uboot: fix Kconfig warning for DEFAULT_ENV_FILE

From: Luca Ceresoli via buildroot <buildroot@buildroot.org>
To: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Brandon Maier <brandon.maier@collins.com>,
	buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH] boot/uboot: fix Kconfig warning for DEFAULT_ENV_FILE
Date: Fri, 19 Jul 2024 09:51:15 +0200	[thread overview]
Message-ID: <20240719095115.226b1d51@booty> (raw)
In-Reply-To: <b165fb5e-38f5-4751-9873-765ab3239549@mind.be>

Hi Arnout, Thomas, Brandon,

On Thu, 18 Jul 2024 22:32:21 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:

> On 18/07/2024 19:13, Luca Ceresoli wrote:
> > Hello Brandon, Arnout,
> > 
> > On Thu, 18 Jul 2024 15:45:45 +0000
> > Brandon Maier <brandon.maier@collins.com> wrote:
> >   
> >> The following warning occurs from commit "boot/uboot: allow taking the
> >> entire default environment from a text file".
> >>
> >>    boot/uboot/Config.in:141:warning: config symbol
> >>    'BR2_TARGET_UBOOT_DEFAULT_ENV_FILE' uses select, but is not bool or
> >>    tristate  
> 
>   Oh right, Kconfig is too stupid for that. You need a helper config but it can 
> be blind. Like:
> 
> config BR2_TARGET_UBOOT_USE_DEFAULT_ENV_FILE
> 	bool
> 	default y
> 	depends on BR2_TARGET_UBOOT_DEFAULT_ENV_FILE != ""
> 	select BR2_TARGET_UBOOT_NEEDS_XXD

Ah, right! And it's similar to what Thomas proposed. Thanks both.

> > Hum, apologies, didn't notice this, at least when working on v2 (see
> > below).
> >   
> >> Add a 'bool' config so we can enable dependencies.  
> > 
> > That's basically what I did in v1 [0], possibly for this same reason,
> > but I'm afraid I don't remember exactly.
> > 
> > Then in his review Arnout suggested to have only a string option, and I
> > did not recall any good reason to not do so, and so v2 came without the
> > bool.
> >   
> >> In addition, the DEFAULT_ENV_FILE works by setting Kconfig options,
> >> therefore we also need to depend on
> >> BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG.  
> 
>   Oh yes, but that's a simple depends on in the 
> BR2_TARGET_UBOOT_DEFAULT_ENV_FILE option.

Indeed it has to be added.

> > Arnout, do you think the issues reported by Brandon can be fixed while
> > keeping a single string option?  
> 
>   Yes I think so. But it needs to be tested so I'd prefer is _someone_ <grin> 
> would do that.

Sure, _someone_ will. :) And that will be me, perhaps next week however,
unless Brandon wants to send a patch before I do.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot