| Message ID | 1363518489-1065-1-git-send-email-spdawson@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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,
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 $@ >
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.
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 $@