[v2] core/legal-info: Add package dependencies with licenses to the manifest

Message ID 20180810140322.2748-1-sojkam1@fel.cvut.cz
State Changes Requested
Headers show
Series
  • [v2] core/legal-info: Add package dependencies with licenses to the manifest
Related show

Commit Message

Michal Sojka Aug. 10, 2018, 2:03 p.m.
From: Michal Sojka <sojka@merica.cz>

This adds one column to the legal-info manifest table. It contains the
dependencies of the given package and their licenses. This information
is useful when assessing license compatibility of the packages and
their libraries.

An example of the content of the new column for the MPD package is
shown below:

    "alsa-lib [LGPL-2.1+ (library), GPL-2.0+ (aserver)], boost
    [BSL-1.0], libid3tag [GPL-2.0+], libmad [GPL-2.0+], libogg
    [BSD-3-Clause], libvorbis [BSD-3-Clause], libzlib [Zlib],
    skeleton-init-common [unknown], skeleton-init-sysv [unknown],
    sqlite [Public domain], toolchain-external-linaro-arm [unknown], "

Signed-off-by: Michal Sojka <sojka@merica.cz>
---
Changes against v1:

* switched parameters of legal-manifest (added one is the last)
* producing some output even for host packages
* documented legal-deps macro


 Makefile               |  6 +++---
 package/pkg-generic.mk |  2 +-
 package/pkg-utils.mk   | 23 +++++++++++++++++++++--
 3 files changed, 25 insertions(+), 6 deletions(-)

Comments

Matthew Weber Aug. 13, 2018, 1:40 p.m. | #1
Yann,
On Sun, Aug 12, 2018 at 9:22 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Michal, All,
>
> On 2018-08-10 16:03 +0200, sojkam1@fel.cvut.cz spake thusly:
> > From: Michal Sojka <sojka@merica.cz>
> >
> > This adds one column to the legal-info manifest table. It contains the
> > dependencies of the given package and their licenses. This information
> > is useful when assessing license compatibility of the packages and
> > their libraries.
> >
> > An example of the content of the new column for the MPD package is
> > shown below:
> >
> >     "alsa-lib [LGPL-2.1+ (library), GPL-2.0+ (aserver)], boost
> >     [BSL-1.0], libid3tag [GPL-2.0+], libmad [GPL-2.0+], libogg
> >     [BSD-3-Clause], libvorbis [BSD-3-Clause], libzlib [Zlib],
> >     skeleton-init-common [unknown], skeleton-init-sysv [unknown],
> >     sqlite [Public domain], toolchain-external-linaro-arm [unknown], "
>
> I believe this is a very good addition to the manifest. Good idea! :-)
>
> The trailing comma is ugly, though. I would just drop the coma
> altogether...
>
> And here, I have two spaces between each packages:
>
>     "alsa-lib [LGPL-2.1+ (library), GPL-2.0+ (aserver)],  boost
>     [BSL-1.0],  libid3tag [GPL-2.0+],  libmad [GPL-2.0+],  [...]"
>
> > Signed-off-by: Michal Sojka <sojka@merica.cz>
> > ---
> > Changes against v1:
> > * switched parameters of legal-manifest (added one is the last)
>
> Actually, I disagree with that one: it is OK that new parameters be
> added before the last, especially since the 'legal-manifest' macro
> would be easier to review, see below...

If we change the format of the legal info csv, is there someway we
could determine version of that file's syntax?  I assume worst case we
can parse out the first line and see the additional dependencies
entry?

I'm concerned about external tools impact to changing this file's
format.   I'm sure there are others that use this file for CVE
analysis and legal reporting.

Matt
Yann E. MORIN Aug. 13, 2018, 3:18 p.m. | #2
Matthew, All,

On 2018-08-13 08:40 -0500, Matthew Weber spake thusly:
> On Sun, Aug 12, 2018 at 9:22 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > On 2018-08-10 16:03 +0200, sojkam1@fel.cvut.cz spake thusly:
[--SNIP--]
> > > Changes against v1:
> > > * switched parameters of legal-manifest (added one is the last)
> > Actually, I disagree with that one: it is OK that new parameters be
> > added before the last, especially since the 'legal-manifest' macro
> > would be easier to review, see below...
> 
> If we change the format of the legal info csv, is there someway we
> could determine version of that file's syntax?  I assume worst case we
> can parse out the first line and see the additional dependencies
> entry?

So, I am not arguing that we should change the output at all.

What I am saying is that the _list_of_parameters_ can be reorganised,
while still keeping the output as before.

I.e. the 'legal-manifest' macro could be rewrittent from the current:

    define legal-manifest # pkg, version, license, license-files, source, url, {HOST|TARGET}
        echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)"' >>$(LEGAL_MANIFEST_CSV_$(7))
    endef

to this new one:

    define legal-manifest # {HOST|TARGET}, pkg, version, license, license-files, source, url, dependencies
        echo '"$(2)","$(3)","$(4)","$(5)","$(6)","$(7)","$(8)"' >>$(LEGAL_MANIFEST_CSV_$(1))
    endef

Regards,
Yann E. MORIN.
Michal Sojka Aug. 20, 2018, 1:57 p.m. | #3
Dear Yann and others,

On Sun, Aug 12 2018, Yann E. MORIN wrote:
> Michal, All,
>
> On 2018-08-10 16:03 +0200, sojkam1@fel.cvut.cz spake thusly:
>> From: Michal Sojka <sojka@merica.cz>
>> 
>> This adds one column to the legal-info manifest table. It contains the
>> dependencies of the given package and their licenses. This information
>> is useful when assessing license compatibility of the packages and
>> their libraries.
>> 
>> An example of the content of the new column for the MPD package is
>> shown below:
>> 
>>     "alsa-lib [LGPL-2.1+ (library), GPL-2.0+ (aserver)], boost
>>     [BSL-1.0], libid3tag [GPL-2.0+], libmad [GPL-2.0+], libogg
>>     [BSD-3-Clause], libvorbis [BSD-3-Clause], libzlib [Zlib],
>>     skeleton-init-common [unknown], skeleton-init-sysv [unknown],
>>     sqlite [Public domain], toolchain-external-linaro-arm [unknown], "
>
> I believe this is a very good addition to the manifest. Good idea! :-)
>
> The trailing comma is ugly, though. I would just drop the coma
> altogether...
>
> And here, I have two spaces between each packages:
>

[...]

>> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
>> index c3acc22b17..f7a3609443 100644
>> --- a/package/pkg-utils.mk
>> +++ b/package/pkg-utils.mk
>> @@ -79,8 +79,8 @@ define legal-warning-nosource # pkg, {local|override}
>>         $(call legal-warning-pkg,$(1),sources not saved ($(2) packages not handled))
>>  endef
>> 
>> -define legal-manifest # pkg, version, license, license-files, source, url, {HOST|TARGET}
>> -	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)"' >>$(LEGAL_MANIFEST_CSV_$(7))
>> +define legal-manifest # pkg, version, license, license-files, source, url, {HOST|TARGET}, dependencies
>> +	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)","$(8)"' >>$(LEGAL_MANIFEST_CSV_$(7))
>>  endef
>
> This change would have been (slightly) easier to review if the order of
> parameters was changed.
>
> In fact, I even believe a beter order would be:
>
>     {HOST|TARGET}, pkg, version, license, license-files, source, url, dependencies
>
> (if you decide to go with this change too, then make it a separate patch
> that comes first.)

Yes, I think that this order is more natural.

>> define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
>> @@ -95,3 +95,22 @@ define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-full
>>  	} && \
>>  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
>>  endef
>> +
>> +remove-virtual-pkgs = $(foreach p,$(1),$(if $($(call UPPERCASE,$(p))_IS_VIRTUAL),,$(p)))
>> +get-direct-deps = $(sort $(foreach p,$(1),$($(call UPPERCASE,$(p))_FINAL_DEPENDENCIES)))
>
> This should be using $(2)_FINAL_ALL_DEPENDENCIES and not $(2)_FINAL_DEPENDENCIES,
> because the _EXTRACT_DEPENDENCIES and _PATCH_DEPENDENCIES can also be used
> to alter the content of the package. For example, we use _PATCH_DEPENDENCIES
> for the linux extensions, and those are changing the source code of the
> linux package, so their licensing terms also apply.

OK

>> +define get-transitive-deps # packages
>> +	$(if $(filter-out $(1),$(call get-direct-deps,$(1))),\
>> +	     $(sort $(1) $(call get-transitive-deps,$(filter-out $(1),$(call get-direct-deps,$(1))))),\
>> +	     $(1))
>> +endef
>
> What about this new variable instead:
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a539483ae4..ca55ac5e8e 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -609,6 +609,14 @@ $(2)_FINAL_ALL_DEPENDENCIES = \
>  		$$($(2)_FINAL_DEPENDENCIES) \
>  		$$($(2)_FINAL_EXTRACT_DEPENDENCIES) \
>  		$$($(2)_FINAL_PATCH_DEPENDENCIES))
> +$(2)_FINAL_RECURSIVE_DEPENDENCIES = \
> +	$$(sort \
> +		$$(foreach p,\
> +			$$($(2)_FINAL_ALL_DEPENDENCIES),\
> +			$$(p)\
> +			$$($$(call UPPERCASE,$$(p))_FINAL_RECURSIVE_DEPENDENCIES)\
> +		)\
> +	)

This looks nice. When I tried it, `make printvars` was about 4 times
slower. I don't know whether it's a problem or not, but I use printvars
quite often during debugging of various stuff. Of course, if I limit the
variables with VARS=..., it is fast again.

>  
>  $(2)_INSTALL_STAGING?= NO
>  $(2)_INSTALL_IMAGES?= NO
>
>> +non-virtual-deps = $(call remove-virtual-pkgs,$(filter-out $(1),$(call get-transitive-deps,$(1))))
>
> And this new macro:
>
> non-virtual-deps = $(foreach p,$(1),$(if $($(call UPPERCASE,$(p))_IS_VIRTUAL),,$(p)))
>
>> +host-pattern-if-target-pkg = $(if $(1:host-%=),host-%,)
>
> I am not sure I grok that one... Oh, OK I got it. I don't think we need
> an intermediate variable (partly because its name is longer than the code
> it replaces...)
>
>> +# Produce a comma-separated list of dependent packages and their
>> +# licenses. Host packages are removed from the list if the argument is
>> +# not a host package.
>> +define legal-deps # package
>> +$(foreach p,$(filter-out $(call host-pattern-if-target-pkg,$(1)),$(call non-virtual-deps,$(1))),$(p) [$($(call UPPERCASE,$(p))_LICENSE)], )
>> +endef
>
> ... and then:
>
> # $1: package name, lowercase
> legal-deps = \
>     $(foreach p,\
>         $(filter-out $(if $(1:host-%=),host-%),\
>             $($(call UPPERCASE,$(1))_FINAL_RECURSIVE_DEPENDENCIES)),\
>         $(p) [$($(call UPPERCASE,$(p))_LICENSE)]\
>     )

> This is totally untested, but owrth investigating I think. ;-)

Yes. It also produces double spaces. I managed to fix that only by
putting the last three lines on a single line.

I'll send v3 with the current state.

-Michal

Patch

diff --git a/Makefile b/Makefile
index f79d39fd26..cadf43146b 100644
--- a/Makefile
+++ b/Makefile
@@ -781,9 +781,9 @@  legal-info-clean:
 legal-info-prepare: $(LEGAL_INFO_DIR)
 	@$(call MESSAGE,"Buildroot $(BR2_VERSION_FULL) Collecting legal info")
 	@$(call legal-license-file,buildroot,buildroot,support/legal-info,COPYING,COPYING,HOST)
-	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,TARGET)
-	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,HOST)
-	@$(call legal-manifest,buildroot,$(BR2_VERSION_FULL),GPL-2.0+,COPYING,not saved,not saved,HOST)
+	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,TARGET,DEPENDENCIES WITH LICENSES)
+	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,HOST,DEPENDENCIES WITH LICENSES)
+	@$(call legal-manifest,buildroot,$(BR2_VERSION_FULL),GPL-2.0+,COPYING,not saved,not saved,HOST,)
 	@$(call legal-warning,the Buildroot source code has not been saved)
 	@cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config
 
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 91b61c6de0..a539483ae4 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -936,7 +936,7 @@  ifeq ($$($(2)_REDISTRIBUTE),YES)
 endif # redistribute
 
 endif # other packages
-	@$$(call legal-manifest,$$($(2)_RAWNAME),$$($(2)_VERSION),$$($(2)_LICENSE),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_ACTUAL_SOURCE_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)),$$(call legal-deps,$(1)))
 endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
 	$$(foreach hook,$$($(2)_POST_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))
 
diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index c3acc22b17..f7a3609443 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -79,8 +79,8 @@  define legal-warning-nosource # pkg, {local|override}
 	$(call legal-warning-pkg,$(1),sources not saved ($(2) packages not handled))
 endef
 
-define legal-manifest # pkg, version, license, license-files, source, url, {HOST|TARGET}
-	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)"' >>$(LEGAL_MANIFEST_CSV_$(7))
+define legal-manifest # pkg, version, license, license-files, source, url, {HOST|TARGET}, dependencies
+	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)","$(8)"' >>$(LEGAL_MANIFEST_CSV_$(7))
 endef
 
 define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
@@ -95,3 +95,22 @@  define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-full
 	} && \
 	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
+
+remove-virtual-pkgs = $(foreach p,$(1),$(if $($(call UPPERCASE,$(p))_IS_VIRTUAL),,$(p)))
+get-direct-deps = $(sort $(foreach p,$(1),$($(call UPPERCASE,$(p))_FINAL_DEPENDENCIES)))
+
+define get-transitive-deps # packages
+	$(if $(filter-out $(1),$(call get-direct-deps,$(1))),\
+	     $(sort $(1) $(call get-transitive-deps,$(filter-out $(1),$(call get-direct-deps,$(1))))),\
+	     $(1))
+endef
+
+non-virtual-deps = $(call remove-virtual-pkgs,$(filter-out $(1),$(call get-transitive-deps,$(1))))
+host-pattern-if-target-pkg = $(if $(1:host-%=),host-%,)
+
+# Produce a comma-separated list of dependent packages and their
+# licenses. Host packages are removed from the list if the argument is
+# not a host package.
+define legal-deps # package
+$(foreach p,$(filter-out $(call host-pattern-if-target-pkg,$(1)),$(call non-virtual-deps,$(1))),$(p) [$($(call UPPERCASE,$(p))_LICENSE)], )
+endef