[Buildroot] [PATCH v2] core/legal-info: Add package dependencies with licenses to the manifest
Yann E. MORIN
yann.morin.1998 at free.fr
Sun Aug 12 14:22:02 UTC 2018
Michal, All,
On 2018-08-10 16:03 +0200, sojkam1 at fel.cvut.cz spake thusly:
> From: Michal Sojka <sojka at 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 at 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...
> * producing some output even for host packages
> * documented legal-deps macro
I have further comments, see below as well...
[--SNIP--]
> 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.)
> 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.
> +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)\
+ )\
+ )
$(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. ;-)
Regards,
Yann E. MORIN.
> --
> 2.18.0
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
More information about the buildroot
mailing list