[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Ezequiel Garcia]

It's often desirable to specify a list of patches, in addition to the current
support to pass a directory full of uboot-*.patch files. This matches the
option currently available for specifying Linux patches, except this commit
doesn't add the URL option. Relying on URLs is fragile and makes a build hard
to reproduce.

U-Boot is as special as Linux, in the sense that it needs to be customized more
often than not, so having a flexible mechanism is useful.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 Config.in.legacy     | 14 ++++++++++++++
 boot/uboot/Config.in | 11 ++++++-----
 boot/uboot/uboot.mk  | 12 ++++++++----
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/Config.in.legacy b/Config.in.legacy
index 23962a2..7fd8b9c 100644
--- a/Config.in.legacy
+++ b/Config.in.legacy
@@ -248,6 +248,20 @@ config BR2_PACKAGE_LIBV4L_V4L2_DBG
 	  This libv4l option has been deprecated and replaced by a single
 	  option to build all the libv4l utilities.
 
+config BR2_TARGET_UBOOT_PATCH
+	default BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR if BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR != ""
+
+config BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR
+	string "u-boot: the patches list option has been renamed"
+	help
+	  The option BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR has been renamed to
+	  BR2_TARGET_UBOOT_PATCH.
+
+config BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR_WRAP
+	bool
+	default y if BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR != ""
+	select BR2_LEGACY
+
 ###############################################################################
 comment "Legacy options removed in 2014.05"
 
diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 0f98d7f..ec7095a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -70,12 +70,13 @@ config BR2_TARGET_UBOOT_VERSION
 	default BR2_TARGET_UBOOT_CUSTOM_REPO_VERSION \
 		if BR2_TARGET_UBOOT_CUSTOM_GIT || BR2_TARGET_UBOOT_CUSTOM_HG
 
-config BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR
-	string "custom patch dir"
+config BR2_TARGET_UBOOT_PATCH
+	string "Custom U-Boot patches"
 	help
-	  If your board requires custom patches, add the path to the
-	  directory containing the patches here. The patches must be
-	  named uboot-<something>.patch.
+	  A space-separated list of patches to be applied.
+	  Each patch can be described as a local file path, or a directory.
+	  In the case of a directory, all files matching uboot-*.patch in
+	  the directory will be applied.
 
 	  Most users may leave this empty
 
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 532ac8b..753bccb 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -85,14 +85,18 @@ endef
 
 UBOOT_POST_EXTRACT_HOOKS += UBOOT_COPY_OLD_LICENSE_FILE
 
-ifneq ($(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR)),)
+UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
 define UBOOT_APPLY_CUSTOM_PATCHES
-	support/scripts/apply-patches.sh $(@D) $(BR2_TARGET_UBOOT_CUSTOM_PATCH_DIR) \
-		uboot-\*.patch
+	for p in $(UBOOT_PATCHES) ; do \
+		if test -d $$p ; then \
+			support/scripts/apply-patches.sh $(@D) $$p uboot-\*.patch ; \
+		else \
+			support/scripts/apply-patches.sh $(@D) `dirname $$p` `basename $$p` ; \
+		fi \
+	done
 endef
 
 UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_CUSTOM_PATCHES
-endif
 
 define UBOOT_CONFIGURE_CMDS
 	$(TARGET_CONFIGURE_OPTS) $(UBOOT_CONFIGURE_OPTS) 	\
-- 
1.9.1

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Tue, 15 Jul 2014 15:26:34 -0300, Ezequiel Garcia wrote:
> It's often desirable to specify a list of patches, in addition to the current
> support to pass a directory full of uboot-*.patch files. This matches the
> option currently available for specifying Linux patches, except this commit
> doesn't add the URL option. Relying on URLs is fragile and makes a build hard
> to reproduce.
> 
> U-Boot is as special as Linux, in the sense that it needs to be customized more
> often than not, so having a flexible mechanism is useful.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  Config.in.legacy     | 14 ++++++++++++++
>  boot/uboot/Config.in | 11 ++++++-----
>  boot/uboot/uboot.mk  | 12 ++++++++----
>  3 files changed, 28 insertions(+), 9 deletions(-)

We now have BR2_GLOBAL_PATCH_DIR, so I believe that what we would
recommend to use from now on. We have even been talking about removing
the Linux-specific and U-Boot specific patching config options.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Ezequiel García]

On 15 July 2014 15:53, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Ezequiel Garcia,
>
> On Tue, 15 Jul 2014 15:26:34 -0300, Ezequiel Garcia wrote:
>> It's often desirable to specify a list of patches, in addition to the current
>> support to pass a directory full of uboot-*.patch files. This matches the
>> option currently available for specifying Linux patches, except this commit
>> doesn't add the URL option. Relying on URLs is fragile and makes a build hard
>> to reproduce.
>>
>> U-Boot is as special as Linux, in the sense that it needs to be customized more
>> often than not, so having a flexible mechanism is useful.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  Config.in.legacy     | 14 ++++++++++++++
>>  boot/uboot/Config.in | 11 ++++++-----
>>  boot/uboot/uboot.mk  | 12 ++++++++----
>>  3 files changed, 28 insertions(+), 9 deletions(-)
>
> We now have BR2_GLOBAL_PATCH_DIR, so I believe that what we would
> recommend to use from now on. We have even been talking about removing
> the Linux-specific and U-Boot specific patching config options.
>

Yeah, Yann already said that on IRC when I asked him about this patch.
You guys are really in sync! :)

The thing is that Linux and U-Boot are not the regular package, since
almost every BR user using it for a custom product will need to
customize the kernel and the bootloader to some extent.

At least from my perspective, it's nice to have the patches (instead
of pointing to some git branch) and be forced to see what
modifications I'm using.

And to produce the patches, I use git-format-patch (aren't we all?),
which doesn't allow to specify a prefix for the patches. This has been
discussed and rejected, so don't think the feature will ever be added.

Sure, I can use a script, but these options seemed natural enough and
it allows for a simple "git-format-patch" run and dump the files to a
directory.

Anyway... this is just my use case. I understand your desire to remove
strange options and reduce the churn.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Yann E. MORIN]

Ezequiel, Thomas, All,

On 2014-07-15 16:49 -0300, Ezequiel Garc?a spake thusly:
> On 15 July 2014 15:53, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Dear Ezequiel Garcia,
> >
> > On Tue, 15 Jul 2014 15:26:34 -0300, Ezequiel Garcia wrote:
> >> It's often desirable to specify a list of patches, in addition to the current
> >> support to pass a directory full of uboot-*.patch files. This matches the
> >> option currently available for specifying Linux patches, except this commit
> >> doesn't add the URL option. Relying on URLs is fragile and makes a build hard
> >> to reproduce.
> >>
> >> U-Boot is as special as Linux, in the sense that it needs to be customized more
> >> often than not, so having a flexible mechanism is useful.
> >>
> >> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >> ---
> >>  Config.in.legacy     | 14 ++++++++++++++
> >>  boot/uboot/Config.in | 11 ++++++-----
> >>  boot/uboot/uboot.mk  | 12 ++++++++----
> >>  3 files changed, 28 insertions(+), 9 deletions(-)
> >
> > We now have BR2_GLOBAL_PATCH_DIR, so I believe that what we would
> > recommend to use from now on. We have even been talking about removing
> > the Linux-specific and U-Boot specific patching config options.
> >
> 
> Yeah, Yann already said that on IRC when I asked him about this patch.
> You guys are really in sync! :)
> 
> The thing is that Linux and U-Boot are not the regular package, since
> almost every BR user using it for a custom product will need to
> customize the kernel and the bootloader to some extent.
> 
> At least from my perspective, it's nice to have the patches (instead
> of pointing to some git branch) and be forced to see what
> modifications I'm using.
> 
> And to produce the patches, I use git-format-patch (aren't we all?),
> which doesn't allow to specify a prefix for the patches. This has been
> discussed and rejected, so don't think the feature will ever be added.

Now I remember that we talked about this on IRC the other day.

The more I think about it, the more I find our policy to require
PKG-prefixed patches to be really cumbersome, since the patches already
are in a subdir named PKG/

Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
be in sync with what we do for our bundled patches.

But still, if patches were just named NNNN-title.patch, that would be as
efficient at sorting the patches. The PKG- prefix is not really
required, and indeed can cause some troubles with some use-cases, such
as yours.

Thomas, was there a specific reason we wanted the patches to be
PKG-prefixed? If not, would it make sense to just accept patches without
a PKG-prefix?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Thomas Petazzoni]

Dear Yann E. MORIN,

On Tue, 15 Jul 2014 22:13:36 +0200, Yann E. MORIN wrote:

> The more I think about it, the more I find our policy to require
> PKG-prefixed patches to be really cumbersome, since the patches already
> are in a subdir named PKG/

Actually, me too. It prevents the output of "git format-patch" from
being directly used as patches.

> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
> be in sync with what we do for our bundled patches.
> 
> But still, if patches were just named NNNN-title.patch, that would be as
> efficient at sorting the patches. The PKG- prefix is not really
> required, and indeed can cause some troubles with some use-cases, such
> as yours.
> 
> Thomas, was there a specific reason we wanted the patches to be
> PKG-prefixed? If not, would it make sense to just accept patches without
> a PKG-prefix?

I honestly don't remember what was the reasoning behind that. Arnout
maybe?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Thomas De Schampheleire]

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> schreef:
>Dear Yann E. MORIN,
>
>On Tue, 15 Jul 2014 22:13:36 +0200, Yann E. MORIN wrote:
>
>> The more I think about it, the more I find our policy to require
>> PKG-prefixed patches to be really cumbersome, since the patches already
>> are in a subdir named PKG/
>
>Actually, me too. It prevents the output of "git format-patch" from
>being directly used as patches.
>
>> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
>> be in sync with what we do for our bundled patches.
>> 
>> But still, if patches were just named NNNN-title.patch, that would be as
>> efficient at sorting the patches. The PKG- prefix is not really
>> required, and indeed can cause some troubles with some use-cases, such
>> as yours.
>> 
>> Thomas, was there a specific reason we wanted the patches to be
>> PKG-prefixed? If not, would it make sense to just accept patches without
>> a PKG-prefix?
>
>I honestly don't remember what was the reasoning behind that. Arnout
>maybe?

Not sure either...

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Arnout Vandecappelle]

On 15/07/14 22:13, Yann E. MORIN wrote:
> The more I think about it, the more I find our policy to require
> PKG-prefixed patches to be really cumbersome, since the patches already
> are in a subdir named PKG/
> 
> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
> be in sync with what we do for our bundled patches.
> 
> But still, if patches were just named NNNN-title.patch, that would be as
> efficient at sorting the patches. The PKG- prefix is not really
> required, and indeed can cause some troubles with some use-cases, such
> as yours.
> 
> Thomas, was there a specific reason we wanted the patches to be
> PKG-prefixed? If not, would it make sense to just accept patches without
> a PKG-prefix?

 Er, we don't... We require this specific naming scheme for contributed
packages, but the code itself just takes *.patch.

 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] u-boot: Allow to specify a list of patches

[Thomas De Schampheleire]

Arnout Vandecappelle <arnout@mind.be> schreef:
>On 15/07/14 22:13, Yann E. MORIN wrote:
>> The more I think about it, the more I find our policy to require
>> PKG-prefixed patches to be really cumbersome, since the patches already
>> are in a subdir named PKG/
>> 
>> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
>> be in sync with what we do for our bundled patches.
>> 
>> But still, if patches were just named NNNN-title.patch, that would be as
>> efficient at sorting the patches. The PKG- prefix is not really
>> required, and indeed can cause some troubles with some use-cases, such
>> as yours.
>> 
>> Thomas, was there a specific reason we wanted the patches to be
>> PKG-prefixed? If not, would it make sense to just accept patches without
>> a PKG-prefix?
>
> Er, we don't... We require this specific naming scheme for contributed
>packages, but the code itself just takes *.patch.

Yes sure, it's a convention only, but the question is: why did we include the package name in the convention?

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Arnout Vandecappelle]

On 17/07/14 06:52, Thomas De Schampheleire wrote:
> Arnout Vandecappelle <arnout@mind.be> schreef:
>> On 15/07/14 22:13, Yann E. MORIN wrote:
>>> The more I think about it, the more I find our policy to require
>>> PKG-prefixed patches to be really cumbersome, since the patches already
>>> are in a subdir named PKG/
>>>
>>> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
>>> be in sync with what we do for our bundled patches.
>>>
>>> But still, if patches were just named NNNN-title.patch, that would be as
>>> efficient at sorting the patches. The PKG- prefix is not really
>>> required, and indeed can cause some troubles with some use-cases, such
>>> as yours.
>>>
>>> Thomas, was there a specific reason we wanted the patches to be
>>> PKG-prefixed? If not, would it make sense to just accept patches without
>>> a PKG-prefix?
>>
>> Er, we don't... We require this specific naming scheme for contributed
>> packages, but the code itself just takes *.patch.
> 
> Yes sure, it's a convention only, but the question is: why did we include the package name in the convention?

 I think it's purely historical. And I think it never was required for patches
in a <pkgname> subdir.


 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] u-boot: Allow to specify a list of patches

[Yann E. MORIN]

Arnout, All,

On 2014-07-17 09:48 +0200, Arnout Vandecappelle spake thusly:
> On 17/07/14 06:52, Thomas De Schampheleire wrote:
> > Arnout Vandecappelle <arnout@mind.be> schreef:
> >> On 15/07/14 22:13, Yann E. MORIN wrote:
> >>> The more I think about it, the more I find our policy to require
> >>> PKG-prefixed patches to be really cumbersome, since the patches already
> >>> are in a subdir named PKG/
> >>>
> >>> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
> >>> be in sync with what we do for our bundled patches.
> >>>
> >>> But still, if patches were just named NNNN-title.patch, that would be as
> >>> efficient at sorting the patches. The PKG- prefix is not really
> >>> required, and indeed can cause some troubles with some use-cases, such
> >>> as yours.
> >>>
> >>> Thomas, was there a specific reason we wanted the patches to be
> >>> PKG-prefixed? If not, would it make sense to just accept patches without
> >>> a PKG-prefix?
> >>
> >> Er, we don't... We require this specific naming scheme for contributed
> >> packages, but the code itself just takes *.patch.
> > 
> > Yes sure, it's a convention only, but the question is: why did we include the package name in the convention?
> 
>  I think it's purely historical. And I think it never was required for patches
> in a <pkgname> subdir.

So we could change the manual to not require patches to be PKG-prefixed?

As long as they are number-prefixed, that's all we need, right?

So, Ezequiel's patch is really no longer needed, and his use-case to use
git-formatted patches is already covered, right?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] u-boot: Allow to specify a list of patches

[Arnout Vandecappelle]

On 17/07/14 19:23, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2014-07-17 09:48 +0200, Arnout Vandecappelle spake thusly:
>> On 17/07/14 06:52, Thomas De Schampheleire wrote:
>>> Arnout Vandecappelle <arnout@mind.be> schreef:
>>>> On 15/07/14 22:13, Yann E. MORIN wrote:
>>>>> The more I think about it, the more I find our policy to require
>>>>> PKG-prefixed patches to be really cumbersome, since the patches already
>>>>> are in a subdir named PKG/
>>>>>
>>>>> Of course, we're enforcing this naming scheme in BR2_GLOBAL_PATCH_DIR to
>>>>> be in sync with what we do for our bundled patches.
>>>>>
>>>>> But still, if patches were just named NNNN-title.patch, that would be as
>>>>> efficient at sorting the patches. The PKG- prefix is not really
>>>>> required, and indeed can cause some troubles with some use-cases, such
>>>>> as yours.
>>>>>
>>>>> Thomas, was there a specific reason we wanted the patches to be
>>>>> PKG-prefixed? If not, would it make sense to just accept patches without
>>>>> a PKG-prefix?
>>>>
>>>> Er, we don't... We require this specific naming scheme for contributed
>>>> packages, but the code itself just takes *.patch.
>>>
>>> Yes sure, it's a convention only, but the question is: why did we include the package name in the convention?
>>
>>  I think it's purely historical. And I think it never was required for patches
>> in a <pkgname> subdir.
> 
> So we could change the manual to not require patches to be PKG-prefixed?
> 
> As long as they are number-prefixed, that's all we need, right?
> 
> So, Ezequiel's patch is really no longer needed, and his use-case to use
> git-formatted patches is already covered, right?

 Yep.

 Untested, of course :-)


 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] u-boot: Allow to specify a list of patches

[Ezequiel García]

Hi everyone,

On 18 Jul 01:20 AM, Arnout Vandecappelle wrote:
[..]
> >>>>>
> >>>>> Thomas, was there a specific reason we wanted the patches to be
> >>>>> PKG-prefixed? If not, would it make sense to just accept patches without
> >>>>> a PKG-prefix?
> >>>>
> >>>> Er, we don't... We require this specific naming scheme for contributed
> >>>> packages, but the code itself just takes *.patch.
> >>>
> >>> Yes sure, it's a convention only, but the question is: why did we include the package name in the convention?
> >>
> >>  I think it's purely historical. And I think it never was required for patches
> >> in a <pkgname> subdir.
> > 
> > So we could change the manual to not require patches to be PKG-prefixed?
> > 
> > As long as they are number-prefixed, that's all we need, right?
> > 
> > So, Ezequiel's patch is really no longer needed, and his use-case to use
> > git-formatted patches is already covered, right?
> 
>  Yep.
> 
>  Untested, of course :-)
> 

I did a quick test in here. It seems GLOBAL_PATCH_DIR works perfectly,
and it's a lot cleaner than having N options, one for each supposedly special
package, as it's keeps all the modification to vanilla in one place.

So I strongly support removing all the UBOOT_PATCH, LINUX_PATCH, and any other
FOO_PATCH options out there.

Any brave hacker stepping up? :)
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar