| Message ID | 1382088860-30524-2-git-send-email-fabio.porcedda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 18/10/13 11:34, Fabio Porcedda wrote: > Move "dependencies" "dirs" "prepare" dependencies from "toolchain" to > every package. > This way we can build correctly every package right after the clean > stage. > As example with this commit we can build successfully the glibc right > after the clean stage: > make clean glibc > > This is also a step forward supporting top-level parallel make. > > Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> Although this is one of the most useful patches of the series, it is also the one that introduces the circular dependency. So although it looks good, I'm not ready to ack it. > --- > package/pkg-generic.mk | 2 ++ > toolchain/toolchain/toolchain.mk | 3 +-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 4bba4b5..1e7154e 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -385,6 +385,8 @@ $(1)-install-host: $(1)-build $$($(2)_TARGET_INSTALL_HOST) > $(1)-build: $(1)-configure \ > $$($(2)_TARGET_BUILD) > > +$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies dirs prepare Is there any reason why you changed the order here? Regards, Arnout > + > ifeq ($$($(2)_OVERRIDE_SRCDIR),) > # In the normal case (no package override), the sequence of steps is > # source, by downloading > diff --git a/toolchain/toolchain/toolchain.mk b/toolchain/toolchain/toolchain.mk > index 44ed629..8559ac9 100644 > --- a/toolchain/toolchain/toolchain.mk > +++ b/toolchain/toolchain/toolchain.mk > @@ -14,5 +14,4 @@ endif > > $(eval $(generic-package)) > > -toolchain-source: prepare dirs dependencies $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake > - > +toolchain: $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake >
Hi Arnout, thanks for reviewing the patch set. On Wed, Oct 23, 2013 at 11:12 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 18/10/13 11:34, Fabio Porcedda wrote: >> >> Move "dependencies" "dirs" "prepare" dependencies from "toolchain" to >> every package. >> This way we can build correctly every package right after the clean >> stage. >> As example with this commit we can build successfully the glibc right >> after the clean stage: >> make clean glibc >> >> This is also a step forward supporting top-level parallel make. >> >> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> > > > Although this is one of the most useful patches of the series, it is also > the one that introduces the circular dependency. So although it looks good, > I'm not ready to ack it. I've found a work around: $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare # to prevent circular dependency ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies endif What do you think about that? > >> --- >> package/pkg-generic.mk | 2 ++ >> toolchain/toolchain/toolchain.mk | 3 +-- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >> index 4bba4b5..1e7154e 100644 >> --- a/package/pkg-generic.mk >> +++ b/package/pkg-generic.mk >> @@ -385,6 +385,8 @@ $(1)-install-host: $(1)-build >> $$($(2)_TARGET_INSTALL_HOST) >> $(1)-build: $(1)-configure \ >> $$($(2)_TARGET_BUILD) >> >> +$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies dirs prepare > > > Is there any reason why you changed the order here? The order whit top-level parallel make does not matter, it's just that i was thinking that it's the right logical order: 1: dependencies: requisite 2: dirs: create empty directories 3. prepare: add something to the empty directories But now i understand that is "dirs prepare dependencies" or "dirs dependencies prepare" because dirs can require host-ccache and so directories, nevertheless this does not matter anymore. > > Regards, > Arnout > > >> + >> ifeq ($$($(2)_OVERRIDE_SRCDIR),) >> # In the normal case (no package override), the sequence of steps is >> # source, by downloading >> diff --git a/toolchain/toolchain/toolchain.mk >> b/toolchain/toolchain/toolchain.mk >> index 44ed629..8559ac9 100644 >> --- a/toolchain/toolchain/toolchain.mk >> +++ b/toolchain/toolchain/toolchain.mk >> @@ -14,5 +14,4 @@ endif >> >> $(eval $(generic-package)) >> >> -toolchain-source: prepare dirs dependencies >> $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake >> - >> +toolchain: $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake >> > > > -- > 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 Best regards
Hi, On Thu, Oct 24, 2013 at 9:41 AM, Fabio Porcedda <fabio.porcedda@gmail.com> wrote: >> >> >> Although this is one of the most useful patches of the series, it is also >> the one that introduces the circular dependency. So although it looks good, >> I'm not ready to ack it. > > I've found a work around: > > $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare > > # to prevent circular dependency > ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) > $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies > endif > > What do you think about that? > I don't think this will work if there are more than one prerequisite: $(1) will be one of them, and DEPENDENCIES_HOST_PREREQ will be the entire list, so that 'ifneq' will always be true. I think you need 'filter' here. http://www.gnu.org/software/make/manual/make.html#index-filter-587 Best regards, Thomas
On 24/10/13 09:41, Fabio Porcedda wrote: > Hi Arnout, > thanks for reviewing the patch set. > > On Wed, Oct 23, 2013 at 11:12 PM, Arnout Vandecappelle <arnout@mind.be> wrote: >> On 18/10/13 11:34, Fabio Porcedda wrote: >>> >>> Move "dependencies" "dirs" "prepare" dependencies from "toolchain" to >>> every package. >>> This way we can build correctly every package right after the clean >>> stage. >>> As example with this commit we can build successfully the glibc right >>> after the clean stage: >>> make clean glibc >>> >>> This is also a step forward supporting top-level parallel make. >>> >>> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> >> >> >> Although this is one of the most useful patches of the series, it is also >> the one that introduces the circular dependency. So although it looks good, >> I'm not ready to ack it. > > I've found a work around: > > $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare > > # to prevent circular dependency > ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) > $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies > endif > > What do you think about that? Looks good except that it's incorrect :-) If HOST_PREREQ contains both ccache and sstrip (not to mention tar or xz), the condition won't match. You can try something like: ifeq ($(filter $(1),$(DEPENDENCIES_HOST_PREREQ)),) [Just noticed now that Thomas gave the same answer.] > >> >>> --- >>> package/pkg-generic.mk | 2 ++ >>> toolchain/toolchain/toolchain.mk | 3 +-- >>> 2 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >>> index 4bba4b5..1e7154e 100644 >>> --- a/package/pkg-generic.mk >>> +++ b/package/pkg-generic.mk >>> @@ -385,6 +385,8 @@ $(1)-install-host: $(1)-build >>> $$($(2)_TARGET_INSTALL_HOST) >>> $(1)-build: $(1)-configure \ >>> $$($(2)_TARGET_BUILD) >>> >>> +$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies dirs prepare >> >> >> Is there any reason why you changed the order here? > > The order whit top-level parallel make does not matter, it's just that > i was thinking that it's the right logical order: > 1: dependencies: requisite > 2: dirs: create empty directories > 3. prepare: add something to the empty directories > > But now i understand that is "dirs prepare dependencies" or "dirs > dependencies prepare" because dirs can require host-ccache and so You mean dependencies can require host-ccache, right? And host-ccache depends on both dirs and prepare itself... So I do think the original order is more logical. Regards, Arnout > directories, > nevertheless this does not matter anymore. [snip]
On Thu, Oct 24, 2013 at 12:37 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 24/10/13 09:41, Fabio Porcedda wrote: >> >> Hi Arnout, >> thanks for reviewing the patch set. >> >> On Wed, Oct 23, 2013 at 11:12 PM, Arnout Vandecappelle <arnout@mind.be> >> wrote: >>> >>> On 18/10/13 11:34, Fabio Porcedda wrote: >>>> >>>> >>>> Move "dependencies" "dirs" "prepare" dependencies from "toolchain" to >>>> every package. >>>> This way we can build correctly every package right after the clean >>>> stage. >>>> As example with this commit we can build successfully the glibc right >>>> after the clean stage: >>>> make clean glibc >>>> >>>> This is also a step forward supporting top-level parallel make. >>>> >>>> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> >>> >>> >>> >>> Although this is one of the most useful patches of the series, it is >>> also >>> the one that introduces the circular dependency. So although it looks >>> good, >>> I'm not ready to ack it. >> >> >> I've found a work around: >> >> $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare >> >> # to prevent circular dependency >> ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) >> $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies >> endif >> >> What do you think about that? > > > Looks good except that it's incorrect :-) If HOST_PREREQ contains both > ccache and sstrip (not to mention tar or xz), the condition won't match. You > can try something like: > > ifeq ($(filter $(1),$(DEPENDENCIES_HOST_PREREQ)),) Ok good, I've updated the patch according. > > [Just noticed now that Thomas gave the same answer.] > > >> >>> >>>> --- >>>> package/pkg-generic.mk | 2 ++ >>>> toolchain/toolchain/toolchain.mk | 3 +-- >>>> 2 files changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >>>> index 4bba4b5..1e7154e 100644 >>>> --- a/package/pkg-generic.mk >>>> +++ b/package/pkg-generic.mk >>>> @@ -385,6 +385,8 @@ $(1)-install-host: $(1)-build >>>> $$($(2)_TARGET_INSTALL_HOST) >>>> $(1)-build: $(1)-configure \ >>>> $$($(2)_TARGET_BUILD) >>>> >>>> +$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies dirs >>>> prepare >>> >>> >>> >>> Is there any reason why you changed the order here? >> >> >> The order whit top-level parallel make does not matter, it's just that >> i was thinking that it's the right logical order: >> 1: dependencies: requisite >> 2: dirs: create empty directories >> 3. prepare: add something to the empty directories >> >> But now i understand that is "dirs prepare dependencies" or "dirs >> dependencies prepare" because dirs can require host-ccache and so > > > You mean dependencies can require host-ccache, right? And host-ccache > depends on both dirs and prepare itself... So I do think the original order > is more logical. Yes the final order is this: dirs prepare dependencies. I moved dirs in front because i think creating directories it's the first thing to do. > Regards, > Arnout > > >> directories, >> nevertheless this does not matter anymore. > > > [snip] > > > > -- > 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 Thanks and regards
On Thu, Oct 24, 2013 at 10:22 AM, Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > Hi, > > On Thu, Oct 24, 2013 at 9:41 AM, Fabio Porcedda > <fabio.porcedda@gmail.com> wrote: >>> >>> >>> Although this is one of the most useful patches of the series, it is also >>> the one that introduces the circular dependency. So although it looks good, >>> I'm not ready to ack it. >> >> I've found a work around: >> >> $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare >> >> # to prevent circular dependency >> ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) >> $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies >> endif >> >> What do you think about that? >> > > I don't think this will work if there are more than one prerequisite: > $(1) will be one of them, and DEPENDENCIES_HOST_PREREQ will be the > entire list, so that 'ifneq' will always be true. > I think you need 'filter' here. > http://www.gnu.org/software/make/manual/make.html#index-filter-587 Yes it works fine with filter, thanks for suggesting it. > Best regards, > Thomas Thanks and regards
On 25/10/13 10:07, Fabio Porcedda wrote: > On Thu, Oct 24, 2013 at 12:37 PM, Arnout Vandecappelle<arnout@mind.be> wrote: >> >On 24/10/13 09:41, Fabio Porcedda wrote: >>> >> [snip] >>> >>I've found a work around: >>> >> >>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare >>> >> >>> >># to prevent circular dependency >>> >>ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) >>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies >>> >>endif >>> >> >>> >>What do you think about that? >> > >> > >> > Looks good except that it's incorrect:-) If HOST_PREREQ contains both >> >ccache and sstrip (not to mention tar or xz), the condition won't match. You >> >can try something like: >> > >> >ifeq ($(filter $(1),$(DEPENDENCIES_HOST_PREREQ)),) > Ok good, I've updated the patch according. > Have you tested it with various combinations of dependencies? Regards, Arnout
On Fri, Oct 25, 2013 at 10:12 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > On 25/10/13 10:07, Fabio Porcedda wrote: >> >> On Thu, Oct 24, 2013 at 12:37 PM, Arnout Vandecappelle<arnout@mind.be> >> wrote: >>> >>> >On 24/10/13 09:41, Fabio Porcedda wrote: >>>> >>>> >> > > [snip] > >>>> >>I've found a work around: >>>> >> >>>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare >>>> >> >>>> >># to prevent circular dependency >>>> >>ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) >>>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies >>>> >>endif >>>> >> >>>> >>What do you think about that? >>> >>> > >>> > >>> > Looks good except that it's incorrect:-) If HOST_PREREQ contains both >>> >ccache and sstrip (not to mention tar or xz), the condition won't match. >>> > You >>> >can try something like: >>> > >>> >ifeq ($(filter $(1),$(DEPENDENCIES_HOST_PREREQ)),) >> >> Ok good, I've updated the patch according. >> > > Have you tested it with various combinations of dependencies? I've tested enabling "BR2_CCACHE" and "BR2_STRIP_sstrip" with qemu_x86_defconfig and seems works fine. The circular dependencies are gone. I think the patch now does not introduce any regression. I'm testing the whole patch set with top-level parallel make and enabling "BR2_CCACHE" and "BR2_STRIP_sstrip". Thanks and regards
On Fri, Oct 25, 2013 at 10:45 AM, Fabio Porcedda <fabio.porcedda@gmail.com> wrote: > On Fri, Oct 25, 2013 at 10:12 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >> On 25/10/13 10:07, Fabio Porcedda wrote: >>> >>> On Thu, Oct 24, 2013 at 12:37 PM, Arnout Vandecappelle<arnout@mind.be> >>> wrote: >>>> >>>> >On 24/10/13 09:41, Fabio Porcedda wrote: >>>>> >>>>> >> >> >> [snip] >> >>>>> >>I've found a work around: >>>>> >> >>>>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare >>>>> >> >>>>> >># to prevent circular dependency >>>>> >>ifneq ($(1),$(DEPENDENCIES_HOST_PREREQ)) >>>>> >>$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies >>>>> >>endif >>>>> >> >>>>> >>What do you think about that? >>>> >>>> > >>>> > >>>> > Looks good except that it's incorrect:-) If HOST_PREREQ contains both >>>> >ccache and sstrip (not to mention tar or xz), the condition won't match. >>>> > You >>>> >can try something like: >>>> > >>>> >ifeq ($(filter $(1),$(DEPENDENCIES_HOST_PREREQ)),) >>> >>> Ok good, I've updated the patch according. >>> >> >> Have you tested it with various combinations of dependencies? > > I've tested enabling "BR2_CCACHE" and "BR2_STRIP_sstrip" with > qemu_x86_defconfig and seems works fine. > The circular dependencies are gone. > I think the patch now does not introduce any regression. > > I'm testing the whole patch set with top-level parallel make and > enabling "BR2_CCACHE" and "BR2_STRIP_sstrip". The update patch seems to works fine. I found only an issue when enabling top-level parallel make and disabling /usr/bin/xz, but that is not a regression and it's an issue to solve with another patch. Regards
Hi Fabio, On Tue, Oct 29, 2013 at 9:36 AM, Fabio Porcedda <fabio.porcedda@gmail.com> wrote: [..] >>> Have you tested it with various combinations of dependencies? >> >> I've tested enabling "BR2_CCACHE" and "BR2_STRIP_sstrip" with >> qemu_x86_defconfig and seems works fine. >> The circular dependencies are gone. >> I think the patch now does not introduce any regression. >> >> I'm testing the whole patch set with top-level parallel make and >> enabling "BR2_CCACHE" and "BR2_STRIP_sstrip". > > The update patch seems to works fine. > I found only an issue when enabling top-level parallel make and > disabling /usr/bin/xz, > but that is not a regression and it's an issue to solve with another patch. > Can you elaborate on this? What is the problem you're seeing exactly, and how did you 'disable /usr/bin/xz' ? When buildroot cannot find a working xzcat version, it builds one itself through host-xz. This should still work in case of parallel builds. The same principle applies when the 'tar' version on host is non-existing or too old. Thanks, Thomas
On Tue, Oct 29, 2013 at 10:35 AM, Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > Hi Fabio, > > On Tue, Oct 29, 2013 at 9:36 AM, Fabio Porcedda > <fabio.porcedda@gmail.com> wrote: > [..] >>>> Have you tested it with various combinations of dependencies? >>> >>> I've tested enabling "BR2_CCACHE" and "BR2_STRIP_sstrip" with >>> qemu_x86_defconfig and seems works fine. >>> The circular dependencies are gone. >>> I think the patch now does not introduce any regression. >>> >>> I'm testing the whole patch set with top-level parallel make and >>> enabling "BR2_CCACHE" and "BR2_STRIP_sstrip". >> >> The update patch seems to works fine. >> I found only an issue when enabling top-level parallel make and >> disabling /usr/bin/xz, >> but that is not a regression and it's an issue to solve with another patch. >> > > Can you elaborate on this? What is the problem you're seeing exactly, > and how did you 'disable /usr/bin/xz' ? To disable /usr/bin/xz i've just renamed it so buildroot doesn't find a suitable xz. The host-ccache source file is a tar.xz so the host-xz is a dependency of host-ccache. I need to add a rule to ensure the right order. > When buildroot cannot find a working xzcat version, it builds one > itself through host-xz. This should still work in case of parallel > builds. > The same principle applies when the 'tar' version on host is > non-existing or too old. Yes i think is affected by the same problem. Thanks
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 4bba4b5..1e7154e 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -385,6 +385,8 @@ $(1)-install-host: $(1)-build $$($(2)_TARGET_INSTALL_HOST) $(1)-build: $(1)-configure \ $$($(2)_TARGET_BUILD) +$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies dirs prepare + ifeq ($$($(2)_OVERRIDE_SRCDIR),) # In the normal case (no package override), the sequence of steps is # source, by downloading diff --git a/toolchain/toolchain/toolchain.mk b/toolchain/toolchain/toolchain.mk index 44ed629..8559ac9 100644 --- a/toolchain/toolchain/toolchain.mk +++ b/toolchain/toolchain/toolchain.mk @@ -14,5 +14,4 @@ endif $(eval $(generic-package)) -toolchain-source: prepare dirs dependencies $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake - +toolchain: $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake
Move "dependencies" "dirs" "prepare" dependencies from "toolchain" to every package. This way we can build correctly every package right after the clean stage. As example with this commit we can build successfully the glibc right after the clean stage: make clean glibc This is also a step forward supporting top-level parallel make. Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> --- package/pkg-generic.mk | 2 ++ toolchain/toolchain/toolchain.mk | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-)