Skip to contentSkip to conversation

uboot-tools: Allow users to use uboot's sources

PATCH New Maxime Hadjinlian 5 participants 12 messages Jan 26, 2014 – Jan 28, 2014

lore
Share & tools
lore

Maxime Hadjinlian

author
#0 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

If the user has specified a custom U-Boot repository, he may also want to use it for U-Boot tools.

This could be usefull in two identified use case:
- User want the same version for U-Boot tools and U-Boot
- User has modified U-Boot tools in his U-Boot repository

Signed-off-by Maxime Hadjinlian <[email protected]>
---
 package/uboot-tools/Config.in      | 10 ++++++++++
 package/uboot-tools/uboot-tools.mk |  9 +++++++++
 2 files changed, 19 insertions(+)
package/uboot-tools/Config.in +10 -0
index 7c8f17c..3742b0e 100644
@@ -7,6 +7,16 @@ config BR2_PACKAGE_UBOOT_TOOLS
 
 if BR2_PACKAGE_UBOOT_TOOLS
 
+if BR2_TARGET_UBOOT
+
+config BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
+	bool "Use the same repository as U-Boot ?"
+	help
+	  Select this to use the same repository specified for U-Boot. Otherwise,
+	  the upstream sources will be used.
+
+endif
+
 config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
 	bool "mkimage"
 	help
package/uboot-tools/uboot-tools.mk +9 -0
index 398ce8b..367d067 100644
@@ -10,6 +10,15 @@ UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
 UBOOT_TOOLS_LICENSE = GPLv2+
 UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
+	UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+	UBOOT_TOOLS_SOURCE  = $(UBOOT_SOURCE)
+	UBOOT_TOOLS_SITE    = $(UBOOT_SITE)
+ifeq ($(UBOOT_SITE_METHOD),)
+	UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
+endif
+endif
+
 define UBOOT_TOOLS_BUILD_CMDS
 	$(MAKE) -C $(@D) 			\
 		HOSTCC="$(TARGET_CC)"		\
Signature · 1 line
1.8.5.2
#1 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Hi Maxime,

Quoted 8 lines · Maxime Hadjinlian

If the user has specified a custom U-Boot repository, he may also want to use it for U-Boot tools.

This could be usefull in two identified use case:
- User want the same version for U-Boot tools and U-Boot
- User has modified U-Boot tools in his U-Boot repository

Signed-off-by Maxime Hadjinlian <[email protected]>

I ACK the idea, but there are a few changes I would make to your patch, see below.

Quoted 10 lines
---
  package/uboot-tools/Config.in      | 10 ++++++++++
  package/uboot-tools/uboot-tools.mk |  9 +++++++++
  2 files changed, 19 insertions(+)
package/uboot-tools/Config.in +0 -0
index 7c8f17c..3742b0e 100644

package/uboot-tools/Config.in config BR2_PACKAGE_UBOOT_TOOLS · line 13

Quoted 7 lines
package/uboot-tools/Config.in +4 -0
@@ -7,6 +7,16 @@ config BR2_PACKAGE_UBOOT_TOOLS
 
  if BR2_PACKAGE_UBOOT_TOOLS
 
+if BR2_TARGET_UBOOT
+
+config BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
+	bool "Use the same repository as U-Boot ?"

We don't usually have question marks in option names.

package/uboot-tools/Config.in config BR2_PACKAGE_UBOOT_TOOLS · line 16

Quoted 3 lines
package/uboot-tools/Config.in +3 -0
+	help
+	  Select this to use the same repository specified for U-Boot. Otherwise,
+	  the upstream sources will be used.

Just nitpicking, but IMO the newcomer might find it confusing: we are always downloading U-Boot sources, so what does this option change?

I would replace "specified for U-Boot" with "specified for the uboot package". This makes it more clear that you refer to the "uboot" package in Buildroot.

This is just a clarification, there is nothing wrong in your wording.

Quoted 11 lines
package/uboot-tools/Config.in +3 -0
+
+endif
+
  config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
  	bool "mkimage"
  	help
package/uboot-tools/uboot-tools.mk +0 -0
index 398ce8b..367d067 100644

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 16

Quoted 7 lines
package/uboot-tools/uboot-tools.mk +4 -0
@@ -10,6 +10,15 @@ UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
  UBOOT_TOOLS_LICENSE = GPLv2+
  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
+	UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+	UBOOT_TOOLS_SOURCE  = $(UBOOT_SOURCE)
+	UBOOT_TOOLS_SITE    = $(UBOOT_SITE)

You are overriding the previously-defined options. I find an if/then/else construct much cleaner:

ifneq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
...use upstream sources... (current code)
else
...use sources for the uboot package...
endif

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 17

Quoted 1 line
package/uboot-tools/uboot-tools.mk +1 -0
+ifeq ($(UBOOT_SITE_METHOD),)

This should be an ifneq, not ifeq.

Quoted 8 lines
package/uboot-tools/uboot-tools.mk +4 -0
+	UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
+endif
+endif
+
  define UBOOT_TOOLS_BUILD_CMDS
  	$(MAKE) -C $(@D) 			\
  		HOSTCC="$(TARGET_CC)"		\
Signature · 1 line
Luca
author
#2 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Quoted 1 line · Luca Ceresoli

Hi Maxime,

Hi Luca

Quoted 46 lines
Quoted 9 lines · Maxime Hadjinlian

If the user has specified a custom U-Boot repository, he may also want to use it for U-Boot tools.

This could be usefull in two identified use case:
- User want the same version for U-Boot tools and U-Boot
- User has modified U-Boot tools in his U-Boot repository

Signed-off-by Maxime Hadjinlian <[email protected]>

I ACK the idea, but there are a few changes I would make to your patch, see below.

Quoted 10 lines
---
  package/uboot-tools/Config.in      | 10 ++++++++++
  package/uboot-tools/uboot-tools.mk |  9 +++++++++
  2 files changed, 19 insertions(+)
package/uboot-tools/Config.in +0 -0
index 7c8f17c..3742b0e 100644

package/uboot-tools/Config.in config BR2_PACKAGE_UBOOT_TOOLS · line 13

Quoted 7 lines
package/uboot-tools/Config.in +4 -0
@@ -7,6 +7,16 @@ config BR2_PACKAGE_UBOOT_TOOLS
 
  if BR2_PACKAGE_UBOOT_TOOLS
 
+if BR2_TARGET_UBOOT
+
+config BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
+       bool "Use the same repository as U-Boot ?"

We don't usually have question marks in option names.

package/uboot-tools/Config.in config BR2_PACKAGE_UBOOT_TOOLS · line 16

Quoted 4 lines
package/uboot-tools/Config.in +3 -0
+       help
+         Select this to use the same repository specified for U-Boot.
 Otherwise,
+         the upstream sources will be used.
Quoted 9 lines

Just nitpicking, but IMO the newcomer might find it confusing: we are always downloading U-Boot sources, so what does this option change?

I would replace "specified for U-Boot" with "specified for the uboot package". This makes it more clear that you refer to the "uboot" package in Buildroot.

This is just a clarification, there is nothing wrong in your wording.

I'm ok with this change. I am not really good in English, so I have a hard time finding good formulation.

Quoted 25 lines
Quoted 13 lines
package/uboot-tools/Config.in +3 -0
+
+endif
+
  config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
        bool "mkimage"

help

package/uboot-tools/uboot-tools.mk +0 -0
index 398ce8b..367d067 100644

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 16

Quoted 7 lines
package/uboot-tools/uboot-tools.mk +4 -0
@@ -10,6 +10,15 @@ UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
  UBOOT_TOOLS_LICENSE = GPLv2+
  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
+       UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+       UBOOT_TOOLS_SOURCE  = $(UBOOT_SOURCE)
+       UBOOT_TOOLS_SITE    = $(UBOOT_SITE)

You are overriding the previously-defined options. I find an if/then/else construct much cleaner:

Quoted 6 lines

ifneq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
...use upstream sources... (current code)
else
...use sources for the uboot package...
endif

I'd like to have a few more opinions on that, after seeing the code in the form if/then/else, I find it awkward as it would be (as far as I know) the only package, where you don't find the SOURCE/SITE variable defined at the top of the file.
So I am not sure about this.

Quoted 4 lines

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 17

Quoted 1 line
package/uboot-tools/uboot-tools.mk +1 -0
+ifeq ($(UBOOT_SITE_METHOD),)

This should be an ifneq, not ifeq.

Thanks !

Quoted 14 lines

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 24

Quoted 10 lines
package/uboot-tools/uboot-tools.mk +4 -0
+       UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
+endif
+endif
+
  define UBOOT_TOOLS_BUILD_CMDS
        $(MAKE) -C $(@D)                        \
                HOSTCC="$(TARGET_CC)"           \

--
Luca

#3 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Hi Maxime,

Quoted 19 lines · Maxime Hadjinlian
Quoted 1 line · Luca Ceresoli

Hi Maxime,

Hi Luca

Quoted 16 lines
Quoted 9 lines · Maxime Hadjinlian

If the user has specified a custom U-Boot repository, he may also want to use it for U-Boot tools.

This could be usefull in two identified use case:
- User want the same version for U-Boot tools and U-Boot
- User has modified U-Boot tools in his U-Boot repository

Signed-off-by Maxime Hadjinlian <[email protected]>

I ACK the idea, but there are a few changes I would make to your patch, see below.

...

Quoted 23 lines
Quoted 17 lines

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 16

Quoted 12 lines

b/package/uboot-tools/uboot-tools.mk
index 398ce8b..367d067 100644

package/uboot-tools/uboot-tools.mk +4 -0
@@ -10,6 +10,15 @@ UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
   UBOOT_TOOLS_LICENSE = GPLv2+
   UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
+       UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+       UBOOT_TOOLS_SOURCE  = $(UBOOT_SOURCE)
+       UBOOT_TOOLS_SITE    = $(UBOOT_SITE)

You are overriding the previously-defined options. I find an if/then/else construct much cleaner:

Quoted 6 lines

ifneq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
...use upstream sources... (current code)
else
...use sources for the uboot package...
endif

Quoted 5 lines

I'd like to have a few more opinions on that, after seeing the code in the form if/then/else, I find it awkward as it would be (as far as I know) the only package, where you don't find the SOURCE/SITE variable defined at the top of the file.
So I am not sure about this.

Very few packages define their SOURCE/SITE variables conditionally:
linux, boot/ubot, boot/barebox, maybe a few more. These all are key components for every embedded Linux system and are often used in a modified form on real products, so they deserve this additional flexibility.

All of them have an ifeq / else ifeq / [else ifeq /...] else / endif construct in their .mk files to handle the various possibilities. I suggest you take one of them as an example.

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

This has the advantage that adding a third possibility would be simply a matter of adding another choice, without having to rework the whole thing and having to take care of backward compatibility.

But this may be overengineering for the relatively simple uboot-tools package.

Signature · 1 line
Luca
#4 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Quoted 9 lines · Luca Ceresoli

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

Quoted 7 lines

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

Actually, I don't even see the need to ask the user anything. If we are
building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
option, and instead make it conditional on BR2_TARGET_UBOOT.

Regards,
Arnout

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#5 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Hi Arnout,

Quoted 16 lines · Arnout Vandecappelle
Quoted 15 lines · Luca Ceresoli

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

Quoted 6 lines
Quoted 1 line

endchoice

Actually, I don't even see the need to ask the user anything. If we
are building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

Smart observation. I can provide no counterexample.

In all my use cases it would be either better or equivalent than the current behavior.

Quoted 3 lines

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
option, and instead make it conditional on BR2_TARGET_UBOOT.

That would be fine for me, but how about backward compatibility?

This would potentially break some existing projects, so it should appear in the Legacy menu, I guess.

Signature · 1 line
Luca
#6 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

In reply to Arnout Vandecappelle

Hi Arnout,

Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <[email protected]>:

Quoted 15 lines · Luca Ceresoli
Quoted 13 lines

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

Quoted 7 lines
Quoted 4 lines

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

Actually, I don't even see the need to ask the user anything. If we are

building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

Quoted 2 lines

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE

option, and instead make it conditional on BR2_TARGET_UBOOT.

I don't agree here. Real life example: we are using vendor-provided uboot, and want to set an env-image in our flash devices' factory image. While this version of uboot clearly supports handling the env at a specified location, it does not (yet) provide the mkenvimage tool.
In this case, we actually want a more recent uboot tools package that does have mkenvimage. My conclusion is thus that we should provide the choice.

Best regards,
Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140128/335d2afb/attachment.html>

author
#7 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

Hi all,

Quoted 33 lines · Thomas De Schampheleire

Hi Arnout,

Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <[email protected]>:

Quoted 21 lines · Luca Ceresoli
Quoted 10 lines

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

Quoted 7 lines

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

Quoted 6 lines

Actually, I don't even see the need to ask the user anything. If we are
building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
option, and instead make it conditional on BR2_TARGET_UBOOT.

Quoted 7 lines

I don't agree here. Real life example: we are using vendor-provided uboot, and want to set an env-image in our flash devices' factory image. While this version of uboot clearly supports handling the env at a specified location, it does not (yet) provide the mkenvimage tool.
In this case, we actually want a more recent uboot tools package that does have mkenvimage. My conclusion is thus that we should provide the choice.

I agree with Thomas here, as this has already happen to me too. I think the user's choice is the best option we could offer. I will do more testing regarding the different type of version one can use for u-boot. And as you said, Arnout, the order of inclusions, was the reason behind the uses of BR2_TARGET_UBOOT_VERSION.
I will test and resend with Luca's proposition to make the whole UBOOT_TOOLS_SOURCE part of an if/then/else structure.

Quoted 2 lines

Best regards,
Thomas

#8 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

In reply to Thomas De Schampheleire

Hi Thomas,

Quoted 35 lines · Thomas De Schampheleire

Hi Arnout,

Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <[email protected] <mailto:[email protected]>>:

Quoted 6 lines · Luca Ceresoli
Quoted 4 lines

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk

<http://barebox.mk>):

Quoted 9 lines
Quoted 9 lines

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

Quoted 7 lines
Quoted 4 lines

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

Actually, I don't even see the need to ask the user anything. If we

are building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

Quoted 2 lines

So I would propose to remove the

BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE option, and instead make it conditional on BR2_TARGET_UBOOT.

Quoted 7 lines

I don't agree here. Real life example: we are using vendor-provided uboot, and want to set an env-image in our flash devices' factory image. While this version of uboot clearly supports handling the env at a specified location, it does not (yet) provide the mkenvimage tool. In this case, we actually want a more recent uboot tools package that does have mkenvimage. My conclusion is thus that we should provide the choice.

And here's the counterexample! So, yes, I agree we should provide a choice.

Signature · 1 line
Luca
#9 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

In reply to Thomas De Schampheleire
Quoted 33 lines · Thomas De Schampheleire

Hi Arnout,

Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <[email protected] <mailto:[email protected]>>:

Quoted 15 lines · Luca Ceresoli
Quoted 13 lines

Going a step ahead, to be more uniform with these packages, you may use a choice construct to allow choosing between two alternatives. Example (modified version of the code in barebox.mk <http://barebox.mk>):

choice
prompt "version"
help
Select the specific uboot-tools version you want to use

config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
bool "Use a recent upstream version"

Quoted 7 lines
Quoted 4 lines

config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
bool "Use the same sources of the uboot package"

endchoice

Actually, I don't even see the need to ask the user anything. If we

are building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

Quoted 2 lines

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE

option, and instead make it conditional on BR2_TARGET_UBOOT.

Quoted 7 lines

I don't agree here. Real life example: we are using vendor-provided uboot, and want to set an env-image in our flash devices' factory image. While this version of uboot clearly supports handling the env at a specified location, it does not (yet) provide the mkenvimage tool. In this case, we actually want a more recent uboot tools package that does have mkenvimage. My conclusion is thus that we should provide the choice.

OK. In that case, I prefer the choice over the bare option. Note that
Kconfig doesn't support help text on the choice itself, only on the config entries. Also the default (_LATEST_VERSION) should be selected explicitly.

Regards,
Arnout

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#10 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

In reply to Thomas De Schampheleire

Dear Thomas De Schampheleire,

Quoted 5 lines · Thomas De Schampheleire
Quoted 1 line

Actually, I don't even see the need to ask the user anything. If we are

building U-Boot, I don't see why we would ever want to use the U-Boot tools from upstream - that just adds a risk of incompatibility between the two.

Quoted 2 lines

So I would propose to remove the BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE

Quoted 8 lines

option, and instead make it conditional on BR2_TARGET_UBOOT.

I don't agree here. Real life example: we are using vendor-provided uboot, and want to set an env-image in our flash devices' factory image. While this version of uboot clearly supports handling the env at a specified location, it does not (yet) provide the mkenvimage tool.
In this case, we actually want a more recent uboot tools package that does have mkenvimage. My conclusion is thus that we should provide the choice.

Then maybe you should just patch your vendor U-Boot to add mkenvimage, and that's it? :-)

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#11 lore raw

PATCH 1/1 uboot-tools: Allow users to use uboot's sources

In reply to Maxime Hadjinlian
Quoted 18 lines · Maxime Hadjinlian

If the user has specified a custom U-Boot repository, he may also want to use it for U-Boot tools.

This could be usefull in two identified use case:
- User want the same version for U-Boot tools and U-Boot
- User has modified U-Boot tools in his U-Boot repository

Signed-off-by Maxime Hadjinlian <[email protected]>
---
  package/uboot-tools/Config.in      | 10 ++++++++++
  package/uboot-tools/uboot-tools.mk |  9 +++++++++
  2 files changed, 19 insertions(+)
package/uboot-tools/Config.in +0 -0
index 7c8f17c..3742b0e 100644

package/uboot-tools/Config.in config BR2_PACKAGE_UBOOT_TOOLS · line 16

Quoted 10 lines
package/uboot-tools/Config.in +7 -0
@@ -7,6 +7,16 @@ config BR2_PACKAGE_UBOOT_TOOLS
 
  if BR2_PACKAGE_UBOOT_TOOLS
 
+if BR2_TARGET_UBOOT
+
+config BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE
+	bool "Use the same repository as U-Boot ?"
+	help
+	  Select this to use the same repository specified for U-Boot. Otherwise,
+	  the upstream sources will be used.

As I explain in a separate mail, I think this option is unnecessary.

Quoted 11 lines
package/uboot-tools/Config.in +3 -0
+
+endif
+
  config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
  	bool "mkimage"
  	help
package/uboot-tools/uboot-tools.mk +0 -0
index 398ce8b..367d067 100644

package/uboot-tools/uboot-tools.mk UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot · line 19

Quoted 10 lines
package/uboot-tools/uboot-tools.mk +7 -0
@@ -10,6 +10,15 @@ UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
  UBOOT_TOOLS_LICENSE = GPLv2+
  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_UBOOT_SOURCE),y)
+	UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+	UBOOT_TOOLS_SOURCE  = $(UBOOT_SOURCE)
+	UBOOT_TOOLS_SITE    = $(UBOOT_SITE)
+ifeq ($(UBOOT_SITE_METHOD),)
+	UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
+endif

I think UBOOT_LICENSE and UBOOT_LICENSE_FILES should also be taken
over. Especially the LICENSE_FILES (i.e. empty), otherwise legal-info will break.

Does this actually work if it doesn't come from an official tarball? I
think the UBOOT_TOOLS_SOURCE variable will evaluate to empty at the time it is ifndef'd in pkg-generic.mk, so it will be set to the default <pkg>-<version>.tar.gz. So anything that doesn't happen to be in this format (e.g. a custom tarball) will not work AFAICS. This is probably also the reason why you use BR2_TARGET_UBOOT_VERSION instead of simply UBOOT_VERSION.

In other words, something like this isn't reliable, because it depends
on the order of inclusion of the .mk files. I think this is actually a problem with the buildroot infrastructure: ideally, it should _not_ depend on the order of inclusion.

Regards,
Arnout

Quoted 6 lines
package/uboot-tools/uboot-tools.mk +2 -0
+endif
+
  define UBOOT_TOOLS_BUILD_CMDS
  	$(MAKE) -C $(@D) 			\
  		HOSTCC="$(TARGET_CC)"		\
Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F