[Buildroot] [PATCH 0/5 v5] legal-info: extract packages to get license files (branch yem/legal)

[Buildroot] [PATCH 0/5 v5] legal-info: extract packages to get license files (branch yem/legal)

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Hello All!

Here is a series that fixes the legal-info infra:
  - extracts sources before copying license files
  - saves license files even for no-redistribute packages
  - cleans up to account for the changes (comment and helper rename)


Changes v4 -> v5:
  - don't warn about missing license fioles for packages bundled
    with Buildroot  (Luca)
  - new patch 5/5 to display a proper error message for missing license
    files
  - typoes

Changes v3 -> v4:
  - save licens files for no-redistribute packages  (Luca)
  - download sources before copying to legal-info/sources/
  - rename a legal-info helper

Changes v2 -> v3:
  - fix manifest for no-redistribute, or overriden, or local packages


Regards,
Yann E. MORIN.


The following changes since commit 712414ce2dc6f40ed98a3d8007e7d7ec382f79d4:

  gcc: drop redundant explicit version handling for aarch64 (2014-06-13 23:01:16 +0200)

are available in the git repository at:

  git://gitorious.org/buildroot/buildroot.git yem/legal

for you to fetch changes up to 8c742a7155e0b24c14ae90f7be00c1043eceb848:

  legal-info: properly error out on missing license file (2014-06-14 16:59:40 +0200)

----------------------------------------------------------------
Yann E. MORIN (5):
      legal-info: extract even no-redistribute packages
      legal-info: save license files even for local or overriden packages
      legal-info: add a comment about what packages we save the tarballs of
      legal-info: rename legal-warning-pkg-savednothing helper
      legal-info: properly error out on missing license file

 package/pkg-generic.mk | 39 +++++++++++++++++++++++++++------------
 package/pkg-utils.mk   | 22 +++++++++++++++-------
 2 files changed, 42 insertions(+), 19 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/5] legal-info: extract even no-redistribute packages

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Currently, if a package is marked _REDISTRIBUTE = NO, then legal-info
will not try to extract it first.

If that package also declares some _LICENSE_FILES, legal-info fails
if it is the only action we're trying to run:

    $ cat defconfig
    BR2_INIT_NONE=y
    BR2_PACKAGE_LIBFSLCODEC=y
    $ make BR2_DEFCONFIG=$(pwd)/defconfig defconfig
    $ make libfslcodec-legal-info
    /bin/sh: /home/ymorin/dev/buildroot/O/legal-info/licenses.txt: No such file or directory
    make[1]: *** [libfslcodec-legal-info] Error 1

Fix this by always having legal-info extract the archives if one or
more _LICENSE_FILES are specified.

We do this for all types of packages: overriden, local or 'normal'
remote packages. Even though we do not save the sources for the
overriden or local packages, we need to save their licensing info,
so we need to extract them.

This implies that we now need to explicitly add PKG-source as a dependency
of legal-info for packages we want to save (ie. redistributable, non-local
and non-overriden packages).

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Fabio Porcedda <fabio.porcedda@gmail.com>

---
Changes v4 -> v5:
  - my usual s/pacakge/package/

Changes v3 -> v4:
  - legal-info needs to depend on PKG-source when it needs to save a
    package's tarball

Changes v2 -> v3:
  - don't include source URL of no-redistribute, or overriden, or local
    packages in the manifest

Changes v1 -> v2:
  - this is not fixing the autobuilders failure it was written to fix,
    so remove the references to such build failures  (Thomas P)
  - also extract overriden and local packages  (Fabio)
---
 package/pkg-generic.mk | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index ccd7f3b..eb3ec9f 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -595,11 +595,18 @@ $(2)_MANIFEST_LICENSE_FILES = $$($(2)_LICENSE_FILES)
 endif
 $(2)_MANIFEST_LICENSE_FILES ?= not saved
 
+# If the package declares _LICENSE_FILES, we need to extract it,
+# for overriden, local or normal remote packages alike, whether
+# we want to redistribute it or not.
+ifneq ($$($(2)_LICENSE_FILES),)
+$(1)-legal-info: $(1)-extract
+endif
+
 ifeq ($$($(2)_REDISTRIBUTE),YES)
 ifneq ($$($(2)_SITE_METHOD),local)
 ifneq ($$($(2)_SITE_METHOD),override)
-# Packages that have a tarball need it downloaded and extracted beforehand
-$(1)-legal-info: $(1)-extract $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4)))
+# We need to download the package archive if we are to save it
+$(1)-legal-info: $(1)-source $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4)))
 $(2)_MANIFEST_TARBALL = $$($(2)_SOURCE)
 endif
 endif
-- 
1.8.3.2

[Buildroot] [PATCH 2/5] legal-info: save license files even for local or overriden packages

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Even if we do not save the sources for local or overrident packages because
it is too complex, we can still quite easily save the license files.

Also, having the license files is a very important part of complying with
the licenses.

Move the copy of license files out of the non-local, non-overriden package
case, but still in the case where packages have a _SOURCE defined, to
avoid catching packages bundled in Buildroot (eg. mkpasswd et al.)

Reported-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Fabio Porcedda <fabio.porcedda@gmail.com>

---
Changes v4 -> v5:
  - avoid warning for packages bundled in Buildroot  (Luca)
  - the change is not a bout non-redistributable packages, but about
    local and overriden packages
---
 package/pkg-generic.mk | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index eb3ec9f..a978e63 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -619,17 +619,12 @@ $(1)-legal-info:
 	$(foreach hook,$($(2)_PRE_LEGAL_INFO_HOOKS),$(call $(hook))$(sep))
 ifneq ($(call qstrip,$$($(2)_SOURCE)),)
 
-ifeq ($$($(2)_SITE_METHOD),local)
-# Packages without a tarball: don't save and warn
-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
-
-else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
-
-else
-# Other packages
-
 # Save license files if defined
+# We save the license files for any kind of package: normal, local,
+# overriden, or non-redistributable alike.
+# The reason to save license files even for no-redistribute packages
+# is that the license still applies to the files distributed as part
+# of the rootfs, even if the sources are not themselves redistributed.
 ifeq ($(call qstrip,$$($(2)_LICENSE_FILES)),)
 	@$(call legal-license-nofiles,$$($(2)_RAWNAME),$(call UPPERCASE,$(4)))
 	@$(call legal-warning-pkg,$$($(2)_RAWNAME),cannot save license ($(2)_LICENSE_FILES not defined))
@@ -640,6 +635,16 @@ else
 	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$(F),$$($(2)_DIR)/$$(F),$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
+ifeq ($$($(2)_SITE_METHOD),local)
+# Packages without a tarball: don't save and warn
+	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
+
+else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
+	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
+
+else
+# Other packages
+
 ifeq ($$($(2)_REDISTRIBUTE),YES)
 # Copy the source tarball (just hardlink if possible)
 	@cp -l $(DL_DIR)/$$($(2)_SOURCE) $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4))) 2>/dev/null || \
-- 
1.8.3.2

[Buildroot] [PATCH 3/5] legal-info: add a comment about what packages we save the tarballs of

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

As the legal-info infra only (rightfully) saves the tarballs of packages
that:
  - we want to redistribute,
  - and are not local,
  - and are not overriden,

add a comment stating so.

This should clarify the code-block, which although trivial to read,
was not easy to interpret without thinking thouroughly about it.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
---
 package/pkg-generic.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index a978e63..d16908a 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -602,6 +602,9 @@ ifneq ($$($(2)_LICENSE_FILES),)
 $(1)-legal-info: $(1)-extract
 endif
 
+# We only save the sources of packages we want to redistribute, that are
+# non-local, and non-overriden. So only store, in the manifest, the tarball
+# name of those packages.
 ifeq ($$($(2)_REDISTRIBUTE),YES)
 ifneq ($$($(2)_SITE_METHOD),local)
 ifneq ($$($(2)_SITE_METHOD),override)
-- 
1.8.3.2

[Buildroot] [PATCH 4/5] legal-info: rename legal-warning-pkg-savednothing helper

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

This helper was called when none of the sources or license
files were saved.

Now we handle license files separately from the sources,
this is no longer the case: they are only called when the
sources are not saved.

Rename the handler and change the warning message accordingly.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
---
 package/pkg-generic.mk | 4 ++--
 package/pkg-utils.mk   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index d16908a..8a32f9d 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -640,10 +640,10 @@ endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)
 # Packages without a tarball: don't save and warn
-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
+	@$(call legal-license-nosource,$$($(2)_RAWNAME),local)
 
 else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
+	@$(call legal-license-nosource,$$($(2)_RAWNAME),override)
 
 else
 # Other packages
diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index 163ef8a..471f3ed 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -101,8 +101,8 @@ define legal-warning-pkg # pkg, text
 	echo "WARNING: $(1): $(2)" >>$(LEGAL_WARNINGS)
 endef
 
-define legal-warning-pkg-savednothing # pkg, {local|override}
-	$(call legal-warning-pkg,$(1),sources and license files not saved ($(2) packages not handled))
+define legal-license-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, {HOST|TARGET}
-- 
1.8.3.2

[Buildroot] [PATCH 5/5] legal-info: properly error out on missing license file

[Yann E. MORIN]

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

When saving the license files, if a declared license file is missing,
print a proper error message, instead of the sometime-criptic error
message from 'cp'.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Fabio Porcedda <fabio.porcedda@gmail.com>

---
Note: when I wrote this back in March 2014, I had a concrete example of
a criptic error message (the name of the license did not make it obvious
we were saving a license file?), but today I can't remember what it was
exactly.

Since I believe showing a properly-formatted error message to the user
is anyway a good thing, I kept that patch. Feel free to ditch it if you
don't like it.
---
 package/pkg-utils.mk | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index 471f3ed..c3f9142 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -101,6 +101,10 @@ define legal-warning-pkg # pkg, text
 	echo "WARNING: $(1): $(2)" >>$(LEGAL_WARNINGS)
 endef
 
+define legal-error-pkg # pkg, text
+	echo "ERROR: $(1): $(2)" && false
+endef
+
 define legal-license-nosource # pkg, {local|override}
 	$(call legal-warning-pkg,$(1),sources not saved ($(2) packages not handled))
 endef
@@ -119,9 +123,13 @@ define legal-license-nofiles # pkg, {HOST|TARGET}
 endef
 
 define legal-license-file # pkg, filename, file-fullpath, {HOST|TARGET}
-	$(call legal-license-header,$(1),$(2) file,$(4)) && \
-	cat $(3) >>$(LEGAL_LICENSES_TXT_$(4)) && \
-	echo >>$(LEGAL_LICENSES_TXT_$(4)) && \
-	mkdir -p $(LICENSE_FILES_DIR_$(4))/$(1)/$(dir $(2)) && \
-	cp $(3) $(LICENSE_FILES_DIR_$(4))/$(1)/$(2)
+	if [ -e $(3) ]; then \
+		$(call legal-license-header,$(1),$(2) file,$(4)) && \
+		cat $(3) >>$(LEGAL_LICENSES_TXT_$(4)) && \
+		echo >>$(LEGAL_LICENSES_TXT_$(4)) && \
+		mkdir -p $(LICENSE_FILES_DIR_$(4))/$(1)/$(dir $(2)) && \
+		cp $(3) $(LICENSE_FILES_DIR_$(4))/$(1)/$(2); \
+	else \
+		$(call legal-error-pkg,$(1),license file $(2) not found); \
+	fi
 endef
-- 
1.8.3.2

[Buildroot] [PATCH 0/5 v5] legal-info: extract packages to get license files (branch yem/legal)

[Yann E. MORIN]

All,

On 2014-06-14 17:10 +0200, Yann E. MORIN spake thusly:
> Here is a series that fixes the legal-info infra:

Forgt this series for now, it causes a hell of a mess to rebase ontop
of the recent $$ changes. I'm fixing locally, and will wait for more
comments before respinning.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/5] legal-info: rename legal-warning-pkg-savednothing helper

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> This helper was called when none of the sources or license
> files were saved.
>
> Now we handle license files separately from the sources,
> this is no longer the case: they are only called when the
> sources are not saved.
>
> Rename the handler and change the warning message accordingly.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
> ---
>   package/pkg-generic.mk | 4 ++--
>   package/pkg-utils.mk   | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index d16908a..8a32f9d 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -640,10 +640,10 @@ endif # license files
>
>   ifeq ($$($(2)_SITE_METHOD),local)
>   # Packages without a tarball: don't save and warn
> -	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
> +	@$(call legal-license-nosource,$$($(2)_RAWNAME),local)
>
>   else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
> -	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
> +	@$(call legal-license-nosource,$$($(2)_RAWNAME),override)
>
>   else
>   # Other packages
> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> index 163ef8a..471f3ed 100644
> --- a/package/pkg-utils.mk
> +++ b/package/pkg-utils.mk
> @@ -101,8 +101,8 @@ define legal-warning-pkg # pkg, text
>   	echo "WARNING: $(1): $(2)" >>$(LEGAL_WARNINGS)
>   endef
>
> -define legal-warning-pkg-savednothing # pkg, {local|override}
> -	$(call legal-warning-pkg,$(1),sources and license files not saved ($(2) packages not handled))
> +define legal-license-nosource # pkg, {local|override}

I'm OK with the idea, but I not with the name.
Why "license"? This has to do with the source tarball.
Also, I would keep "warning" in the name, as it's what it is.

So, I'd rename it to legal-warning-nosource.

-- 
Luca

[Buildroot] [PATCH 0/5 v5] legal-info: extract packages to get license files (branch yem/legal)

[Thomas De Schampheleire]

"Yann E. MORIN" <yann.morin.1998@free.fr> schreef:
>All,
>
>On 2014-06-14 17:10 +0200, Yann E. MORIN spake thusly:
>> Here is a series that fixes the legal-info infra:
>
>Forgt this series for now, it causes a hell of a mess to rebase ontop
>of the recent $$ changes. 

You wanted dollars, now you'll have to deal with them!

;-)

[Buildroot] [PATCH 1/5] legal-info: extract even no-redistribute packages

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Currently, if a package is marked _REDISTRIBUTE = NO, then legal-info
> will not try to extract it first.
>
> If that package also declares some _LICENSE_FILES, legal-info fails
> if it is the only action we're trying to run:
>
>      $ cat defconfig
>      BR2_INIT_NONE=y
>      BR2_PACKAGE_LIBFSLCODEC=y
>      $ make BR2_DEFCONFIG=$(pwd)/defconfig defconfig
>      $ make libfslcodec-legal-info
>      /bin/sh: /home/ymorin/dev/buildroot/O/legal-info/licenses.txt: No such file or directory
>      make[1]: *** [libfslcodec-legal-info] Error 1

Note that the present patchset does not solve _this_ error.

The error that your patchset _does_ solve is:

   $ make BR2_DL_DIR=~/src legal-info-prepare libfslcodec-legal-info
   >>>   Collecting legal info
   cat: 
/home/murray/devel/buildroot-test/output/build/libfslcodec-3.5.7-1.0.0/EULA: 
No such file or directory
   make: *** [libfslcodec-legal-info] Error 1
   $


The error that you reported due to the fact that output/legal-info has
not been created yet.

Workaround:
$ make BR2_DL_DIR=~/src legal-info-prepare libfslcodec-legal-info
                         ^^^^^^^^^^^^^^^^^^

This is not related to your patches, and it's due to the fact that
the legal-info stuff is meant to be executed only from its top-level
target:
   $ make legal-info
which in turn runs:
  - legal-info-clean, which removes the entire legal-info subdir;
  - legal-info-prepare, which creates dirs and prepares skeleton files
    and the manifest heading;
  - <pkg>-legal-info, which fills the skeleton files.

Maybe this could be fixed by making all $(1)-legal-info targets depend
on legal-info-clean and then legal-info-prepare.
If it works it would be a nice improvement, although mostly visible
when testing the legal infra. For real cases I don't see a big point in
running anything else than `make legal-info`.

>
> Fix this by always having legal-info extract the archives if one or
> more _LICENSE_FILES are specified.
>
> We do this for all types of packages: overriden, local or 'normal'

s/overriden/overridden/

> remote packages. Even though we do not save the sources for the
> overriden or local packages, we need to save their licensing info,

Ditto.

> so we need to extract them.
>
> This implies that we now need to explicitly add PKG-source as a dependency
> of legal-info for packages we want to save (ie. redistributable, non-local
> and non-overriden packages).

Said this way, it looks like we're adding a dependency. Instead we are
changing the dependence from PKG-extract to PKG-source, which is one
step less (-extract implies -source), so bottom line we are removing a
dependency.

Better explained IMO:
  This implies that we now need only PKG-source, not PKG-extract anymore,
  as a dependency of legal-info for packages we want to save (.....).

>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
>
> ---
> Changes v4 -> v5:
>    - my usual s/pacakge/package/
>
> Changes v3 -> v4:
>    - legal-info needs to depend on PKG-source when it needs to save a
>      package's tarball
>
> Changes v2 -> v3:
>    - don't include source URL of no-redistribute, or overriden, or local
>      packages in the manifest
>
> Changes v1 -> v2:
>    - this is not fixing the autobuilders failure it was written to fix,
>      so remove the references to such build failures  (Thomas P)
>    - also extract overriden and local packages  (Fabio)
> ---
>   package/pkg-generic.mk | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index ccd7f3b..eb3ec9f 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -595,11 +595,18 @@ $(2)_MANIFEST_LICENSE_FILES = $$($(2)_LICENSE_FILES)
>   endif
>   $(2)_MANIFEST_LICENSE_FILES ?= not saved
>
> +# If the package declares _LICENSE_FILES, we need to extract it,
> +# for overriden, local or normal remote packages alike, whether

Ditto.

> +# we want to redistribute it or not.
> +ifneq ($$($(2)_LICENSE_FILES),)
> +$(1)-legal-info: $(1)-extract
> +endif
> +
>   ifeq ($$($(2)_REDISTRIBUTE),YES)
>   ifneq ($$($(2)_SITE_METHOD),local)
>   ifneq ($$($(2)_SITE_METHOD),override)
> -# Packages that have a tarball need it downloaded and extracted beforehand
> -$(1)-legal-info: $(1)-extract $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4)))
> +# We need to download the package archive if we are to save it
> +$(1)-legal-info: $(1)-source $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4)))
>   $(2)_MANIFEST_TARBALL = $$($(2)_SOURCE)
>   endif
>   endif
>

With the above fixed, and once rebased on top of master, and since I'm
OK with all of the very few code lines you touched:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

[Quick test on top of e00c631ef4aa, will test again once rebased]
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

[Buildroot] [PATCH 2/5] legal-info: save license files even for local or overriden packages

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> Even if we do not save the sources for local or overrident packages because

s/overrident/overridden/

> it is too complex, we can still quite easily save the license files.
>
> Also, having the license files is a very important part of complying with
> the licenses.
>
> Move the copy of license files out of the non-local, non-overriden package

s/overriden/overridden/

> case, but still in the case where packages have a _SOURCE defined, to
> avoid catching packages bundled in Buildroot (eg. mkpasswd et al.)
>
> Reported-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
>
> ---
> Changes v4 -> v5:
>    - avoid warning for packages bundled in Buildroot  (Luca)
>    - the change is not a bout non-redistributable packages, but about
>      local and overriden packages
> ---
>   package/pkg-generic.mk | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index eb3ec9f..a978e63 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -619,17 +619,12 @@ $(1)-legal-info:
>   	$(foreach hook,$($(2)_PRE_LEGAL_INFO_HOOKS),$(call $(hook))$(sep))
>   ifneq ($(call qstrip,$$($(2)_SOURCE)),)
>
> -ifeq ($$($(2)_SITE_METHOD),local)
> -# Packages without a tarball: don't save and warn
> -	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
> -
> -else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
> -	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
> -
> -else
> -# Other packages
> -
>   # Save license files if defined
> +# We save the license files for any kind of package: normal, local,
> +# overriden, or non-redistributable alike.

s/overriden/overridden/

> +# The reason to save license files even for no-redistribute packages
> +# is that the license still applies to the files distributed as part
> +# of the rootfs, even if the sources are not themselves redistributed.
>   ifeq ($(call qstrip,$$($(2)_LICENSE_FILES)),)
>   	@$(call legal-license-nofiles,$$($(2)_RAWNAME),$(call UPPERCASE,$(4)))
>   	@$(call legal-warning-pkg,$$($(2)_RAWNAME),cannot save license ($(2)_LICENSE_FILES not defined))
> @@ -640,6 +635,16 @@ else
>   	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$(F),$$($(2)_DIR)/$$(F),$(call UPPERCASE,$(4)))$$(sep))
>   endif # license files
>
> +ifeq ($$($(2)_SITE_METHOD),local)
> +# Packages without a tarball: don't save and warn
> +	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
> +
> +else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
> +	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
> +
> +else
> +# Other packages
> +
>   ifeq ($$($(2)_REDISTRIBUTE),YES)
>   # Copy the source tarball (just hardlink if possible)
>   	@cp -l $(DL_DIR)/$$($(2)_SOURCE) $(REDIST_SOURCES_DIR_$(call UPPERCASE,$(4))) 2>/dev/null || \
>

With the above fixed, and once rebased on top of master, and since I'm
OK with the actual code changes:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

[Quick test on top of e00c631ef4aa, will test again once rebased]
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

[Buildroot] [PATCH 3/5] legal-info: add a comment about what packages we save the tarballs of

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> As the legal-info infra only (rightfully) saves the tarballs of packages
> that:
>    - we want to redistribute,
>    - and are not local,
>    - and are not overriden,
>
> add a comment stating so.
>
> This should clarify the code-block, which although trivial to read,
> was not easy to interpret without thinking thouroughly about it.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
> ---
>   package/pkg-generic.mk | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a978e63..d16908a 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -602,6 +602,9 @@ ifneq ($$($(2)_LICENSE_FILES),)
>   $(1)-legal-info: $(1)-extract
>   endif
>
> +# We only save the sources of packages we want to redistribute, that are
> +# non-local, and non-overriden. So only store, in the manifest, the tarball
> +# name of those packages.
>   ifeq ($$($(2)_REDISTRIBUTE),YES)
>   ifneq ($$($(2)_SITE_METHOD),local)
>   ifneq ($$($(2)_SITE_METHOD),override)
>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

[Buildroot] [PATCH 5/5] legal-info: properly error out on missing license file

[Luca Ceresoli]

Dear Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> When saving the license files, if a declared license file is missing,
> print a proper error message, instead of the sometime-criptic error
> message from 'cp'.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
>
> ---
> Note: when I wrote this back in March 2014, I had a concrete example of
> a criptic error message (the name of the license did not make it obvious
> we were saving a license file?), but today I can't remember what it was
> exactly.
>
> Since I believe showing a properly-formatted error message to the user
> is anyway a good thing, I kept that patch. Feel free to ditch it if you
> don't like it.

I may be missing something, but I wonder what cryptic error you had
seen. I tried removing a license file and got, before your patch:

   cat: 
/home/murray/devel/buildroot-test/output/build/busybox-1.22.1/LICENSE: 
No such file or directory

Except for the "cat: " prefix, it's quite understandable IMHO.

With your patch applied:

   ERROR: busybox: license file LICENSE not found

This is much more concise, but gives a less info about where the
LICENSE file should be found. To the inexperienced user, the path might
be a useful hint.
OTOH, the "ERROR: busybox: " prefix is very informative.

How about changing your message to:
   ERROR: busybox: license file LICENSE not found in /.../busybox-1.22.1/
or
   ERROR: busybox: license file /.../busybox-1.22.1/LICENSE not found

-- 
Luca

[Buildroot] [PATCH 1/5] legal-info: extract even no-redistribute packages

[Yann E. MORIN]

Luca, All,

On 2014-06-18 23:17 +0200, Luca Ceresoli spake thusly:
> Dear Yann,
> 
> Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> >Currently, if a package is marked _REDISTRIBUTE = NO, then legal-info
> >will not try to extract it first.
> >
> >If that package also declares some _LICENSE_FILES, legal-info fails
> >if it is the only action we're trying to run:
> >
> >     $ cat defconfig
> >     BR2_INIT_NONE=y
> >     BR2_PACKAGE_LIBFSLCODEC=y
> >     $ make BR2_DEFCONFIG=$(pwd)/defconfig defconfig
> >     $ make libfslcodec-legal-info
> >     /bin/sh: /home/ymorin/dev/buildroot/O/legal-info/licenses.txt: No such file or directory
> >     make[1]: *** [libfslcodec-legal-info] Error 1
> 
> Note that the present patchset does not solve _this_ error.
> 
> The error that your patchset _does_ solve is:
> 
>   $ make BR2_DL_DIR=~/src legal-info-prepare libfslcodec-legal-info
>   >>>   Collecting legal info
>   cat:
> /home/murray/devel/buildroot-test/output/build/libfslcodec-3.5.7-1.0.0/EULA:
> No such file or directory
>   make: *** [libfslcodec-legal-info] Error 1

Well, that's not even the error I'm trying to solve. In fact, I'm trying
to solve this:

    $ cat defconfig
    BR2_arm=y
    BR2_TOOLCHAIN_BUILDROOT_EGLIBC=y
    BR2_PACKAGE_LIBFSLCODEC=y
    $ make BR2_DEFCONFIG=$(pwd)/defconfig defconfig
    $ make legal-info
    [--SNIP--]
    cat: /home/ymorin/dev/buildroot/O/build/libfslcodec-3.5.7-1.0.0/EULA: No
    such file or directory

> The error that you reported due to the fact that output/legal-info has
> not been created yet.

Indeed. I was a bit too fast at writing the commit log (it happens quite
often these days, I must be moire careful.) Sorry.

> For real cases I don't see a big point in
> running anything else than `make legal-info`.

So do I.

[--SNIP--]
> >This implies that we now need to explicitly add PKG-source as a dependency
> >of legal-info for packages we want to save (ie. redistributable, non-local
> >and non-overriden packages).
> 
> Said this way, it looks like we're adding a dependency. Instead we are
> changing the dependence from PKG-extract to PKG-source, which is one
> step less (-extract implies -source), so bottom line we are removing a
> dependency.
> 
> Better explained IMO:
>  This implies that we now need only PKG-source, not PKG-extract anymore,
>  as a dependency of legal-info for packages we want to save (.....).

OK, will rephrase.

[--SNIP--]
> >+# If the package declares _LICENSE_FILES, we need to extract it,
> >+# for overriden, local or normal remote packages alike, whether
> 
> Ditto.

Will fix that and the others.

[--SNIP--]
> With the above fixed, and once rebased on top of master, and since I'm
> OK with all of the very few code lines you touched:
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> [Quick test on top of e00c631ef4aa, will test again once rebased]
> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

OK, thank you! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 4/5] legal-info: rename legal-warning-pkg-savednothing helper

[Yann E. MORIN]

Luca, All,

On 2014-06-18 17:32 +0200, Luca Ceresoli spake thusly:
> Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> >This helper was called when none of the sources or license
> >files were saved.
> >
> >Now we handle license files separately from the sources,
> >this is no longer the case: they are only called when the
> >sources are not saved.
> >
> >Rename the handler and change the warning message accordingly.
> >
> >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >Cc: Luca Ceresoli <luca@lucaceresoli.net>
> >Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> >Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
> >---
> >  package/pkg-generic.mk | 4 ++--
> >  package/pkg-utils.mk   | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> >diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> >index d16908a..8a32f9d 100644
> >--- a/package/pkg-generic.mk
> >+++ b/package/pkg-generic.mk
> >@@ -640,10 +640,10 @@ endif # license files
> >
> >  ifeq ($$($(2)_SITE_METHOD),local)
> >  # Packages without a tarball: don't save and warn
> >-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),local)
> >+	@$(call legal-license-nosource,$$($(2)_RAWNAME),local)
> >
> >  else ifneq ($$($(2)_OVERRIDE_SRCDIR),)
> >-	@$(call legal-warning-pkg-savednothing,$$($(2)_RAWNAME),override)
> >+	@$(call legal-license-nosource,$$($(2)_RAWNAME),override)
> >
> >  else
> >  # Other packages
> >diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> >index 163ef8a..471f3ed 100644
> >--- a/package/pkg-utils.mk
> >+++ b/package/pkg-utils.mk
> >@@ -101,8 +101,8 @@ define legal-warning-pkg # pkg, text
> >  	echo "WARNING: $(1): $(2)" >>$(LEGAL_WARNINGS)
> >  endef
> >
> >-define legal-warning-pkg-savednothing # pkg, {local|override}
> >-	$(call legal-warning-pkg,$(1),sources and license files not saved ($(2) packages not handled))
> >+define legal-license-nosource # pkg, {local|override}
> 
> I'm OK with the idea, but I not with the name.
> Why "license"? This has to do with the source tarball.
> Also, I would keep "warning" in the name, as it's what it is.
> 
> So, I'd rename it to legal-warning-nosource.

Indeed. What was on my head when I did that?... :-/

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 5/5] legal-info: properly error out on missing license file

[Yann E. MORIN]

Luca, All,

On 2014-06-18 23:38 +0200, Luca Ceresoli spake thusly:
> Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> >When saving the license files, if a declared license file is missing,
> >print a proper error message, instead of the sometime-criptic error
> >message from 'cp'.
> >
> >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >Cc: Luca Ceresoli <luca@lucaceresoli.net>
> >Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> >Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >Cc: Fabio Porcedda <fabio.porcedda@gmail.com>
> >
> >---
> >Note: when I wrote this back in March 2014, I had a concrete example of
> >a criptic error message (the name of the license did not make it obvious
> >we were saving a license file?), but today I can't remember what it was
> >exactly.
> >
> >Since I believe showing a properly-formatted error message to the user
> >is anyway a good thing, I kept that patch. Feel free to ditch it if you
> >don't like it.
> 
> I may be missing something, but I wonder what cryptic error you had
> seen.

Well, now I tried again, that's what I observed:

    $ make legal-info
    [--SNIP--]
    >>> busybox 1.22.1 Extracting
    bzcat /home/ymorin/src/busybox-1.22.1.tar.bz2 | tar --strip-components=1
    -C /home/ymorin/dev/buildroot/O/build/busybox-1.22.1  -xf -
    cat: /home/ymorin/dev/buildroot/O/build/libfslcodec-3.5.7-1.0.0/EULA: No
    such file or directory
    make[1]: *** [libfslcodec-legal-info] Error 1

So, OK, the error is explicitly about fslcodec. But remember that the
line  ">>> busybox 1.22.1 Extracting"  is highlighted, and thus what
stand out is the 'busybox' package, and it took me a while to understand
why on Earth busybox was trying to save an EULA file, when moments later
I saw that it was in fact in the libfslcodec package...

But since this series is expressly about this error case (no-redistribute
packages failed to save the license files), the initial error should be
handled.

I'll redo my tsting here to see if that's still a concern now...

> With your patch applied:
> 
>   ERROR: busybox: license file LICENSE not found
> 
> This is much more concise, but gives a less info about where the
> LICENSE file should be found. To the inexperienced user, the path might
> be a useful hint.
> OTOH, the "ERROR: busybox: " prefix is very informative.
> 
> How about changing your message to:
>   ERROR: busybox: license file LICENSE not found in /.../busybox-1.22.1/
> or
>   ERROR: busybox: license file /.../busybox-1.22.1/LICENSE not found

Yes, we can use the full path to the missing license file, indeed.

Thanks! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'