| Message ID | 1996959fcee1f7bf71e9e8539fa2471fce6b1308.1445545973.git.yann.morin.1998@free.fr |
|---|---|
| State | Accepted |
| Headers | show |
On 22-10-15 22:33, Yann E. MORIN wrote: > One of the selling points for br2-external is to provide a mean to add > new packages. However, it is not supported that a package be defined by > Buildroot and then redefined in a br2-external tree. > > This situation may occur without the user noticing or even willing to > redefine the package, for example: > - br2-external is first created against a version of Buildroot > - a package (missing in Buildroot) is added to that br2-external tree > - upstream Buildroot adds this package > - user updates to the new Buildroot > > In this case, the result in undefined, and we can't make any guarantee > on the result (working or not). > > Add a sanity check so that a package redefinition gets caught. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Arnout Vandecappelle <arnout@mind.be> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> For you multi-br2-external haters: please commit independently of be multi- feature :-) But see below. > --- > Makefile | 1 + > package/pkg-generic.mk | 9 +++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/Makefile b/Makefile > index dd8959f..da78f18 100644 > --- a/Makefile > +++ b/Makefile > @@ -336,6 +336,7 @@ unexport O > GNU_HOST_NAME := $(shell support/gnuconfig/config.guess) > > PACKAGES := > +PACKAGES_ALL := > > # silent mode requested? > QUIET := $(if $(findstring s,$(filter-out --%,$(MAKEFLAGS))),-q) > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index ffef4d3..7f0c2ab 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -341,6 +341,14 @@ endef > > define inner-generic-package > > +# Ensure the package is only declared once, i.e. do not accept that a > +# package be re-defined by a br2-external tree > +ifneq ($(call strip,$(filter $(1),$(PACKAGES_ALL))),) > +$$(error Package '$(1)' defined a second time in '$(pkgdir)'; \ > + previous definition was in '$$($(2)_PKGDIR)') > +endif > +PACKAGES_ALL += $(1) > + > # Define default values for various package-related variables, if not > # already defined. For some variables (version, source, site and > # subdir), if they are undefined, we try to see if a variable without > @@ -351,6 +359,7 @@ define inner-generic-package > $(2)_TYPE = $(4) > $(2)_NAME = $(1) > $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) > +$(2)_PKGDIR = $(pkgdir) Now we have this variable, it would be nice to replace all occurences of $(PKGDIR) with $($(PKG)_PKGDIR) and remove the (IMHO ugly) $$($(2)_TARGET_PATCH): PKGDIR=$(pkgdir) (obviously in a follow-up patch, which I might produce myself if I feel like it). Regards, Arnout > > # Keep the package version that may contain forward slashes in the _DL_VERSION > # variable, then replace all forward slashes ('/') by underscores ('_') to >
Arnout, All, On 2015-10-22 23:01 +0200, Arnout Vandecappelle spake thusly: > On 22-10-15 22:33, Yann E. MORIN wrote: > > One of the selling points for br2-external is to provide a mean to add > > new packages. However, it is not supported that a package be defined by > > Buildroot and then redefined in a br2-external tree. > > > > This situation may occur without the user noticing or even willing to > > redefine the package, for example: > > - br2-external is first created against a version of Buildroot > > - a package (missing in Buildroot) is added to that br2-external tree > > - upstream Buildroot adds this package > > - user updates to the new Buildroot > > > > In this case, the result in undefined, and we can't make any guarantee > > on the result (working or not). > > > > Add a sanity check so that a package redefinition gets caught. > > > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Cc: Peter Korsgaard <jacmet@uclibc.org> > > Cc: Arnout Vandecappelle <arnout@mind.be> > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Thanks! :-) > For you multi-br2-external haters: please commit independently of be multi- > feature :-) Yes, this is independent. It's in this series because it was prompted by my work on multi-br2-external, but I could very well have submitted it separately. [--SNIP--] > > @@ -351,6 +359,7 @@ define inner-generic-package > > $(2)_TYPE = $(4) > > $(2)_NAME = $(1) > > $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) > > +$(2)_PKGDIR = $(pkgdir) > > Now we have this variable, it would be nice to replace all occurences of > $(PKGDIR) with $($(PKG)_PKGDIR) and remove the (IMHO ugly) > > $$($(2)_TARGET_PATCH): PKGDIR=$(pkgdir) > > (obviously in a follow-up patch, which I might produce myself if I feel like it). I already started doing so, jsut to see to what extent that would have an impact, but it is not ready at all. I also wanted to replace every occurences of hard-coded paths like: package/foo/my-file with: $(FOO_PKGDIR)/my-file but this is an insane amount of work. I'll cary it if/when this patch is applied. Regards, Yann E. MORIN.
On 22-10-15 23:09, Yann E. MORIN wrote: > Arnout, All, > > On 2015-10-22 23:01 +0200, Arnout Vandecappelle spake thusly: >> On 22-10-15 22:33, Yann E. MORIN wrote: [snip] >>> @@ -351,6 +359,7 @@ define inner-generic-package >>> $(2)_TYPE = $(4) >>> $(2)_NAME = $(1) >>> $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) >>> +$(2)_PKGDIR = $(pkgdir) >> >> Now we have this variable, it would be nice to replace all occurences of >> $(PKGDIR) with $($(PKG)_PKGDIR) and remove the (IMHO ugly) BTW, an alternative is to to globally assign: PKGDIR = $($(PKG)_PKGDIR) then we can keep on using $(PKGDIR). But that probably is not really clear for most developers... >> >> $$($(2)_TARGET_PATCH): PKGDIR=$(pkgdir) >> >> (obviously in a follow-up patch, which I might produce myself if I feel like it). > > I already started doing so, jsut to see to what extent that would have > an impact, but it is not ready at all. > > I also wanted to replace every occurences of hard-coded paths like: > package/foo/my-file > > with: > $(FOO_PKGDIR)/my-file > > but this is an insane amount of work. I'll cary it if/when this patch is > applied. I don't think it makes sense to make a mass change like that. If you want to make a mass change, calculate some hashes :-) Regards, Arnout > > Regards, > Yann E. MORIN. >
Yann, On Thu, Oct 22, 2015 at 10:33 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > One of the selling points for br2-external is to provide a mean to add > new packages. However, it is not supported that a package be defined by > Buildroot and then redefined in a br2-external tree. > > This situation may occur without the user noticing or even willing to > redefine the package, for example: > - br2-external is first created against a version of Buildroot > - a package (missing in Buildroot) is added to that br2-external tree > - upstream Buildroot adds this package > - user updates to the new Buildroot > > In this case, the result in undefined, and we can't make any guarantee > on the result (working or not). > > Add a sanity check so that a package redefinition gets caught. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Arnout Vandecappelle <arnout@mind.be> Reviewed-by: Samuel Martin <s.martin49@gmail.com> > --- > Makefile | 1 + > package/pkg-generic.mk | 9 +++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/Makefile b/Makefile > index dd8959f..da78f18 100644 > --- a/Makefile > +++ b/Makefile > @@ -336,6 +336,7 @@ unexport O > GNU_HOST_NAME := $(shell support/gnuconfig/config.guess) > > PACKAGES := > +PACKAGES_ALL := > > # silent mode requested? > QUIET := $(if $(findstring s,$(filter-out --%,$(MAKEFLAGS))),-q) > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index ffef4d3..7f0c2ab 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -341,6 +341,14 @@ endef > > define inner-generic-package > > +# Ensure the package is only declared once, i.e. do not accept that a > +# package be re-defined by a br2-external tree > +ifneq ($(call strip,$(filter $(1),$(PACKAGES_ALL))),) > +$$(error Package '$(1)' defined a second time in '$(pkgdir)'; \ > + previous definition was in '$$($(2)_PKGDIR)') > +endif > +PACKAGES_ALL += $(1) > + > # Define default values for various package-related variables, if not > # already defined. For some variables (version, source, site and > # subdir), if they are undefined, we try to see if a variable without > @@ -351,6 +359,7 @@ define inner-generic-package > $(2)_TYPE = $(4) > $(2)_NAME = $(1) > $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) > +$(2)_PKGDIR = $(pkgdir) This change can also provide clean/unique way of handling packages' files coming within Buildroot and br2-external trees. :-) (I'm not a big fan of the package/foo/foo.conf vs. $(BR2_EXTERNAL)/package/bar/bar.conf thing, depending whether foo is in the Builldroot tree, and bar in the br2-external one). Regards,
Samuel, All, On 2015-10-23 21:39 +0200, Samuel Martin spake thusly: > On Thu, Oct 22, 2015 at 10:33 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > One of the selling points for br2-external is to provide a mean to add > > new packages. However, it is not supported that a package be defined by > > Buildroot and then redefined in a br2-external tree. > > > > This situation may occur without the user noticing or even willing to > > redefine the package, for example: > > - br2-external is first created against a version of Buildroot > > - a package (missing in Buildroot) is added to that br2-external tree > > - upstream Buildroot adds this package > > - user updates to the new Buildroot > > > > In this case, the result in undefined, and we can't make any guarantee > > on the result (working or not). > > > > Add a sanity check so that a package redefinition gets caught. > > > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Cc: Peter Korsgaard <jacmet@uclibc.org> > > Cc: Arnout Vandecappelle <arnout@mind.be> > > Reviewed-by: Samuel Martin <s.martin49@gmail.com> > [--SNIP--] > > @@ -351,6 +359,7 @@ define inner-generic-package > > $(2)_TYPE = $(4) > > $(2)_NAME = $(1) > > $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) > > +$(2)_PKGDIR = $(pkgdir) > > This change can also provide clean/unique way of handling packages' > files coming within Buildroot and br2-external trees. :-) > (I'm not a big fan of the package/foo/foo.conf vs. > $(BR2_EXTERNAL)/package/bar/bar.conf thing, depending whether foo is > in the Builldroot tree, and bar in the br2-external one). See what I already replied to Arnout! ;-) I don;t like it either, and I'd like we switch to using that variable instead. Still, that's pretty much of a change, and Arnout is right: such a bulk change is not needed right now; we can switch over time, as packages are updated/fixed... Regards, Yann E. MORIN.
Dear Yann E. MORIN, On Thu, 22 Oct 2015 22:33:56 +0200, Yann E. MORIN wrote: > One of the selling points for br2-external is to provide a mean to add > new packages. However, it is not supported that a package be defined by > Buildroot and then redefined in a br2-external tree. > > This situation may occur without the user noticing or even willing to > redefine the package, for example: > - br2-external is first created against a version of Buildroot > - a package (missing in Buildroot) is added to that br2-external tree > - upstream Buildroot adds this package > - user updates to the new Buildroot > > In this case, the result in undefined, and we can't make any guarantee > on the result (working or not). > > Add a sanity check so that a package redefinition gets caught. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Arnout Vandecappelle <arnout@mind.be> > --- > Makefile | 1 + > package/pkg-generic.mk | 9 +++++++++ > 2 files changed, 10 insertions(+) Applied, thanks. Thomas
diff --git a/Makefile b/Makefile index dd8959f..da78f18 100644 --- a/Makefile +++ b/Makefile @@ -336,6 +336,7 @@ unexport O GNU_HOST_NAME := $(shell support/gnuconfig/config.guess) PACKAGES := +PACKAGES_ALL := # silent mode requested? QUIET := $(if $(findstring s,$(filter-out --%,$(MAKEFLAGS))),-q) diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index ffef4d3..7f0c2ab 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -341,6 +341,14 @@ endef define inner-generic-package +# Ensure the package is only declared once, i.e. do not accept that a +# package be re-defined by a br2-external tree +ifneq ($(call strip,$(filter $(1),$(PACKAGES_ALL))),) +$$(error Package '$(1)' defined a second time in '$(pkgdir)'; \ + previous definition was in '$$($(2)_PKGDIR)') +endif +PACKAGES_ALL += $(1) + # Define default values for various package-related variables, if not # already defined. For some variables (version, source, site and # subdir), if they are undefined, we try to see if a variable without @@ -351,6 +359,7 @@ define inner-generic-package $(2)_TYPE = $(4) $(2)_NAME = $(1) $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) +$(2)_PKGDIR = $(pkgdir) # Keep the package version that may contain forward slashes in the _DL_VERSION # variable, then replace all forward slashes ('/') by underscores ('_') to
One of the selling points for br2-external is to provide a mean to add new packages. However, it is not supported that a package be defined by Buildroot and then redefined in a br2-external tree. This situation may occur without the user noticing or even willing to redefine the package, for example: - br2-external is first created against a version of Buildroot - a package (missing in Buildroot) is added to that br2-external tree - upstream Buildroot adds this package - user updates to the new Buildroot In this case, the result in undefined, and we can't make any guarantee on the result (working or not). Add a sanity check so that a package redefinition gets caught. Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Peter Korsgaard <jacmet@uclibc.org> Cc: Arnout Vandecappelle <arnout@mind.be> --- Makefile | 1 + package/pkg-generic.mk | 9 +++++++++ 2 files changed, 10 insertions(+)