| Message ID | 82f1af6280a8c913d3df331813dec4376fda2fb5.1450031251.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Yann, Yann E. MORIN wrote: > Almost all packages which are saved for legal-info have their source > archives downloaded as part of 'make source', which makes an off-line > build completely possible [0]. > > However, for the pre-configured external toolchains, the source tarball > is different, as the main tarball is a binary package. And that source > tarball is only downloaded during the legal-info phase, which makes it > inconvenient for full off-line builds. > > We fix that in two steps: > > - first, we move the declaration of _ACTUAL_SOURCE_TARBALL and _SITE > earlier in the pkg-generic.mk file, so we get those definitions at > the time we define the -source and -all-source rules; > > - second, we add a new rule, $(1)-legal-source which only > $(1)-all-source depends on, so that we only download it for a > top-level 'make source', not as part of the standard download > mechanism (i.e. only what is really needed to build). > > This way, we can do a complete [0] off-line build and are still able to > generate legal-info, while at the same time we do not incur any download > overhead during a simple build. > > Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was > not empty. However, since _ACTUAL_SOURCE_TARBALL default to the value of > _SOURCE, it can not be empty when _SOURCE is not. Thus, we'd get a > spurious report of a missing hash for the tarball, since it was not in > a standard package rule (configure, build, install..) and thus would > miss the PKG and PKGDIR variables to find the .hash file. > > We fix that in this commit as well, by: > > - setting PKG and PKGDIR just for the -legal-source rule; > > - only downloading _ACTUAL_SOURCE_TARBALL if it is not empty *and* not > the same as _SOURCE (to avoid a second report about the hash). > > [0] Save for nodejs which invarriably wants to download stuff at build > time. Sigh... :-( Fixing that is work for another time... > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Luca Ceresoli <luca@lucaceresoli.net> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> Thanks for the very detailed commit log message. It's very useful! Just a couple of minor notes below. [...] > +# Download actual sources if they differ from the extracted sources > +# (e.g. for external toolchains) > +# > +# We need to provide PKG and PKGDIR, because there's no .stamp file for > +# the legal-info step. > +$(1)-legal-source: PKG=$(2) > +$(1)-legal-source: PKGDIR=$(pkgdir) > +$(1)-legal-source: > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > + $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > +endif # actuall sources != sources actuall sources -> actual source > +endif # actuall source != "" actuall -> actual With these fixed: Acked-by: Luca Ceresoli <luca@lucaceresoli.net> [Tested with a minimal Sourcery ARM 2014.05 config] Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
Dear Yann, Luca Ceresoli wrote: [...] >> +# Download actual sources if they differ from the extracted sources >> +# (e.g. for external toolchains) >> +# >> +# We need to provide PKG and PKGDIR, because there's no .stamp file for >> +# the legal-info step. >> +$(1)-legal-source: PKG=$(2) >> +$(1)-legal-source: PKGDIR=$(pkgdir) >> +$(1)-legal-source: >> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) >> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) >> + $$(call >> DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) >> +endif # actuall sources != sources > > actuall sources -> actual source > >> +endif # actuall source != "" > > actuall -> actual > > With these fixed: > Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > > [Tested with a minimal Sourcery ARM 2014.05 config] > Tested-by: Luca Ceresoli <luca@lucaceresoli.net> Hey, NO! This patch is wrong. It makes off-line legal-info work, and that's what I tested successfully: $ make clean $ rm -fr dl/ $ make source $ make legal-info (offline) with this defconfig: BR2_arm=y BR2_TOOLCHAIN_EXTERNAL=y But it breaks online 'make legal-info' when _ACTUAL_SOURCES have not yet been downloaded, i.e.: $ make clean $ rm -fr dl/ $ make legal-info [...] cp: cannot stat '/home/murray/devel/buildroot/dl/arm-2014.05-29-arm-none-linux-gnueabi.src.tar.bz2': No such file or directory I think this is because you moved the downloading of _ACTUAL_SOURCE from the $(1)-legal-info to $(1)-legal-source, and the former does not depend on the latter. Perhaps we just need this? $(1)-legal-info: $(1)-legal-source Sorry for having missed this during my first review.
Luca, All, On 2015-12-16 23:47 +0100, Luca Ceresoli spake thusly: > Luca Ceresoli wrote: > [...] > >>+# Download actual sources if they differ from the extracted sources > >>+# (e.g. for external toolchains) > >>+# > >>+# We need to provide PKG and PKGDIR, because there's no .stamp file for > >>+# the legal-info step. > >>+$(1)-legal-source: PKG=$(2) > >>+$(1)-legal-source: PKGDIR=$(pkgdir) > >>+$(1)-legal-source: > >>+ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > >>+ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > >>+ $$(call > >>DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > >>+endif # actuall sources != sources > > > >actuall sources -> actual source > > > >>+endif # actuall source != "" > > > >actuall -> actual > > > >With these fixed: > > Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > > > >[Tested with a minimal Sourcery ARM 2014.05 config] > >Tested-by: Luca Ceresoli <luca@lucaceresoli.net> > > Hey, NO! This patch is wrong. [--SNIP--] > with this defconfig: > > BR2_arm=y > BR2_TOOLCHAIN_EXTERNAL=y > > But it breaks online 'make legal-info' when _ACTUAL_SOURCES have not > yet been downloaded, i.e.: > > $ make clean > $ rm -fr dl/ > $ make legal-info > [...] > cp: cannot stat '/home/murray/devel/buildroot/dl/arm-2014.05-29-arm-none-linux-gnueabi.src.tar.bz2': > No such file or directory Damit... :-( > I think this is because you moved the downloading of _ACTUAL_SOURCE > from the $(1)-legal-info to $(1)-legal-source, and the former does not > depend on the latter. > > Perhaps we just need this? > > $(1)-legal-info: $(1)-legal-source Nope, that does not work either. The download indeed occurs, but then it can not find a hash for it. Indeed, we do not have ssuch a hash anywhere. I'll investigate tomorrow (or this WE). > Sorry for having missed this during my first review. No problem, thank you very much for the testing! :-) Regards, Yann E. MORIN.
On 17-12-15 00:05, Yann E. MORIN wrote: > Luca, All, > > On 2015-12-16 23:47 +0100, Luca Ceresoli spake thusly: >> Luca Ceresoli wrote: >> [...] >>>> +# Download actual sources if they differ from the extracted sources >>>> +# (e.g. for external toolchains) >>>> +# >>>> +# We need to provide PKG and PKGDIR, because there's no .stamp file for >>>> +# the legal-info step. >>>> +$(1)-legal-source: PKG=$(2) >>>> +$(1)-legal-source: PKGDIR=$(pkgdir) >>>> +$(1)-legal-source: >>>> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) >>>> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) >>>> + $$(call >>>> DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) >>>> +endif # actuall sources != sources >>> >>> actuall sources -> actual source >>> >>>> +endif # actuall source != "" >>> >>> actuall -> actual >>> >>> With these fixed: >>> Acked-by: Luca Ceresoli <luca@lucaceresoli.net> >>> >>> [Tested with a minimal Sourcery ARM 2014.05 config] >>> Tested-by: Luca Ceresoli <luca@lucaceresoli.net> >> >> Hey, NO! This patch is wrong. > [--SNIP--] >> with this defconfig: >> >> BR2_arm=y >> BR2_TOOLCHAIN_EXTERNAL=y >> >> But it breaks online 'make legal-info' when _ACTUAL_SOURCES have not >> yet been downloaded, i.e.: >> >> $ make clean >> $ rm -fr dl/ >> $ make legal-info >> [...] >> cp: cannot stat '/home/murray/devel/buildroot/dl/arm-2014.05-29-arm-none-linux-gnueabi.src.tar.bz2': >> No such file or directory > > Damit... :-( > >> I think this is because you moved the downloading of _ACTUAL_SOURCE >> from the $(1)-legal-info to $(1)-legal-source, and the former does not >> depend on the latter. >> >> Perhaps we just need this? >> >> $(1)-legal-info: $(1)-legal-source > > Nope, that does not work either. The download indeed occurs, but then it > can not find a hash for it. Indeed, we do not have ssuch a hash > anywhere. Isn't it just that we need to add hashes for the _ACTUAL_SOURCE_TARBALLs that we have? Regards, Arnout > > I'll investigate tomorrow (or this WE). > >> Sorry for having missed this during my first review. > > No problem, thank you very much for the testing! :-) > > Regards, > Yann E. MORIN. >
Arnout, All, On 2015-12-17 00:09 +0100, Arnout Vandecappelle spake thusly: > On 17-12-15 00:05, Yann E. MORIN wrote: > > On 2015-12-16 23:47 +0100, Luca Ceresoli spake thusly: > >> Luca Ceresoli wrote: > >> [...] > >>>> +# Download actual sources if they differ from the extracted sources > >>>> +# (e.g. for external toolchains) > >>>> +# > >>>> +# We need to provide PKG and PKGDIR, because there's no .stamp file for > >>>> +# the legal-info step. > >>>> +$(1)-legal-source: PKG=$(2) > >>>> +$(1)-legal-source: PKGDIR=$(pkgdir) > >>>> +$(1)-legal-source: > >>>> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > >>>> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > >>>> + $$(call > >>>> DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > >>>> +endif # actuall sources != sources > >>> > >>> actuall sources -> actual source > >>> > >>>> +endif # actuall source != "" > >>> > >>> actuall -> actual > >>> > >>> With these fixed: > >>> Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > >>> > >>> [Tested with a minimal Sourcery ARM 2014.05 config] > >>> Tested-by: Luca Ceresoli <luca@lucaceresoli.net> > >> > >> Hey, NO! This patch is wrong. > > [--SNIP--] > >> with this defconfig: > >> > >> BR2_arm=y > >> BR2_TOOLCHAIN_EXTERNAL=y > >> > >> But it breaks online 'make legal-info' when _ACTUAL_SOURCES have not > >> yet been downloaded, i.e.: > >> > >> $ make clean > >> $ rm -fr dl/ > >> $ make legal-info > >> [...] > >> cp: cannot stat '/home/murray/devel/buildroot/dl/arm-2014.05-29-arm-none-linux-gnueabi.src.tar.bz2': > >> No such file or directory > > > > Damit... :-( > > > >> I think this is because you moved the downloading of _ACTUAL_SOURCE > >> from the $(1)-legal-info to $(1)-legal-source, and the former does not > >> depend on the latter. > >> > >> Perhaps we just need this? > >> > >> $(1)-legal-info: $(1)-legal-source > > > > Nope, that does not work either. The download indeed occurs, but then it > > can not find a hash for it. Indeed, we do not have ssuch a hash > > anywhere. > > Isn't it just that we need to add hashes for the _ACTUAL_SOURCE_TARBALLs that > we have? It seems that does the trick, indeed. By "investigate", I really meant "adding the missing hashes". ;-) Anyway, I'll do some more tests later this week... Regards, Yann E. MORIN.
On 13-12-15 19:35, Yann E. MORIN wrote: > Almost all packages which are saved for legal-info have their source > archives downloaded as part of 'make source', which makes an off-line > build completely possible [0]. > > However, for the pre-configured external toolchains, the source tarball > is different, as the main tarball is a binary package. And that source > tarball is only downloaded during the legal-info phase, which makes it > inconvenient for full off-line builds. > > We fix that in two steps: > > - first, we move the declaration of _ACTUAL_SOURCE_TARBALL and _SITE > earlier in the pkg-generic.mk file, so we get those definitions at > the time we define the -source and -all-source rules; I don't see why this is needed: they are first use at exactly the place where they were originally defined. That said, these definitions fit much better where you put them now (just after the handling of _SOURCE), so keep it this way. Could be a seperate patch but not really required for me. > > - second, we add a new rule, $(1)-legal-source which only > $(1)-all-source depends on, so that we only download it for a > top-level 'make source', not as part of the standard download > mechanism (i.e. only what is really needed to build). > > This way, we can do a complete [0] off-line build and are still able to > generate legal-info, while at the same time we do not incur any download > overhead during a simple build. > > Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was > not empty. I haven't tested, but according to me this code already did the right thing: ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) endif It is true, however, that the original code didn't handle the case where a package explicitly declares ACTUAL_SOURCE_TARBALL to be empty. > However, since _ACTUAL_SOURCE_TARBALL default to the value of > _SOURCE, it can not be empty when _SOURCE is not. Thus, we'd get a > spurious report of a missing hash for the tarball, since it was not in > a standard package rule (configure, build, install..) and thus would > miss the PKG and PKGDIR variables to find the .hash file. > > We fix that in this commit as well, by: > > - setting PKG and PKGDIR just for the -legal-source rule; Have we still not gotten rid of PKGDIR? How silly... > > - only downloading _ACTUAL_SOURCE_TARBALL if it is not empty *and* not > the same as _SOURCE (to avoid a second report about the hash). > > [0] Save for nodejs which invarriably wants to download stuff at build > time. Sigh... :-( Fixing that is work for another time... > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Luca Ceresoli <luca@lucaceresoli.net> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Peter Korsgaard <jacmet@uclibc.org> > > --- > Changes v1 -> v2: > - drop the 'redistribute == ignore', it does not exist yet > --- > package/pkg-generic.mk | 36 ++++++++++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 10 deletions(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index e69c970..832045e 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -428,6 +428,14 @@ ifndef $(2)_SOURCE > endif > endif > > +# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is > +# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE > +# point to the actual sources tarball. Use the actual sources for legal-info. > +# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code, > +# so these are the defaults for FOO_ACTUAL_*. > +$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) > +$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) This is not really related to this patch (it was like that in the original), but why do we qstrip here? > + > ifndef $(2)_PATCH > ifdef $(3)_PATCH > $(2)_PATCH = $$($(3)_PATCH) > @@ -627,6 +635,10 @@ $(1)-depends: $$($(2)_FINAL_DEPENDENCIES) > > $(1)-source: $$($(2)_TARGET_SOURCE) > > +$(1)-all-source: $(1)-legal-source > + > +$(1)-legal-source: $(1)-source As noted by Luca, we're missing a -legal-info -> legal-source dependency here. > + > $(1)-source-check: > $$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep)) > > @@ -764,13 +776,19 @@ endif > endif > endif > > -# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is > -# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE > -# point to the actual sources tarball. Use the actual sources for legal-info. > -# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code, > -# so these are the defaults for FOO_ACTUAL_*. > -$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) > -$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) > +# Download actual sources if they differ from the extracted sources > +# (e.g. for external toolchains) > +# > +# We need to provide PKG and PKGDIR, because there's no .stamp file for > +# the legal-info step. > +$(1)-legal-source: PKG=$(2) > +$(1)-legal-source: PKGDIR=$(pkgdir) > +$(1)-legal-source: > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > + $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) Actually we should have the same kind of logic here as in _ALL_DOWNLOADS, to allow for multiple ACTUAL_SOURCE_TARBALLs and different SITEs. And that would also remove the need for the ACTUAL_SOURCE_SITE. And if we use a foreach, it would remove the need to check for an empty TARBALL. But of course, all that is not for this patch :-) > +endif # actuall sources != sources > +endif # actuall source != "" > > # legal-info: produce legally relevant info. > $(1)-legal-info: > @@ -802,9 +820,6 @@ else > # Other packages > > ifeq ($$($(2)_REDISTRIBUTE),YES) > -ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > - $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > -endif > # Copy the source tarball > $$(Q)$$(call hardlink-copy,\ > $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\ > @@ -907,6 +922,7 @@ endif > $(1)-show-depends \ > $(1)-show-version \ > $(1)-source \ > + $(1)-legal-source \ The rest of .PHONY is kept alphabetically ordered... Regards, Arnout > $(1)-source-check > > ifeq ($$(patsubst %/,ERROR,$$($(2)_SITE)),ERROR) >
Arnout, All, On 2015-12-17 00:19 +0100, Arnout Vandecappelle spake thusly: > On 13-12-15 19:35, Yann E. MORIN wrote: > > Almost all packages which are saved for legal-info have their source > > archives downloaded as part of 'make source', which makes an off-line > > build completely possible [0]. > > > > However, for the pre-configured external toolchains, the source tarball > > is different, as the main tarball is a binary package. And that source > > tarball is only downloaded during the legal-info phase, which makes it > > inconvenient for full off-line builds. > > > > We fix that in two steps: > > > > - first, we move the declaration of _ACTUAL_SOURCE_TARBALL and _SITE > > earlier in the pkg-generic.mk file, so we get those definitions at > > the time we define the -source and -all-source rules; > > I don't see why this is needed: they are first use at exactly the place where > they were originally defined. Yep. That was nto the case in a previous iteration (that I did not post), and the code has changed since then, but I overlooked that part of the commit log. Will fix it. > That said, these definitions fit much better where you put them now (just after > the handling of _SOURCE), so keep it this way. Could be a seperate patch but not > really required for me. Yep, I'll split and keep the move, so the code is "nicer". ;-) > > - second, we add a new rule, $(1)-legal-source which only > > $(1)-all-source depends on, so that we only download it for a > > top-level 'make source', not as part of the standard download > > mechanism (i.e. only what is really needed to build). > > > > This way, we can do a complete [0] off-line build and are still able to > > generate legal-info, while at the same time we do not incur any download > > overhead during a simple build. > > > > Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was > > not empty. > > I haven't tested, but according to me this code already did the right thing: > > ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > endif > > It is true, however, that the original code didn't handle the case where a > package explicitly declares ACTUAL_SOURCE_TARBALL to be empty. Yes, everything was downloaded, even missing pieces. However, this patch is about getting the missing pieces when running 'make source' for completely-offline builds. What was previously broken was; make menuconfig [enable whatever, like an external toolchain with an actual source] make source [unplug your ehternet cable, disable WiFi, get under a tunnel to avoid 3g...] make legal-info -> broken, it tries to download the actual sources. What this patch does is; - 'make source' will also download the actual sources - but since it can take some time to do so, we do not want to do it for a simple build, like just runnint 'make' > > However, since _ACTUAL_SOURCE_TARBALL default to the value of > > _SOURCE, it can not be empty when _SOURCE is not. Thus, we'd get a > > spurious report of a missing hash for the tarball, since it was not in > > a standard package rule (configure, build, install..) and thus would > > miss the PKG and PKGDIR variables to find the .hash file. > > > > We fix that in this commit as well, by: > > > > - setting PKG and PKGDIR just for the -legal-source rule; > > Have we still not gotten rid of PKGDIR? How silly... Hmm... It;s used elsewhere in the code, so I stick to it. Removing can be another series. [--SNIP--] > > +$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) > > +$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) > > This is not really related to this patch (it was like that in the original), > but why do we qstrip here? Good question. I just moved the code around. > > @@ -627,6 +635,10 @@ $(1)-depends: $$($(2)_FINAL_DEPENDENCIES) > > > > $(1)-source: $$($(2)_TARGET_SOURCE) > > > > +$(1)-all-source: $(1)-legal-source > > + > > +$(1)-legal-source: $(1)-source > > As noted by Luca, we're missing a -legal-info -> legal-source dependency here. Yup, already fixed here. Thanks! :-) > > $(1)-source-check: > > $$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep)) > > > > @@ -764,13 +776,19 @@ endif > > endif > > endif > > > > -# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is > > -# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE > > -# point to the actual sources tarball. Use the actual sources for legal-info. > > -# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code, > > -# so these are the defaults for FOO_ACTUAL_*. > > -$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) > > -$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) > > +# Download actual sources if they differ from the extracted sources > > +# (e.g. for external toolchains) > > +# > > +# We need to provide PKG and PKGDIR, because there's no .stamp file for > > +# the legal-info step. > > +$(1)-legal-source: PKG=$(2) > > +$(1)-legal-source: PKGDIR=$(pkgdir) > > +$(1)-legal-source: > > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > > + $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > > Actually we should have the same kind of logic here as in _ALL_DOWNLOADS, to > allow for multiple ACTUAL_SOURCE_TARBALLs and different SITEs. And that would > also remove the need for the ACTUAL_SOURCE_SITE. And if we use a foreach, it > would remove the need to check for an empty TARBALL. But of course, all that is > not for this patch :-) Yup, can be done later. > > @@ -907,6 +922,7 @@ endif > > $(1)-show-depends \ > > $(1)-show-version \ > > $(1)-source \ > > + $(1)-legal-source \ > > The rest of .PHONY is kept alphabetically ordered... OK, fixed. Thanks! :-) Regards, Yann E. MORIN.
Arnout, All, On 2015-12-18 23:11 +0100, Yann E. MORIN spake thusly: > On 2015-12-17 00:19 +0100, Arnout Vandecappelle spake thusly: > > On 13-12-15 19:35, Yann E. MORIN wrote: [--SNIP--] > > > - second, we add a new rule, $(1)-legal-source which only > > > $(1)-all-source depends on, so that we only download it for a > > > top-level 'make source', not as part of the standard download > > > mechanism (i.e. only what is really needed to build). > > > > > > This way, we can do a complete [0] off-line build and are still able to > > > generate legal-info, while at the same time we do not incur any download > > > overhead during a simple build. > > > > > > Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was > > > not empty. > > > > I haven't tested, but according to me this code already did the right thing: > > > > ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > > $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) > > endif > > > > It is true, however, that the original code didn't handle the case where a > > package explicitly declares ACTUAL_SOURCE_TARBALL to be empty. > > Yes, everything was downloaded, even missing pieces. Hmmm... I did not answer your question. I'm off-topic... OK, so now for the real answer to your real question: s/not empty/not equal to the _SOURCE/ But I'll improve the commit log in this respect. Regards, Yann E. MORIN.
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index e69c970..832045e 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -428,6 +428,14 @@ ifndef $(2)_SOURCE endif endif +# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is +# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE +# point to the actual sources tarball. Use the actual sources for legal-info. +# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code, +# so these are the defaults for FOO_ACTUAL_*. +$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) +$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) + ifndef $(2)_PATCH ifdef $(3)_PATCH $(2)_PATCH = $$($(3)_PATCH) @@ -627,6 +635,10 @@ $(1)-depends: $$($(2)_FINAL_DEPENDENCIES) $(1)-source: $$($(2)_TARGET_SOURCE) +$(1)-all-source: $(1)-legal-source + +$(1)-legal-source: $(1)-source + $(1)-source-check: $$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep)) @@ -764,13 +776,19 @@ endif endif endif -# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is -# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE -# point to the actual sources tarball. Use the actual sources for legal-info. -# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code, -# so these are the defaults for FOO_ACTUAL_*. -$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE) -$(2)_ACTUAL_SOURCE_SITE ?= $$(call qstrip,$$($(2)_SITE)) +# Download actual sources if they differ from the extracted sources +# (e.g. for external toolchains) +# +# We need to provide PKG and PKGDIR, because there's no .stamp file for +# the legal-info step. +$(1)-legal-source: PKG=$(2) +$(1)-legal-source: PKGDIR=$(pkgdir) +$(1)-legal-source: +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),) +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) + $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) +endif # actuall sources != sources +endif # actuall source != "" # legal-info: produce legally relevant info. $(1)-legal-info: @@ -802,9 +820,6 @@ else # Other packages ifeq ($$($(2)_REDISTRIBUTE),YES) -ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) - $$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL)) -endif # Copy the source tarball $$(Q)$$(call hardlink-copy,\ $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\ @@ -907,6 +922,7 @@ endif $(1)-show-depends \ $(1)-show-version \ $(1)-source \ + $(1)-legal-source \ $(1)-source-check ifeq ($$(patsubst %/,ERROR,$$($(2)_SITE)),ERROR)
Almost all packages which are saved for legal-info have their source archives downloaded as part of 'make source', which makes an off-line build completely possible [0]. However, for the pre-configured external toolchains, the source tarball is different, as the main tarball is a binary package. And that source tarball is only downloaded during the legal-info phase, which makes it inconvenient for full off-line builds. We fix that in two steps: - first, we move the declaration of _ACTUAL_SOURCE_TARBALL and _SITE earlier in the pkg-generic.mk file, so we get those definitions at the time we define the -source and -all-source rules; - second, we add a new rule, $(1)-legal-source which only $(1)-all-source depends on, so that we only download it for a top-level 'make source', not as part of the standard download mechanism (i.e. only what is really needed to build). This way, we can do a complete [0] off-line build and are still able to generate legal-info, while at the same time we do not incur any download overhead during a simple build. Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was not empty. However, since _ACTUAL_SOURCE_TARBALL default to the value of _SOURCE, it can not be empty when _SOURCE is not. Thus, we'd get a spurious report of a missing hash for the tarball, since it was not in a standard package rule (configure, build, install..) and thus would miss the PKG and PKGDIR variables to find the .hash file. We fix that in this commit as well, by: - setting PKG and PKGDIR just for the -legal-source rule; - only downloading _ACTUAL_SOURCE_TARBALL if it is not empty *and* not the same as _SOURCE (to avoid a second report about the hash). [0] Save for nodejs which invarriably wants to download stuff at build time. Sigh... :-( Fixing that is work for another time... Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Luca Ceresoli <luca@lucaceresoli.net> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Peter Korsgaard <jacmet@uclibc.org> --- Changes v1 -> v2: - drop the 'redistribute == ignore', it does not exist yet --- package/pkg-generic.mk | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)