Patchwork [v2] uboot-tools: Allow users to use uboot's sources

login
register
mail settings
Submitter Maxime Hadjinlian
Date Feb. 3, 2014, 12:05 a.m.
Message ID <1391385955-13166-1-git-send-email-maxime.hadjinlian@gmail.com>
Download mbox | patch
Permalink /patch/316047/
State Changes Requested
Headers show

Comments

Maxime Hadjinlian - Feb. 3, 2014, 12:05 a.m.
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>
---
Changes v1 -> v2:
    - Add a choice option (Suggested by Luca Ceresoli)
    - Add an if/then/else structure (Suggested by Luca Ceresoli)
    - Take UBOOT_LICENSE and UBOOT_LICENCE_FILES into account (Suggested
      by Arnout Vandecappelle)
---
 package/uboot-tools/Config.in      | 18 ++++++++++++++++++
 package/uboot-tools/uboot-tools.mk | 17 +++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)
Thomas Petazzoni - Feb. 3, 2014, 9:58 p.m.
Dear Maxime Hadjinlian,

On Mon,  3 Feb 2014 01:05:55 +0100, Maxime Hadjinlian wrote:

> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
> index 7c8f17c..29580dd 100644
> --- a/package/uboot-tools/Config.in
> +++ b/package/uboot-tools/Config.in
> @@ -7,6 +7,24 @@ config BR2_PACKAGE_UBOOT_TOOLS
>  
>  if BR2_PACKAGE_UBOOT_TOOLS
>  
> +if BR2_TARGET_UBOOT
> +
> +choice
> +	prompt "version"
> +	default BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
> +	help
> +	  Select the specific uboot-tools version you want to use
> +
> +config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
> +	bool "Use latest upstream version"
> +
> +config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
> +	bool "Use the same version as the uboot package"

Shouldn't this option only be available if the U-Boot package has been
enabled in the configuration?

> +ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION),y)
> +# We have to use the BR2_TARGET_UBOOT_VERSION because of the order of inclusion
> +# of the mk files.
> +UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))

That's more of a question for Arnout, but does the order of .mk files
inclusion matter since we're using '=' and not ':=' ?

> +UBOOT_TOOLS_SOURCE = $(UBOOT_SOURCE)
> +UBOOT_TOOLS_SITE = $(UBOOT_SITE)
> +ifneq ($(UBOOT_SITE_METHOD),)
> +UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
> +endif
> +UBOOT_TOOLS_LICENSE = $(UBOOT_LICENSE)
> +UBOOT_TOOLS_LICENSE_FILES = $(UBOOT_LICENSE_FILES)
> +else
>  UBOOT_TOOLS_VERSION = 2014.01
> -UBOOT_TOOLS_SOURCE  = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
> -UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
> +UBOOT_TOOLS_SOURCE = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
> +UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot
>  UBOOT_TOOLS_LICENSE = GPLv2+
>  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
> +endif
>  
>  define UBOOT_TOOLS_BUILD_CMDS
>  	$(MAKE) -C $(@D) 			\

Thomas
Maxime Hadjinlian - Feb. 3, 2014, 10:01 p.m.
On Mon, Feb 3, 2014 at 10:58 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Maxime Hadjinlian,
>
> On Mon,  3 Feb 2014 01:05:55 +0100, Maxime Hadjinlian wrote:
>
>> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
>> index 7c8f17c..29580dd 100644
>> --- a/package/uboot-tools/Config.in
>> +++ b/package/uboot-tools/Config.in
>> @@ -7,6 +7,24 @@ config BR2_PACKAGE_UBOOT_TOOLS
>>
>>  if BR2_PACKAGE_UBOOT_TOOLS
>>
>> +if BR2_TARGET_UBOOT
>> +
>> +choice
>> +     prompt "version"
>> +     default BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
>> +     help
>> +       Select the specific uboot-tools version you want to use
>> +
>> +config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
>> +     bool "Use latest upstream version"
>> +
>> +config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
>> +     bool "Use the same version as the uboot package"
>
> Shouldn't this option only be available if the U-Boot package has been
> enabled in the configuration?
It is already in this patch.
>
>> +ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION),y)
>> +# We have to use the BR2_TARGET_UBOOT_VERSION because of the order of inclusion
>> +# of the mk files.
>> +UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
>
> That's more of a question for Arnout, but does the order of .mk files
> inclusion matter since we're using '=' and not ':=' ?
Arnout will confirm (or not, if I misunderstood) with much more
detail, but I found that the inclusion did matter, even with the use
of '='.
>
>> +UBOOT_TOOLS_SOURCE = $(UBOOT_SOURCE)
>> +UBOOT_TOOLS_SITE = $(UBOOT_SITE)
>> +ifneq ($(UBOOT_SITE_METHOD),)
>> +UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
>> +endif
>> +UBOOT_TOOLS_LICENSE = $(UBOOT_LICENSE)
>> +UBOOT_TOOLS_LICENSE_FILES = $(UBOOT_LICENSE_FILES)
>> +else
>>  UBOOT_TOOLS_VERSION = 2014.01
>> -UBOOT_TOOLS_SOURCE  = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
>> -UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
>> +UBOOT_TOOLS_SOURCE = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
>> +UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot
>>  UBOOT_TOOLS_LICENSE = GPLv2+
>>  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
>> +endif
>>
>>  define UBOOT_TOOLS_BUILD_CMDS
>>       $(MAKE) -C $(@D)                        \
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
Arnout Vandecappelle - Feb. 5, 2014, 5:36 p.m.
On 03/02/14 23:01, Maxime Hadjinlian wrote:
> On Mon, Feb 3, 2014 at 10:58 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
>> Dear Maxime Hadjinlian,
>>
>> On Mon,  3 Feb 2014 01:05:55 +0100, Maxime Hadjinlian wrote:
>>
>>> diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
>>> index 7c8f17c..29580dd 100644
>>> --- a/package/uboot-tools/Config.in
>>> +++ b/package/uboot-tools/Config.in
>>> @@ -7,6 +7,24 @@ config BR2_PACKAGE_UBOOT_TOOLS
>>>
>>>  if BR2_PACKAGE_UBOOT_TOOLS
>>>
>>> +if BR2_TARGET_UBOOT
>>> +
>>> +choice
>>> +     prompt "version"
>>> +     default BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
>>> +     help
>>> +       Select the specific uboot-tools version you want to use
>>> +
>>> +config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
>>> +     bool "Use latest upstream version"
>>> +
>>> +config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
>>> +     bool "Use the same version as the uboot package"
>>
>> Shouldn't this option only be available if the U-Boot package has been
>> enabled in the configuration?
> It is already in this patch.
>>
>>> +ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION),y)
>>> +# We have to use the BR2_TARGET_UBOOT_VERSION because of the order of inclusion
>>> +# of the mk files.
>>> +UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
>>
>> That's more of a question for Arnout, but does the order of .mk files
>> inclusion matter since we're using '=' and not ':=' ?
> Arnout will confirm (or not, if I misunderstood) with much more
> detail, but I found that the inclusion did matter, even with the use
> of '='.

 Indeed it does. The _VERSION is evaluated in generic-package and set to
'undefined' if it is empty. Since UBOOT_VERSION is only assigned later,
when uboot.mk is included, UBOOT_TOOLS_VERSION is empty at the time it is
evaluated. So it would always be undefined.

>>
>>> +UBOOT_TOOLS_SOURCE = $(UBOOT_SOURCE)

 This statement is useless, for the same reason that you cannot use
UBOOT_TOOLS_VERSION = $(UBOOT_VERSION). In generic-package,
UBOOT_TOOLS_SOURCE is evaluated to be empty so it will be set to the
default $$($(2)_RAWNAME)-$$($(2)_VERSION).tar.gz. The default is OK in
most cases, however, that's why you didn't notice.

>>> +UBOOT_TOOLS_SITE = $(UBOOT_SITE)
>>> +ifneq ($(UBOOT_SITE_METHOD),)

 Since UBOOT_SITE_METHOD is not defined yet, this condition will never be
used.

>>> +UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
>>> +endif
>>> +UBOOT_TOOLS_LICENSE = $(UBOOT_LICENSE)

 This one happens to work because generic-package uses a ?= for this one.
However, it will _not_ work for host-uboot-tools, because there an ifdef
is used.


 As you can see, all of this is extremely fragile. In essence, you need
to repeat all the logic from uboot.mk to derive these variables - or you
have to check really really carefully if it works in all cases, for both
uboot-tools and host-uboot-tools.

 Bottom line: I don't think this is the way to do it. I mentioned it
briefly in the developer days: there should be infrastructural support
for a shared-source package like this, so that the problem can be solved
fundamentally and it never depends on the order of evaluation, and it
doesn't break when a ?= in the package-generic is replaced with an ifndef.

 The generic structure of the shared source support would be something
like this:

if shared source

 assign VERSION, SOURCE, SITE, SITE_METHOD, LICENSE, LICENSE_FILES
according to the base package (with ?= so the derived package can still
override it).

else

 use the current way of assigning these variables

endif

 There are some more manipulations required. E.g., the legal-info
infrastructure makes very heavy use of conditions, so that has to be
rewritten as shell conditions.


 Have fun with that :-)


 Regards,
 Arnout


>>> +UBOOT_TOOLS_LICENSE_FILES = $(UBOOT_LICENSE_FILES)
>>> +else
>>>  UBOOT_TOOLS_VERSION = 2014.01
>>> -UBOOT_TOOLS_SOURCE  = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
>>> -UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
>>> +UBOOT_TOOLS_SOURCE = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
>>> +UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot
>>>  UBOOT_TOOLS_LICENSE = GPLv2+
>>>  UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
>>> +endif
>>>
>>>  define UBOOT_TOOLS_BUILD_CMDS
>>>       $(MAKE) -C $(@D)                        \
>>
>> Thomas
>> --
>> Thomas Petazzoni, CTO, Free Electrons
>> Embedded Linux, Kernel and Android engineering
>> http://free-electrons.com
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>

Patch

diff --git a/package/uboot-tools/Config.in b/package/uboot-tools/Config.in
index 7c8f17c..29580dd 100644
--- a/package/uboot-tools/Config.in
+++ b/package/uboot-tools/Config.in
@@ -7,6 +7,24 @@  config BR2_PACKAGE_UBOOT_TOOLS
 
 if BR2_PACKAGE_UBOOT_TOOLS
 
+if BR2_TARGET_UBOOT
+
+choice
+	prompt "version"
+	default BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
+	help
+	  Select the specific uboot-tools version you want to use
+
+config BR2_PACKAGE_UBOOT_TOOLS_LATEST_VERSION
+	bool "Use latest upstream version"
+
+config BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION
+	bool "Use the same version as the uboot package"
+
+endchoice
+
+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..71c64ee 100644
--- a/package/uboot-tools/uboot-tools.mk
+++ b/package/uboot-tools/uboot-tools.mk
@@ -4,11 +4,24 @@ 
 #
 ################################################################################
 
+ifeq ($(BR2_PACKAGE_UBOOT_TOOLS_USE_UBOOT_VERSION),y)
+# We have to use the BR2_TARGET_UBOOT_VERSION because of the order of inclusion
+# of the mk files.
+UBOOT_TOOLS_VERSION = $(call qstrip,$(BR2_TARGET_UBOOT_VERSION))
+UBOOT_TOOLS_SOURCE = $(UBOOT_SOURCE)
+UBOOT_TOOLS_SITE = $(UBOOT_SITE)
+ifneq ($(UBOOT_SITE_METHOD),)
+UBOOT_TOOL_SITE_METHOD = $(UBOOT_SITE_METHOD)
+endif
+UBOOT_TOOLS_LICENSE = $(UBOOT_LICENSE)
+UBOOT_TOOLS_LICENSE_FILES = $(UBOOT_LICENSE_FILES)
+else
 UBOOT_TOOLS_VERSION = 2014.01
-UBOOT_TOOLS_SOURCE  = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
-UBOOT_TOOLS_SITE    = ftp://ftp.denx.de/pub/u-boot
+UBOOT_TOOLS_SOURCE = u-boot-$(UBOOT_TOOLS_VERSION).tar.bz2
+UBOOT_TOOLS_SITE = ftp://ftp.denx.de/pub/u-boot
 UBOOT_TOOLS_LICENSE = GPLv2+
 UBOOT_TOOLS_LICENSE_FILES = Licenses/gpl-2.0.txt
+endif
 
 define UBOOT_TOOLS_BUILD_CMDS
 	$(MAKE) -C $(@D) 			\