Patchwork [v2] rework patch model

login
register
mail settings
Submitter Simon Dawson
Date March 17, 2013, 11:08 a.m.
Message ID <1363518489-1065-1-git-send-email-spdawson@gmail.com>
Download mbox | patch
Permalink /patch/228291/
State Superseded
Headers show

Comments

Simon Dawson - March 17, 2013, 11:08 a.m.
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
Baruch Siach - March 17, 2013, 12:02 p.m.
Hi Simon,

On Sun, Mar 17, 2013 at 11:08:09AM +0000, spdawson@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
Samuel Martin - March 17, 2013, 10:09 p.m.
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,
Arnout Vandecappelle - March 21, 2013, 7:25 a.m.
On 17/03/13 12:08, spdawson@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 $@
>
Simon Dawson - March 21, 2013, 9:27 a.m.
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.

Patch

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