| Message ID | 1420199015-16907-3-git-send-email-luca@lucaceresoli.net |
|---|---|
| State | Changes Requested |
| Headers | show |
On 02/01/15 12:43, Luca Ceresoli wrote: > The FOO_SITE/FOO_SOURCE variables usually point to a tarball containing > source code. > > For the downloaded external toolchains this is not true, the "source" > tarball actually contains binaries. This is fine for making Buildroot > work, but for legal-info we really want to ship real source code, not > binaries. > > Luckily, some (hopefully all) toolchain vendors publish a downloadable > tarball containing the source code counterpart for their binary > packages. > > Here we allow the user to declare the URL of this other tarball in the > pair of variables FOO_ACTUAL_SOURCE_TARBALL (by default equal to > FOO_SOURCE) and FOO_ACTUAL_SOURCE_SITE (by default equal to FOO_SITE). > If the "actual source" package can be downloaded from the same > directory as the binary package, then only FOO_ACTUAL_SOURCE_TARBALL > needs to be set. > > Note this change is not strictly toolchain-specific: it might be useful > for other packages that happen to ship binaries in the same way. > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> It is adding even more complexity to the already too complex legal-info target, but it doesn't look too bad. Although I would indeed prefer a pre-legal-info hook. But even that wouldn't be enough, because the source reference in the manifest would still be wrong... Some minor comments below. > --- > Makefile | 1 - > package/pkg-generic.mk | 23 ++++++++++++++++++----- > 2 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 5e0b4f2..9f96016 100644 > --- a/Makefile > +++ b/Makefile > @@ -640,7 +640,6 @@ legal-info-prepare: $(LEGAL_INFO_DIR) > @$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,HOST) > @$(call legal-manifest,buildroot,$(BR2_VERSION_FULL),GPLv2+,COPYING,not saved,not saved,HOST) > @$(call legal-warning,the Buildroot source code has not been saved) > - @$(call legal-warning,the toolchain has not been saved) > @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config > > legal-info: dirs legal-info-clean legal-info-prepare $(TARGETS_LEGAL_INFO) \ > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 38ef581..7a477af 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -648,12 +648,18 @@ ifneq ($$($(2)_SITE_METHOD),local) > ifneq ($$($(2)_SITE_METHOD),override) > # Packages that have a tarball need it downloaded beforehand > $(1)-legal-info: $(1)-source $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) > -$(2)_MANIFEST_TARBALL = $$($(2)_SOURCE) > -$(2)_MANIFEST_SITE = $$(call qstrip,$$($(2)_SITE)) > 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 msot 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)) > + > # legal-info: produce legally relevant info. > $(1)-legal-info: > # Packages without a source are assumed to be part of Buildroot, skip them. > @@ -684,13 +690,20 @@ else > # Other packages > > ifeq ($$($(2)_REDISTRIBUTE),YES) > +ifeq ($$($(2)_ACTUAL_SOURCE_TARBALL),) > + @$(call legal-warning,the toolchain source code has not been saved) So, this only works for the toolchain :-) If you keep this approach rather than the pre-legal-info-hook, then you probably want to use $(1) instead of toolchain. 'toolchain-external' should be sufficiently understandable. > +else > +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) > + $(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE:/=)/$$($(2)$($(PKG)_SITE:/=)_ACTUAL_SOURCE_TARBALL)) I think the $($(PKG)_SITE:/=) construct was just introduced because for some packages, the _SITE ends with a / and that should be stripped, and we were too lazy to fix the packages. Hm, looks like all the the external toolchain _SITEs end with a /... Regards, Arnout > +endif > # Copy the source tarball (just hardlink if possible) > - @cp -l $$(DL_DIR)/$$($(2)_SOURCE) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) 2>/dev/null || \ > - cp $$(DL_DIR)/$$($(2)_SOURCE) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) > + @cp -l $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) 2>/dev/null || \ > + cp $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) > +endif # ($$($(2)_ACTUAL_SOURCE_TARBALL),) > endif # redistribute > > endif # other packages > - @$$(call legal-manifest,$$($(2)_RAWNAME),$$($(2)_VERSION),$$($(2)_LICENSE),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_MANIFEST_TARBALL),$$($(2)_MANIFEST_SITE),$$(call UPPERCASE,$(4))) > + @$$(call legal-manifest,$$($(2)_RAWNAME),$$($(2)_VERSION),$$($(2)_LICENSE),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_ACTUAL_SOURCE_SITE),$$(call UPPERCASE,$(4))) > endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),) > $$(foreach hook,$$($(2)_POST_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep)) > >
On 02/02/15 22:47, Arnout Vandecappelle wrote: > On 02/01/15 12:43, Luca Ceresoli wrote: >> The FOO_SITE/FOO_SOURCE variables usually point to a tarball containing >> source code. >> >> For the downloaded external toolchains this is not true, the "source" >> tarball actually contains binaries. This is fine for making Buildroot >> work, but for legal-info we really want to ship real source code, not >> binaries. >> >> Luckily, some (hopefully all) toolchain vendors publish a downloadable >> tarball containing the source code counterpart for their binary >> packages. >> >> Here we allow the user to declare the URL of this other tarball in the >> pair of variables FOO_ACTUAL_SOURCE_TARBALL (by default equal to >> FOO_SOURCE) and FOO_ACTUAL_SOURCE_SITE (by default equal to FOO_SITE). >> If the "actual source" package can be downloaded from the same >> directory as the binary package, then only FOO_ACTUAL_SOURCE_TARBALL >> needs to be set. >> >> Note this change is not strictly toolchain-specific: it might be useful >> for other packages that happen to ship binaries in the same way. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > It is adding even more complexity to the already too complex legal-info target, > but it doesn't look too bad. Although I would indeed prefer a pre-legal-info > hook. But even that wouldn't be enough, because the source reference in the > manifest would still be wrong... Since my comments aren't serious and the pre-legal-info-hook is probably not feasible after all, you can just repost (with my Reviewed-by tag) if you don't feel like changing anything. Regards, Arnout [snip]
Dear Arnout, Arnout Vandecappelle wrote: > On 02/01/15 12:43, Luca Ceresoli wrote: >> The FOO_SITE/FOO_SOURCE variables usually point to a tarball containing >> source code. >> >> For the downloaded external toolchains this is not true, the "source" >> tarball actually contains binaries. This is fine for making Buildroot >> work, but for legal-info we really want to ship real source code, not >> binaries. >> >> Luckily, some (hopefully all) toolchain vendors publish a downloadable >> tarball containing the source code counterpart for their binary >> packages. >> >> Here we allow the user to declare the URL of this other tarball in the >> pair of variables FOO_ACTUAL_SOURCE_TARBALL (by default equal to >> FOO_SOURCE) and FOO_ACTUAL_SOURCE_SITE (by default equal to FOO_SITE). >> If the "actual source" package can be downloaded from the same >> directory as the binary package, then only FOO_ACTUAL_SOURCE_TARBALL >> needs to be set. >> >> Note this change is not strictly toolchain-specific: it might be useful >> for other packages that happen to ship binaries in the same way. >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > It is adding even more complexity to the already too complex legal-info target, > but it doesn't look too bad. Although I would indeed prefer a pre-legal-info > hook. But even that wouldn't be enough, because the source reference in the > manifest would still be wrong... That's right. Maybe this can be overcome in some (probably ugly) way. Raise your hand if you'd like to see: FOO_TOOLCHAIN_EXTERNAL_URL_TO_WRITE_IN_LEGAL_MANIFEST = <...> Did you discuss this patches with the other developers at the Buildroot meeting earlier this week? Overall, what do people think about this approach? Would it be even worth trying the hook-based implementation? Concerning the legal-info target complexity I fully agree with you. Some day I'd like to try to move the whole legal-info machinery out in a helper script, much like what Yann did for the download mechanisms. Not sure it's feasible, but it would probably make things cleaner. >> @@ -684,13 +690,20 @@ else >> # Other packages >> >> ifeq ($$($(2)_REDISTRIBUTE),YES) >> +ifeq ($$($(2)_ACTUAL_SOURCE_TARBALL),) >> + @$(call legal-warning,the toolchain source code has not been saved) > > So, this only works for the toolchain :-) > > If you keep this approach rather than the pre-legal-info-hook, then you > probably want to use $(1) instead of toolchain. 'toolchain-external' should be > sufficiently understandable. Sure, will do. > >> +else >> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) >> + $(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE:/=)/$$($(2)$($(PKG)_SITE:/=)_ACTUAL_SOURCE_TARBALL)) > > I think the $($(PKG)_SITE:/=) construct was just introduced because for some > packages, the _SITE ends with a / and that should be stripped, and we were too > lazy to fix the packages. Hm, looks like all the the external toolchain _SITEs > end with a /... So I'll remove all of those '/'s from toolchain-external.mk first... Regards,
Hi, for the records, I have not forgot this patchset... Luca Ceresoli wrote: > Dear Arnout, > > Arnout Vandecappelle wrote: [...] >>> +else >>> +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) >>> + $(call >>> DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE:/=)/$$($(2)$($(PKG)_SITE:/=)_ACTUAL_SOURCE_TARBALL)) >>> >> >> I think the $($(PKG)_SITE:/=) construct was just introduced because >> for some >> packages, the _SITE ends with a / and that should be stripped, and we >> were too >> lazy to fix the packages. Hm, looks like all the the external >> toolchain _SITEs >> end with a /... > > So I'll remove all of those '/'s from toolchain-external.mk first... I started working on Arnout's comment above and sent a patchset to cleanup _SITE URLs with a trailing '/'. I sent a patchset in March (http://lists.busybox.net/pipermail/buildroot/2015-March/121502.html) for that. That patchset was partially applied, but an improvement was asked before it could be fully applied. So the present patchset sits on the bottom of my stack, until I have time to implement the requested improvement... I marked this patched as "Changes Requested" in patchwork, and will send a v2 when possible, along with the other improvements requested in this thread (which hopefully won't push again on my stack!).
diff --git a/Makefile b/Makefile index 5e0b4f2..9f96016 100644 --- a/Makefile +++ b/Makefile @@ -640,7 +640,6 @@ legal-info-prepare: $(LEGAL_INFO_DIR) @$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,HOST) @$(call legal-manifest,buildroot,$(BR2_VERSION_FULL),GPLv2+,COPYING,not saved,not saved,HOST) @$(call legal-warning,the Buildroot source code has not been saved) - @$(call legal-warning,the toolchain has not been saved) @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config legal-info: dirs legal-info-clean legal-info-prepare $(TARGETS_LEGAL_INFO) \ diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 38ef581..7a477af 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -648,12 +648,18 @@ ifneq ($$($(2)_SITE_METHOD),local) ifneq ($$($(2)_SITE_METHOD),override) # Packages that have a tarball need it downloaded beforehand $(1)-legal-info: $(1)-source $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) -$(2)_MANIFEST_TARBALL = $$($(2)_SOURCE) -$(2)_MANIFEST_SITE = $$(call qstrip,$$($(2)_SITE)) 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 msot 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)) + # legal-info: produce legally relevant info. $(1)-legal-info: # Packages without a source are assumed to be part of Buildroot, skip them. @@ -684,13 +690,20 @@ else # Other packages ifeq ($$($(2)_REDISTRIBUTE),YES) +ifeq ($$($(2)_ACTUAL_SOURCE_TARBALL),) + @$(call legal-warning,the toolchain source code has not been saved) +else +ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE)) + $(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE:/=)/$$($(2)_ACTUAL_SOURCE_TARBALL)) +endif # Copy the source tarball (just hardlink if possible) - @cp -l $$(DL_DIR)/$$($(2)_SOURCE) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) 2>/dev/null || \ - cp $$(DL_DIR)/$$($(2)_SOURCE) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) + @cp -l $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) 2>/dev/null || \ + cp $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) +endif # ($$($(2)_ACTUAL_SOURCE_TARBALL),) endif # redistribute endif # other packages - @$$(call legal-manifest,$$($(2)_RAWNAME),$$($(2)_VERSION),$$($(2)_LICENSE),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_MANIFEST_TARBALL),$$($(2)_MANIFEST_SITE),$$(call UPPERCASE,$(4))) + @$$(call legal-manifest,$$($(2)_RAWNAME),$$($(2)_VERSION),$$($(2)_LICENSE),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_ACTUAL_SOURCE_SITE),$$(call UPPERCASE,$(4))) endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),) $$(foreach hook,$$($(2)_POST_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))
The FOO_SITE/FOO_SOURCE variables usually point to a tarball containing source code. For the downloaded external toolchains this is not true, the "source" tarball actually contains binaries. This is fine for making Buildroot work, but for legal-info we really want to ship real source code, not binaries. Luckily, some (hopefully all) toolchain vendors publish a downloadable tarball containing the source code counterpart for their binary packages. Here we allow the user to declare the URL of this other tarball in the pair of variables FOO_ACTUAL_SOURCE_TARBALL (by default equal to FOO_SOURCE) and FOO_ACTUAL_SOURCE_SITE (by default equal to FOO_SITE). If the "actual source" package can be downloaded from the same directory as the binary package, then only FOO_ACTUAL_SOURCE_TARBALL needs to be set. Note this change is not strictly toolchain-specific: it might be useful for other packages that happen to ship binaries in the same way. Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com> --- Makefile | 1 - package/pkg-generic.mk | 23 ++++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-)