[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[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 <maxime.hadjinlian@gmail.com>
---
 package/uboot-tools/Config.in      | 10 ++++++++++
 package/uboot-tools/uboot-tools.mk |  9 +++++++++
 2 files changed, 19 insertions(+)

diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
index 7c8f17c..3742b0e 100644
--- a/package/uboot-tools/Config.in
+++ b/package/uboot-tools/Config.in
@@ -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
diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk
index 398ce8b..367d067 100644
--- a/package/uboot-tools/uboot-tools.mk
+++ b/package/uboot-tools/uboot-tools.mk
@@ -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)"		\
-- 
1.8.5.2

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Luca Ceresoli]

Hi Maxime,

Maxime Hadjinlian wrote:
> 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 <maxime.hadjinlian@gmail.com>

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

> ---
>   package/uboot-tools/Config.in      | 10 ++++++++++
>   package/uboot-tools/uboot-tools.mk |  9 +++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
> index 7c8f17c..3742b0e 100644
> --- a/package/uboot-tools/Config.in
> +++ b/package/uboot-tools/Config.in
> @@ -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.

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

> +
> +endif
> +
>   config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
>   	bool "mkimage"
>   	help
> diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk
> index 398ce8b..367d067 100644
> --- a/package/uboot-tools/uboot-tools.mk
> +++ b/package/uboot-tools/uboot-tools.mk
> @@ -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

> +ifeq ($(UBOOT_SITE_METHOD),)

This should be an ifneq, not ifeq.

> +	UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
> +endif
> +endif
> +
>   define UBOOT_TOOLS_BUILD_CMDS
>   	$(MAKE) -C $(@D) 			\
>   		HOSTCC="$(TARGET_CC)"		\
>


-- 
Luca

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Maxime Hadjinlian]

On Sun, Jan 26, 2014 at 4:59 PM, Luca Ceresoli <luca@lucaceresoli.net> wrote:
> Hi Maxime,
Hi Luca
>
>
> Maxime Hadjinlian wrote:
>>
>> 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 <maxime.hadjinlian@gmail.com>
>
>
> I ACK the idea, but there are a few changes I would make to your patch,
> see below.
>
>
>> ---
>>   package/uboot-tools/Config.in      | 10 ++++++++++
>>   package/uboot-tools/uboot-tools.mk |  9 +++++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
>> index 7c8f17c..3742b0e 100644
>> --- a/package/uboot-tools/Config.in
>> +++ b/package/uboot-tools/Config.in
>> @@ -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.
>
>
>> +       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.
>
I'm ok with this change. I am not really good in English, so I have a
hard time finding good formulation.
>
>> +
>> +endif
>> +
>>   config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
>>         bool "mkimage"
>>         help
>> diff --git a/package/uboot-tools/uboot-tools.mk
>> b/package/uboot-tools/uboot-tools.mk
>> index 398ce8b..367d067 100644
>> --- a/package/uboot-tools/uboot-tools.mk
>> +++ b/package/uboot-tools/uboot-tools.mk
>> @@ -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
>
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.
>> +ifeq ($(UBOOT_SITE_METHOD),)
>
>
> This should be an ifneq, not ifeq.
Thanks !
>
>
>> +       UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
>> +endif
>> +endif
>> +
>>   define UBOOT_TOOLS_BUILD_CMDS
>>         $(MAKE) -C $(@D)                        \
>>                 HOSTCC="$(TARGET_CC)"           \
>>
>
>
> --
> Luca

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Luca Ceresoli]

Hi Maxime,

Maxime Hadjinlian wrote:
> On Sun, Jan 26, 2014 at 4:59 PM, Luca Ceresoli <luca@lucaceresoli.net> wrote:
>> Hi Maxime,
> Hi Luca
>>
>>
>> Maxime Hadjinlian wrote:
>>>
>>> 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 <maxime.hadjinlian@gmail.com>
>>
>>
>> I ACK the idea, but there are a few changes I would make to your patch,
>> see below.

...

>>> b/package/uboot-tools/uboot-tools.mk
>>> index 398ce8b..367d067 100644
>>> --- a/package/uboot-tools/uboot-tools.mk
>>> +++ b/package/uboot-tools/uboot-tools.mk
>>> @@ -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
>>
> 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.

-- 
Luca

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Arnout Vandecappelle]

On 26/01/14 22:43, Luca Ceresoli wrote:
> 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

  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

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

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Arnout Vandecappelle]

On 26/01/14 01:35, Maxime Hadjinlian wrote:
> 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 <maxime.hadjinlian@gmail.com>
> ---
>   package/uboot-tools/Config.in      | 10 ++++++++++
>   package/uboot-tools/uboot-tools.mk |  9 +++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
> index 7c8f17c..3742b0e 100644
> --- a/package/uboot-tools/Config.in
> +++ b/package/uboot-tools/Config.in
> @@ -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.

> +
> +endif
> +
>   config BR2_PACKAGE_UBOOT_TOOLS_MKIMAGE
>   	bool "mkimage"
>   	help
> diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk
> index 398ce8b..367d067 100644
> --- a/package/uboot-tools/uboot-tools.mk
> +++ b/package/uboot-tools/uboot-tools.mk
> @@ -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

> +endif
> +
>   define UBOOT_TOOLS_BUILD_CMDS
>   	$(MAKE) -C $(@D) 			\
>   		HOSTCC="$(TARGET_CC)"		\
>


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

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Luca Ceresoli]

Hi Arnout,

Arnout Vandecappelle wrote:
> On 26/01/14 22:43, Luca Ceresoli wrote:
>> 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
>
>   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.

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


-- 
Luca

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Thomas De Schampheleire]

Hi Arnout,

Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <arnout@mind.be>:
>
> On 26/01/14 22:43, Luca Ceresoli wrote:
>>
>> 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
>
>
>  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.

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>

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Maxime Hadjinlian]

Hi all,

On Tue, Jan 28, 2014 at 7:02 AM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Hi Arnout,
>
> Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <arnout@mind.be>:
>
>
>>
>> On 26/01/14 22:43, Luca Ceresoli wrote:
>>>
>>> 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
>>
>>
>>  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.
>
> 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.
> Best regards,
> Thomas

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Luca Ceresoli]

Hi Thomas,

Thomas De Schampheleire wrote:
> Hi Arnout,
>
> Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <arnout@mind.be
> <mailto:arnout@mind.be>>:
>  >
>  > On 26/01/14 22:43, Luca Ceresoli wrote:
>  >>
>  >> 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"
>  >>
>  >> 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.
>
> 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.

-- 
Luca

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Arnout Vandecappelle]

On 28/01/14 07:02, Thomas De Schampheleire wrote:
> Hi Arnout,
>
> Op 27-jan.-2014 22:28 schreef "Arnout Vandecappelle" <arnout@mind.be
> <mailto:arnout@mind.be>>:
>  >
>  > On 26/01/14 22:43, Luca Ceresoli wrote:
>  >>
>  >> 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"
>  >>
>  >> 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.
>
> 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


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

[Buildroot] [PATCH] uboot-tools: Allow users to use uboot's sources

[Thomas Petazzoni]

Dear Thomas De Schampheleire,

On Tue, 28 Jan 2014 07:02:56 +0100, Thomas De Schampheleire wrote:

> >  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.
> 
> 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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com