[Buildroot] [PATCH v2] rework patch model

[Buildroot] [PATCH v2] rework patch model

[spdawson at gmail.com]

From: Simon Dawson <spdawson@gmail.com>

At the Buildroot Developers Meeting (4-5 February 2013, in Brussels) a change
to the patch logic was discussed. See 

http://elinux.org/Buildroot:DeveloperDaysFOSDEM2013

for details. In summary:

* For patches stored in the package directory, if package/<pkg>/<version>/ does exist, apply package/<pkg>/<version>/*.patch, otherwise, apply package/<pkg>/*.patch
* For patches stored in the global patches directory, if $(GLOBAL_PATCH_DIR)/<pkg>/<version>/ does exist, apply $(GLOBAL_PATCH_DIR)/<pkg>/<version>/*.patch, otherwise, apply $(GLOBAL_PATCH_DIR)/<pkg>/*.patch

This patch adds the new BR2_GLOBAL_PATCH_DIR configuration item, and reworks
the generic package infrastructure to implement the new patch logic.

Signed-off-by: Simon Dawson <spdawson@gmail.com>
---
 v2: Add documentation, as suggested by Baruch Siach

 Config.in                          |   15 +++++++++++++++
 docs/manual/customize-packages.txt |   23 +++++++++++++++++++++++
 docs/manual/customize.txt          |    2 ++
 package/pkg-generic.mk             |   16 ++++++++--------
 4 files changed, 48 insertions(+), 8 deletions(-)
 create mode 100644 docs/manual/customize-packages.txt

diff --git a/Config.in b/Config.in
index a2c305f..03c4a3e 100644
--- a/Config.in
+++ b/Config.in
@@ -437,6 +437,21 @@ config BR2_PACKAGE_OVERRIDE_FILE
 	  as the source directory for a particular package. See the
 	  Buildroot documentation for more details on this feature.
 
+config BR2_GLOBAL_PATCH_DIR
+	string "global patch directory"
+	help
+	  You may specify a directory containing global package patches.
+	  For a specific version <version> of a specific package <pkg>, patches
+	  are applied as follows.
+
+	  First, the default Buildroot patch set for the package is applied.
+
+	  If the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg>/<version> exists, then
+	  all *.patch files in the directory will be applied.
+
+	  Otherwise, if the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg> exists, then
+	  all *.patch files in the directory will be applied.
+
 endmenu
 
 source "toolchain/Config.in"
diff --git a/docs/manual/customize-packages.txt b/docs/manual/customize-packages.txt
new file mode 100644
index 0000000..9e158c7
--- /dev/null
+++ b/docs/manual/customize-packages.txt
@@ -0,0 +1,23 @@
+// -*- mode:doc -*- ;
+
+[[packages-custom]]
+Customizing packages
+~~~~~~~~~~~~~~~~~~~~
+
+It is sometimes useful to apply 'extra' patches to packages - over and
+above those provided in Buildroot. This might be used to support custom
+features in a project, for example, or when working on a new architecture.
+
+The +BR2_GLOBAL_PATCH_DIR+ configuration file option can be
+used to specify a directory containing global package patches.
+
+For a specific version <version> of a specific package <pkg>, patches
+are applied as follows.
+
+First, the default Buildroot patch set for the package is applied.
+
+If the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg>/<version> exists, then
+all *.patch files in the directory will be applied.
+
+Otherwise, if the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg> exists, then
+all *.patch files in the directory will be applied.
diff --git a/docs/manual/customize.txt b/docs/manual/customize.txt
index 3b1a5a7..0456ef1 100644
--- a/docs/manual/customize.txt
+++ b/docs/manual/customize.txt
@@ -15,3 +15,5 @@ include::customize-kernel-config.txt[]
 include::customize-toolchain.txt[]
 
 include::customize-store.txt[]
+
+include::customize-packages.txt[]
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 57b0fd0..db0b025 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -82,21 +82,21 @@ endif
 # find the package directory (typically package/<pkgname>) and the
 # prefix of the patches
 $(BUILD_DIR)/%/.stamp_patched: NAMEVER = $(RAWNAME)-$($(PKG)_VERSION)
+$(BUILD_DIR)/%/.stamp_patched: PATCH_BASE_DIRS = $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(call qstrip,$(BR2_GLOBAL_PATCH_DIR))/$(RAWNAME)
 $(BUILD_DIR)/%/.stamp_patched:
 	@$(call MESSAGE,"Patching $($(PKG)_DIR_PREFIX)/$(RAWNAME)")
 	$(foreach hook,$($(PKG)_PRE_PATCH_HOOKS),$(call $(hook))$(sep))
 	$(foreach p,$($(PKG)_PATCH),support/scripts/apply-patches.sh $(@D) $(DL_DIR) $(p)$(sep))
 	$(Q)( \
-	if test -d $($(PKG)_DIR_PREFIX)/$(RAWNAME); then \
-	  if test "$(wildcard $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER)*.patch*)"; then \
-	    support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(NAMEVER)\*.patch $(NAMEVER)\*.patch.$(ARCH) || exit 1; \
-	  else \
-	    support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(RAWNAME)\*.patch $(RAWNAME)\*.patch.$(ARCH) || exit 1; \
-	    if test -d $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER); then \
-	      support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER) \*.patch \*.patch.$(ARCH) || exit 1; \
+	for D in $(PATCH_BASE_DIRS); do \
+	  if test -d $${D}; then \
+	    if test -d $${D}/$($(PKG)_VERSION); then \
+	      support/scripts/apply-patches.sh $(@D) $${D}/$($(PKG)_VERSION) \*.patch \*.patch.$(ARCH) || exit 1; \
+	    else \
+	      support/scripts/apply-patches.sh $(@D) $${D} \*.patch \*.patch.$(ARCH) || exit 1; \
 	    fi; \
 	  fi; \
-	fi; \
+	done; \
 	)
 	$(foreach hook,$($(PKG)_POST_PATCH_HOOKS),$(call $(hook))$(sep))
 	$(Q)touch $@
-- 
1.7.10.4

[Buildroot] [PATCH v2] rework patch model

[Baruch Siach]

Hi Simon,

On Sun, Mar 17, 2013 at 11:08:09AM +0000, spdawson at gmail.com wrote:
> From: Simon Dawson <spdawson@gmail.com>
> 
> At the Buildroot Developers Meeting (4-5 February 2013, in Brussels) a change
> to the patch logic was discussed. See 
> 
> http://elinux.org/Buildroot:DeveloperDaysFOSDEM2013
> 
> for details. In summary:
> 
> * For patches stored in the package directory, if package/<pkg>/<version>/ does exist, apply package/<pkg>/<version>/*.patch, otherwise, apply package/<pkg>/*.patch
> * For patches stored in the global patches directory, if $(GLOBAL_PATCH_DIR)/<pkg>/<version>/ does exist, apply $(GLOBAL_PATCH_DIR)/<pkg>/<version>/*.patch, otherwise, apply $(GLOBAL_PATCH_DIR)/<pkg>/*.patch
> 
> This patch adds the new BR2_GLOBAL_PATCH_DIR configuration item, and reworks
> the generic package infrastructure to implement the new patch logic.

Sorry for not mentioning it in my first review, but commit log message lines 
should be wrapped at 80 characters.

Thanks,
baruch

> 
> Signed-off-by: Simon Dawson <spdawson@gmail.com>
> ---
>  v2: Add documentation, as suggested by Baruch Siach

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[Buildroot] [PATCH v2] rework patch model

[Samuel Martin]

Hi Simon,

Nice to see this point moving forward :-)

2013/3/17  <spdawson@gmail.com>:
> From: Simon Dawson <spdawson@gmail.com>
>
> At the Buildroot Developers Meeting (4-5 February 2013, in Brussels) a change
> to the patch logic was discussed. See
>
> http://elinux.org/Buildroot:DeveloperDaysFOSDEM2013
>
> for details. In summary:
>
> * For patches stored in the package directory, if package/<pkg>/<version>/ does exist, apply package/<pkg>/<version>/*.patch, otherwise, apply package/<pkg>/*.patch
> * For patches stored in the global patches directory, if $(GLOBAL_PATCH_DIR)/<pkg>/<version>/ does exist, apply $(GLOBAL_PATCH_DIR)/<pkg>/<version>/*.patch, otherwise, apply $(GLOBAL_PATCH_DIR)/<pkg>/*.patch
>
> This patch adds the new BR2_GLOBAL_PATCH_DIR configuration item, and reworks
> the generic package infrastructure to implement the new patch logic.
>
> Signed-off-by: Simon Dawson <spdawson@gmail.com>
> ---
>  v2: Add documentation, as suggested by Baruch Siach
>
>  Config.in                          |   15 +++++++++++++++
>  docs/manual/customize-packages.txt |   23 +++++++++++++++++++++++
>  docs/manual/customize.txt          |    2 ++
>  package/pkg-generic.mk             |   16 ++++++++--------
>  4 files changed, 48 insertions(+), 8 deletions(-)
>  create mode 100644 docs/manual/customize-packages.txt
>
[...]
> diff --git a/docs/manual/customize-packages.txt b/docs/manual/customize-packages.txt
> new file mode 100644
> index 0000000..9e158c7
> --- /dev/null
> +++ b/docs/manual/customize-packages.txt
> @@ -0,0 +1,23 @@
> +// -*- mode:doc -*- ;
> +
> +[[packages-custom]]
> +Customizing packages
> +~~~~~~~~~~~~~~~~~~~~
> +
> +It is sometimes useful to apply 'extra' patches to packages - over and
> +above those provided in Buildroot. This might be used to support custom
> +features in a project, for example, or when working on a new architecture.
> +
> +The +BR2_GLOBAL_PATCH_DIR+ configuration file option can be
> +used to specify a directory containing global package patches.
> +
> +For a specific version <version> of a specific package <pkg>, patches
> +are applied as follows.
> +
> +First, the default Buildroot patch set for the package is applied.
> +
> +If the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg>/<version> exists, then
> +all *.patch files in the directory will be applied.
> +
> +Otherwise, if the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg> exists, then
> +all *.patch files in the directory will be applied.
> diff --git a/docs/manual/customize.txt b/docs/manual/customize.txt
> index 3b1a5a7..0456ef1 100644
> --- a/docs/manual/customize.txt
> +++ b/docs/manual/customize.txt
> @@ -15,3 +15,5 @@ include::customize-kernel-config.txt[]
>  include::customize-toolchain.txt[]
>
>  include::customize-store.txt[]
> +
> +include::customize-packages.txt[]

Would also be good to update and maybe add cross-refs in the "Patching
a package" section in docs/manual/patch-policy.txt

Regards,

-- 
Samuel

[Buildroot] [PATCH v2] rework patch model

[Arnout Vandecappelle]

On 17/03/13 12:08, spdawson at gmail.com wrote:
> From: Simon Dawson <spdawson@gmail.com>
>
> At the Buildroot Developers Meeting (4-5 February 2013, in Brussels) a change
> to the patch logic was discussed. See
>
> http://elinux.org/Buildroot:DeveloperDaysFOSDEM2013
>
> for details. In summary:
>
> * For patches stored in the package directory, if package/<pkg>/<version>/ does exist, apply package/<pkg>/<version>/*.patch, otherwise, apply package/<pkg>/*.patch

  I wonder if perhaps I made a mistake while writing the report... Did we 
really agree to apply package/<pkg>/*.patch instead of
package/<pkg>/<pkg>-*.patch?

  Now that I think about it: I wouldn't mind, actually, the <pkg>- prefix 
is pretty redundant and it sits in the way of creating patches with 
git-format-patch. Should we then also change our patch naming policy?

> * For patches stored in the global patches directory, if $(GLOBAL_PATCH_DIR)/<pkg>/<version>/ does exist, apply $(GLOBAL_PATCH_DIR)/<pkg>/<version>/*.patch, otherwise, apply $(GLOBAL_PATCH_DIR)/<pkg>/*.patch
>
> This patch adds the new BR2_GLOBAL_PATCH_DIR configuration item, and reworks
> the generic package infrastructure to implement the new patch logic.

  For clarity, I would have split up this patch into two patches:

1. Remove the $(NAMEVER).patch way of specifying versioned patches.
2. Add the GLOBAL_PATCH_DIR option.

  But it's no biggy, since the first patch is rather trivial. Still, I 
would mention this removal of the $(NAMEVER).patch explicitly in the 
commit message.

  Note that this patch will break any remaining packages that still have 
both $(NAMEVER).patch and $(RAWNAME).patch patches. So watch your 
autobuilders! There are still more than 200 $(NAMEVER).patch files:
find package -name *-[0-9].[0-9]*.patch


[snip]
> diff --git a/docs/manual/customize-packages.txt b/docs/manual/customize-packages.txt
> new file mode 100644
> index 0000000..9e158c7
> --- /dev/null
> +++ b/docs/manual/customize-packages.txt
> @@ -0,0 +1,23 @@
> +// -*- mode:doc -*- ;
> +
> +[[packages-custom]]
> +Customizing packages
> +~~~~~~~~~~~~~~~~~~~~
> +
> +It is sometimes useful to apply 'extra' patches to packages - over and
> +above those provided in Buildroot. This might be used to support custom
> +features in a project, for example, or when working on a new architecture.
> +
> +The +BR2_GLOBAL_PATCH_DIR+ configuration file option can be
> +used to specify a directory containing global package patches.
> +
> +For a specific version <version> of a specific package <pkg>, patches
> +are applied as follows.
> +
> +First, the default Buildroot patch set for the package is applied.
> +
> +If the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg>/<version> exists, then
> +all *.patch files in the directory will be applied.
> +
> +Otherwise, if the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg> exists, then
> +all *.patch files in the directory will be applied.

  Could be nice to add:

The patches are applied in alphabetical order (the order of 'ls' in the C 
locale). Therefore, it is advisable to name patches 
<num>-<description>.patch, where <num> is a 4-digit numerical value that 
enforces the order.

> diff --git a/docs/manual/customize.txt b/docs/manual/customize.txt
> index 3b1a5a7..0456ef1 100644
> --- a/docs/manual/customize.txt
> +++ b/docs/manual/customize.txt
> @@ -15,3 +15,5 @@ include::customize-kernel-config.txt[]
>   include::customize-toolchain.txt[]
>
>   include::customize-store.txt[]
> +
> +include::customize-packages.txt[]
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 57b0fd0..db0b025 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -82,21 +82,21 @@ endif
>   # find the package directory (typically package/<pkgname>) and the
>   # prefix of the patches
>   $(BUILD_DIR)/%/.stamp_patched: NAMEVER = $(RAWNAME)-$($(PKG)_VERSION)
> +$(BUILD_DIR)/%/.stamp_patched: PATCH_BASE_DIRS = $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(call qstrip,$(BR2_GLOBAL_PATCH_DIR))/$(RAWNAME)

  Since GLOBAL_PATCH_DIR defaults to the empty string, this will by 
default look in /$(RAWNAME). Fortunately we don't have a package called 
etc or tmp or something similar, but you can imagine the weird fallout 
waiting to happen...  So, this should be something like:

GLOBAL_PATCH_DIR = $(call qstrip,$(BR2_GLOBAL_PATCH_DIR))

PATCH_BASE_DIRS = $($(PKG)_DIR_PREFIX)/$(RAWNAME) \
    $(if $(GLOBAL_PATCH_DIR),$(GLOBAL_PATCH_DIR)/$(RAWNAME))

>   $(BUILD_DIR)/%/.stamp_patched:
>   	@$(call MESSAGE,"Patching $($(PKG)_DIR_PREFIX)/$(RAWNAME)")
>   	$(foreach hook,$($(PKG)_PRE_PATCH_HOOKS),$(call $(hook))$(sep))
>   	$(foreach p,$($(PKG)_PATCH),support/scripts/apply-patches.sh $(@D) $(DL_DIR) $(p)$(sep))
>   	$(Q)( \
> -	if test -d $($(PKG)_DIR_PREFIX)/$(RAWNAME); then \
> -	  if test "$(wildcard $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER)*.patch*)"; then \
> -	    support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(NAMEVER)\*.patch $(NAMEVER)\*.patch.$(ARCH) || exit 1; \
> -	  else \
> -	    support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(RAWNAME)\*.patch $(RAWNAME)\*.patch.$(ARCH) || exit 1; \
> -	    if test -d $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER); then \
> -	      support/scripts/apply-patches.sh $(@D) $($(PKG)_DIR_PREFIX)/$(RAWNAME)/$(NAMEVER) \*.patch \*.patch.$(ARCH) || exit 1; \
> +	for D in $(PATCH_BASE_DIRS); do \

  Since this is the only place where PATCH_BASE_DIRS is used, I don't see 
much point of creating a variable for it.

  Also, if you remove the /$(RAWNAME) from the base dirs list and add it 
to the uses of $${D} below, then you remove the need for the 
GLOBAL_PATCH_DIRS condition.


  Regards,
  Arnout

> +	  if test -d $${D}; then \
> +	    if test -d $${D}/$($(PKG)_VERSION); then \
> +	      support/scripts/apply-patches.sh $(@D) $${D}/$($(PKG)_VERSION) \*.patch \*.patch.$(ARCH) || exit 1; \
> +	    else \
> +	      support/scripts/apply-patches.sh $(@D) $${D} \*.patch \*.patch.$(ARCH) || exit 1; \
>   	    fi; \
>   	  fi; \
> -	fi; \
> +	done; \
>   	)
>   	$(foreach hook,$($(PKG)_POST_PATCH_HOOKS),$(call $(hook))$(sep))
>   	$(Q)touch $@
>


-- 
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 v2] rework patch model

[Simon Dawson]

Hi Arnout. Thanks for all the suggestions.

On 21 March 2013 07:25, Arnout Vandecappelle <arnout@mind.be> wrote:
>  Note that this patch will break any remaining packages that still have both
> $(NAMEVER).patch and $(RAWNAME).patch patches. So watch your autobuilders!
> There are still more than 200 $(NAMEVER).patch files:
> find package -name *-[0-9].[0-9]*.patch

Oh dear...

>> +The +BR2_GLOBAL_PATCH_DIR+ configuration file option can be
>> +used to specify a directory containing global package patches.
>> +
>> +For a specific version <version> of a specific package <pkg>, patches
>> +are applied as follows.
>> +
>> +First, the default Buildroot patch set for the package is applied.
>> +
>> +If the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg>/<version> exists, then
>> +all *.patch files in the directory will be applied.
>> +
>> +Otherwise, if the directory $(BR2_GLOBAL_PATCH_DIR)/<pkg> exists, then
>> +all *.patch files in the directory will be applied.
>
>
>  Could be nice to add:
>
> The patches are applied in alphabetical order (the order of 'ls' in the C
> locale). Therefore, it is advisable to name patches
> <num>-<description>.patch, where <num> is a 4-digit numerical value that
> enforces the order.

Sounds like a good idea.

>> +$(BUILD_DIR)/%/.stamp_patched: PATCH_BASE_DIRS =
>> $($(PKG)_DIR_PREFIX)/$(RAWNAME) $(call
>> qstrip,$(BR2_GLOBAL_PATCH_DIR))/$(RAWNAME)
>
>
>  Since GLOBAL_PATCH_DIR defaults to the empty string, this will by default
> look in /$(RAWNAME). Fortunately we don't have a package called etc or tmp
> or something similar, but you can imagine the weird fallout waiting to
> happen...  So, this should be something like:
>
> GLOBAL_PATCH_DIR = $(call qstrip,$(BR2_GLOBAL_PATCH_DIR))
>
> PATCH_BASE_DIRS = $($(PKG)_DIR_PREFIX)/$(RAWNAME) \
>    $(if $(GLOBAL_PATCH_DIR),$(GLOBAL_PATCH_DIR)/$(RAWNAME))

Okay; will fix this.

>> +       for D in $(PATCH_BASE_DIRS); do \
>
>
>  Since this is the only place where PATCH_BASE_DIRS is used, I don't see
> much point of creating a variable for it.
>
>  Also, if you remove the /$(RAWNAME) from the base dirs list and add it to
> the uses of $${D} below, then you remove the need for the GLOBAL_PATCH_DIRS
> condition.

Good idea.

I'll work up a patch to fix these issues.

Simon.