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

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

[Brandon Maier via buildroot]

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

Add a 'bool' config so we can enable dependencies.

In addition, the DEFAULT_ENV_FILE works by setting Kconfig options,
therefore we also need to depend on
BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG.

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
 boot/uboot/Config.in | 10 +++++++++-
 boot/uboot/uboot.mk  |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 57aac06e20..92e7bcc20a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -136,9 +136,15 @@ config BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES
 	  that will be merged to the main U-Boot configuration file.
 endif
 
+config BR2_TARGET_UBOOT_DEFAULT_ENV
+	bool "Use default environment file"
+	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
+	select BR2_TARGET_UBOOT_NEEDS_XXD
+
+if BR2_TARGET_UBOOT_DEFAULT_ENV
+
 config BR2_TARGET_UBOOT_DEFAULT_ENV_FILE
 	string "Text file with default environment"
-	select BR2_TARGET_UBOOT_NEEDS_XXD
 	help
 	  Text file containing the variables to be used as the default
 	  environment in U-Boot.
@@ -165,6 +171,8 @@ config BR2_TARGET_UBOOT_DEFAULT_ENV_FILE
 
 	  Requires U-Boot >= v2018.05.
 
+endif
+
 config BR2_TARGET_UBOOT_NEEDS_DTC
 	bool "U-Boot needs dtc"
 	select BR2_PACKAGE_HOST_DTC
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 6cc87b35d3..df53fb28c1 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -391,8 +391,8 @@ UBOOT_KCONFIG_EDITORS = menuconfig xconfig gconfig nconfig
 # time, so use HOSTCC_NOCCACHE.
 UBOOT_KCONFIG_OPTS = $(UBOOT_MAKE_OPTS) HOSTCC="$(HOSTCC_NOCCACHE)" HOSTLDFLAGS=""
 
+ifeq ($(BR2_TARGET_UBOOT_DEFAULT_ENV),y)
 UBOOT_DEFAULT_ENV_FILE = $(call qstrip,$(BR2_TARGET_UBOOT_DEFAULT_ENV_FILE))
-ifneq ($(UBOOT_DEFAULT_ENV_FILE),)
 define UBOOT_KCONFIG_DEFAULT_ENV_FILE
 	$(call KCONFIG_SET_OPT,CONFIG_USE_DEFAULT_ENV_FILE,y)
 	$(call KCONFIG_SET_OPT,CONFIG_DEFAULT_ENV_FILE,"$(shell readlink -f $(UBOOT_DEFAULT_ENV_FILE))")

---
base-commit: c624eee120b6ee0c81e636bd47abe92452610cf7
change-id: 20240718-boot-uboot-env-select-2f98051c7af9

Best regards,
-- 
Brandon Maier <brandon.maier@collins.com>

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Luca Ceresoli via buildroot]

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

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.

Arnout, do you think the issues reported by Brandon can be fixed while
keeping a single string option?

If they can't, I think it's worth resurrecting the help text from the
two options as I had written them initially in [0]. Otherwise one would
have a bool without help text, and woudl have to enable it just to make
the string option (with help text) visible. Kinda chicken-egg.

[0]
https://lore.kernel.org/buildroot/20240617-uboot-default-env-v1-1-9ac88f0e1789@bootlin.com/

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

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

[Thomas Petazzoni via buildroot]

Hello,

On Thu, 18 Jul 2024 19:13:55 +0200
Luca Ceresoli via buildroot <buildroot@buildroot.org> wrote:

> > 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.  
> 
> Arnout, do you think the issues reported by Brandon can be fixed while
> keeping a single string option?
> 
> If they can't, I think it's worth resurrecting the help text from the
> two options as I had written them initially in [0]. Otherwise one would
> have a bool without help text, and woudl have to enable it just to make
> the string option (with help text) visible. Kinda chicken-egg.

Here is something that seems to work:

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 57aac06e20..aff9f01ea4 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -136,9 +136,13 @@ config BR2_TARGET_UBOOT_CONFIG_FRAGMENT_FILES
          that will be merged to the main U-Boot configuration file.
 endif
 
+config BR2_TARGET_UBOOT_DEFAULT_ENV_FILE_HIDDEN
+       bool
+       default y if BR2_TARGET_UBOOT_DEFAULT_ENV_FILE != ""
+       select BR2_TARGET_UBOOT_NEEDS_XXD
+
 config BR2_TARGET_UBOOT_DEFAULT_ENV_FILE
        string "Text file with default environment"
-       select BR2_TARGET_UBOOT_NEEDS_XXD
        help
          Text file containing the variables to be used as the default
          environment in U-Boot.

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Arnout Vandecappelle via buildroot]

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


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

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

  Regards,
  Arnout


> If they can't, I think it's worth resurrecting the help text from the
> two options as I had written them initially in [0]. Otherwise one would
> have a bool without help text, and woudl have to enable it just to make
> the string option (with help text) visible. Kinda chicken-egg.
> 
> [0]
> https://lore.kernel.org/buildroot/20240617-uboot-default-env-v1-1-9ac88f0e1789@bootlin.com/
> 
> Luca
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Luca Ceresoli via buildroot]

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