[Buildroot] [PATCH 0/3] core: check hashes of license files

[Buildroot] [PATCH 0/3] core: check hashes of license files

[Yann E. MORIN]

Hello All!

This small series is a proposal to check the hashes of the license files
during legal-info, to catch the packages whose license changes but where
the text of the new license is in the same file.

This was suggested by Rahul recently:
    http://lists.busybox.net/pipermail/buildroot/2017-June/194425.html


Regards,
Yann E. MORIN.


The following changes since commit 859764ac39c18c6aaabbb6a1a47f2fa2e5793044

  linux-headers: bump 4.{1, 4, 9, 11}.x series (2017-06-17 16:17:04 +0200)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to 41013c99543173da1ec547219c7f67e52d323a40

  docs/manual: document hashes for license files (2017-06-18 09:59:06 +0200)


----------------------------------------------------------------
Yann E. MORIN (3):
      core/pkg-util: pass package directory and name when saving license files
      core/pkg-utils: check hashes of license files
      docs/manual: document hashes for license files

 Makefile                                  |  2 +-
 docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
 package/pkg-generic.mk                    |  2 +-
 package/pkg-utils.mk                      | 11 ++++++++---
 4 files changed, 24 insertions(+), 7 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/3] core/pkg-util: pass package directory and name when saving license files

[Yann E. MORIN]

This will be usefull when checking the hashes of the license files.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
---
 Makefile               | 2 +-
 package/pkg-generic.mk | 2 +-
 package/pkg-utils.mk   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 88d98e0405..1c272d009c 100644
--- a/Makefile
+++ b/Makefile
@@ -740,7 +740,7 @@ legal-info-clean:
 .PHONY: legal-info-prepare
 legal-info-prepare: $(LEGAL_INFO_DIR)
 	@$(call MESSAGE,"Collecting legal info")
-	@$(call legal-license-file,buildroot,COPYING,COPYING,HOST)
+	@$(call legal-license-file,buildroot,buildroot,,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)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f474704980..8a1dc310d3 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -849,7 +849,7 @@ ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
 ifeq ($$(call qstrip,$$($(2)_LICENSE_FILES)),)
 	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))
 else
-	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAW_BASE_NAME),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
+	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_NAME),$$($(2)_RAW_BASE_NAME),$$($(2)_PKGDIR),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)
diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index c95e77953b..e9ac56276f 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -83,7 +83,7 @@ define legal-manifest # pkg, version, license, license-files, source, url, {HOST
 	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)"' >>$(LEGAL_MANIFEST_CSV_$(7))
 endef
 
-define legal-license-file # pkg, filename, file-fullpath, {HOST|TARGET}
-	mkdir -p $(LICENSE_FILES_DIR_$(4))/$(1)/$(dir $(2)) && \
-	cp $(3) $(LICENSE_FILES_DIR_$(4))/$(1)/$(2)
+define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
+	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
+	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
-- 
2.11.0

[Buildroot] [PATCH 2/3] core/pkg-utils: check hashes of license files

[Yann E. MORIN]

This will help catch a change of license even if the filename does
not change.

For now, a missing hash for the license files is not a fatal error, to
let people catch up and add them. When we switch to make it mandatory,
we can simplify the code by just removing the case statement.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
---
 package/pkg-utils.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index e9ac56276f..accf48c464 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -85,5 +85,10 @@ endef
 
 define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
 	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
+	{ \
+		support/download/check-hash $(3)/$(1).hash $(5) $(4); \
+		ret=$${?}; \
+		case $${ret} in (0|3) ;; (*) exit 1;; esac; \
+	} && \
 	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
-- 
2.11.0

[Buildroot] [PATCH 3/3] docs/manual: document hashes for license files

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
---
 docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
index 08f5d42f91..5007d5368d 100644
--- a/docs/manual/adding-packages-directory.txt
+++ b/docs/manual/adding-packages-directory.txt
@@ -443,7 +443,7 @@ Optionally, you can add a third file, named +libfoo.hash+, that contains
 the hashes of the downloaded files for the +libfoo+ package.
 
 The hashes stored in that file are used to validate the integrity of the
-downloaded files.
+downloaded files and of the license files.
 
 The format of this file is one line for each file for which to check the
 hash, each line being space-separated, with these three fields:
@@ -458,7 +458,10 @@ hash, each line being space-separated, with these three fields:
 ** for +sha256+, 64 hexadecimal characters
 ** for +sha384+, 96 hexadecimal characters
 ** for +sha512+, 128 hexadecimal characters
-* the name of the file, without any directory component
+* the name of the file:
+** for a source archive: the basename of the file, without any directory
+   component,
+** for a license file: the path as it appears in +FOO_LICENSE_FILES+.
 
 Lines starting with a +#+ sign are considered comments, and ignored. Empty
 lines are ignored.
@@ -476,6 +479,11 @@ strong hash yourself (preferably +sha256+, but not +md5+), and mention
 this in a comment line above the hashes.
 
 .Note
+The hashes for license files are used to detect a license change when a
+package version is bumped, so a (relatively) weak hash like +sha1+ is
+enough for license files.
+
+.Note
 The number of spaces does not matter, so one can use spaces (or tabs) to
 properly align the different fields.
 
@@ -501,6 +509,10 @@ sha256 ff52101fb90bbfc3fe9475e425688c660f46216d7e751c4bbdb1dc85cdccacb9 libfoo-f
 
 # No hash for 1234:
 none   xxx                                                              libfoo-1234.tar.gz
+
+# Hash for license files:
+sha1  c47a888f2be626e1197b6f651dee966ef882077d  COPYING
+sha1  e101765734390d664b59325b2d644d80d9a6bd9a  doc/COPYING.LGPL
 ----
 
 If the +.hash+ file is present, and it contains one or more hashes for a
-- 
2.11.0

[Buildroot] [PATCH 0/3] core: check hashes of license files

[Rahul Bedarkar]

Hello Yann,

On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Hello All!
>
> This small series is a proposal to check the hashes of the license files
> during legal-info, to catch the packages whose license changes but where
> the text of the new license is in the same file.

Thanks for this series. Checking hashes of the license files during
legal-info stage looks logical but we discussed about doing that after
downloading sources so that change in license file is noticed early
(as a part of build test after version bump).

Regards,
Rahul

>
> This was suggested by Rahul recently:
>     http://lists.busybox.net/pipermail/buildroot/2017-June/194425.html
>
>
> Regards,
> Yann E. MORIN.
>
>
> The following changes since commit 859764ac39c18c6aaabbb6a1a47f2fa2e5793044
>
>   linux-headers: bump 4.{1, 4, 9, 11}.x series (2017-06-17 16:17:04 +0200)
>
>
> are available in the git repository at:
>
>   git://git.buildroot.org/~ymorin/git/buildroot.git
>
> for you to fetch changes up to 41013c99543173da1ec547219c7f67e52d323a40
>
>   docs/manual: document hashes for license files (2017-06-18 09:59:06 +0200)
>
>
> ----------------------------------------------------------------
> Yann E. MORIN (3):
>       core/pkg-util: pass package directory and name when saving license files
>       core/pkg-utils: check hashes of license files
>       docs/manual: document hashes for license files
>
>  Makefile                                  |  2 +-
>  docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
>  package/pkg-generic.mk                    |  2 +-
>  package/pkg-utils.mk                      | 11 ++++++++---
>  4 files changed, 24 insertions(+), 7 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 0/3] core: check hashes of license files

[Yann E. MORIN]

Rahul, All,

On 2017-06-19 22:47 +0530, Rahul Bedarkar spake thusly:
> On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >
> > Hello All!
> >
> > This small series is a proposal to check the hashes of the license files
> > during legal-info, to catch the packages whose license changes but where
> > the text of the new license is in the same file.
> 
> Thanks for this series. Checking hashes of the license files during
> legal-info stage looks logical but we discussed about doing that after
> downloading sources so that change in license file is noticed early
> (as a part of build test after version bump).

It is not possible to do at download time. It can only be done after
the package has been extracted and patched.

That is why, when you run legal-info on a non-built (but configured)
tree, you'll notice that Buildroot extracts and patches the packages
before saving their legal-info.

Besides, if one uses the support/scripts/test-pkg script to test the
version bump, then legal-info is run by the script.

So, I still believe it is better done during legal-info.

Regards,
Yann E. MORIN.

> Regards,
> Rahul
> 
> >
> > This was suggested by Rahul recently:
> >     http://lists.busybox.net/pipermail/buildroot/2017-June/194425.html
> >
> >
> > Regards,
> > Yann E. MORIN.
> >
> >
> > The following changes since commit 859764ac39c18c6aaabbb6a1a47f2fa2e5793044
> >
> >   linux-headers: bump 4.{1, 4, 9, 11}.x series (2017-06-17 16:17:04 +0200)
> >
> >
> > are available in the git repository at:
> >
> >   git://git.buildroot.org/~ymorin/git/buildroot.git
> >
> > for you to fetch changes up to 41013c99543173da1ec547219c7f67e52d323a40
> >
> >   docs/manual: document hashes for license files (2017-06-18 09:59:06 +0200)
> >
> >
> > ----------------------------------------------------------------
> > Yann E. MORIN (3):
> >       core/pkg-util: pass package directory and name when saving license files
> >       core/pkg-utils: check hashes of license files
> >       docs/manual: document hashes for license files
> >
> >  Makefile                                  |  2 +-
> >  docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
> >  package/pkg-generic.mk                    |  2 +-
> >  package/pkg-utils.mk                      | 11 ++++++++---
> >  4 files changed, 24 insertions(+), 7 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.  |
> > '------------------------------^-------^------------------^--------------------'

-- 
.-----------------.--------------------.------------------.--------------------.
|  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 0/3] core: check hashes of license files

[Thomas De Schampheleire]

2017-06-19 19:47 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>:
> Rahul, All,
>
> On 2017-06-19 22:47 +0530, Rahul Bedarkar spake thusly:
>> On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>> >
>> > Hello All!
>> >
>> > This small series is a proposal to check the hashes of the license files
>> > during legal-info, to catch the packages whose license changes but where
>> > the text of the new license is in the same file.
>>
>> Thanks for this series. Checking hashes of the license files during
>> legal-info stage looks logical but we discussed about doing that after
>> downloading sources so that change in license file is noticed early
>> (as a part of build test after version bump).
>
> It is not possible to do at download time. It can only be done after
> the package has been extracted and patched.
>
> That is why, when you run legal-info on a non-built (but configured)
> tree, you'll notice that Buildroot extracts and patches the packages
> before saving their legal-info.
>
> Besides, if one uses the support/scripts/test-pkg script to test the
> version bump, then legal-info is run by the script.
>
> So, I still believe it is better done during legal-info.
>

Yann, I think Rahul means that the checking of the hashing should be
checked as part of the standard 'make pkg' target, whichever subtarget
it is, be it -build, -install or what not.

But, I don't think we should mix such topics: legal info topics should
stay in the -legal-info target.
One solution could be to make '-legal-info' part of the standard build
process, although it will slow down the build and some/many people
will not like that.
An alternative is to split '-legal-info' in two parts:
-legal-info-checks and actual -legal-info. The first part would verify
some important things, i.e. presence of valid LICENSE, presence of all
files specified in LICENSE_FILES, hash checking on these files. It
could be added to the standard 'make pkg' group. The second part would
do the actual creation of the manifest, copying the sources, etc. and
remains on-demand only.

I don't know what you think of that approach, I'm thinking out loud.

/Thomas

[Buildroot] [PATCH 0/3] core: check hashes of license files

[Rahul Bedarkar]

On Mon, Jun 19, 2017 at 11:17 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Rahul, All,
>
> It is not possible to do at download time. It can only be done after
> the package has been extracted and patched.

I mean after after download is complete and before we build package.

> That is why, when you run legal-info on a non-built (but configured)
> tree, you'll notice that Buildroot extracts and patches the packages
> before saving their legal-info.
>
> Besides, if one uses the support/scripts/test-pkg script to test the
> version bump, then legal-info is run by the script.
>
> So, I still believe it is better done during legal-info.

Yes, during legal-info looks more logical and as test-pkg script runs
legal-info that should be fine.

Thanks,
Rahul

[Buildroot] [PATCH 0/3] core: check hashes of license files

[Yann E. MORIN]

Thomas, All,

On 2017-06-19 21:32 +0200, Thomas De Schampheleire spake thusly:
> 2017-06-19 19:47 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>:
> > On 2017-06-19 22:47 +0530, Rahul Bedarkar spake thusly:
> >> On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >> >
> >> > Hello All!
> >> >
> >> > This small series is a proposal to check the hashes of the license files
> >> > during legal-info, to catch the packages whose license changes but where
> >> > the text of the new license is in the same file.
> >>
> >> Thanks for this series. Checking hashes of the license files during
> >> legal-info stage looks logical but we discussed about doing that after
> >> downloading sources so that change in license file is noticed early
> >> (as a part of build test after version bump).
> >
> > It is not possible to do at download time. It can only be done after
> > the package has been extracted and patched.
> >
> > That is why, when you run legal-info on a non-built (but configured)
> > tree, you'll notice that Buildroot extracts and patches the packages
> > before saving their legal-info.
> >
> > Besides, if one uses the support/scripts/test-pkg script to test the
> > version bump, then legal-info is run by the script.
> >
> > So, I still believe it is better done during legal-info.
> >
> 
> Yann, I think Rahul means that the checking of the hashing should be
> checked as part of the standard 'make pkg' target, whichever subtarget
> it is, be it -build, -install or what not.

OK, I see.

Still, I believe it is better suited to keep that for during the
legal-info step.

Regards,
Yann E. MORIN.

> But, I don't think we should mix such topics: legal info topics should
> stay in the -legal-info target.
> One solution could be to make '-legal-info' part of the standard build
> process, although it will slow down the build and some/many people
> will not like that.
> An alternative is to split '-legal-info' in two parts:
> -legal-info-checks and actual -legal-info. The first part would verify
> some important things, i.e. presence of valid LICENSE, presence of all
> files specified in LICENSE_FILES, hash checking on these files. It
> could be added to the standard 'make pkg' group. The second part would
> do the actual creation of the manifest, copying the sources, etc. and
> remains on-demand only.
> 
> I don't know what you think of that approach, I'm thinking out loud.
> 
> /Thomas

-- 
.-----------------.--------------------.------------------.--------------------.
|  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 3/3] docs/manual: document hashes for license files

[Ricardo Martincoski]

Hello,

On Sun, Jun 18, 2017 at 05:01 AM, Yann E. MORIN wrote:

> +# Hash for license files:
> +sha1  c47a888f2be626e1197b6f651dee966ef882077d  COPYING
> +sha1  e101765734390d664b59325b2d644d80d9a6bd9a  doc/COPYING.LGPL

This will need a small change in check-package to not generate false warnings
like this: "use filename without directory component".
See HashFilename in checkpackagelib/lib_hash.py

'make package-dirclean package-source' already generates 'ERROR: No hash found'
if someone mistakenly adds a directory, for example dl/, to the name of the
tarball in the hash file, so we are safe to just remove the entire HashFilename
class.

Regards,
Ricardo

[Buildroot] [PATCH 2/3] core/pkg-utils: check hashes of license files

[Luca Ceresoli]

Hi Yann,

On 18/06/2017 10:01, Yann E. MORIN wrote:
> This will help catch a change of license even if the filename does
> not change.
> 
> For now, a missing hash for the license files is not a fatal error, to
> let people catch up and add them. When we switch to make it mandatory,
> we can simplify the code by just removing the case statement.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> ---
>  package/pkg-utils.mk | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> index e9ac56276f..accf48c464 100644
> --- a/package/pkg-utils.mk
> +++ b/package/pkg-utils.mk
> @@ -85,5 +85,10 @@ endef
>  
>  define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
>  	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
> +	{ \
> +		support/download/check-hash $(3)/$(1).hash $(5) $(4); \
> +		ret=$${?}; \
> +		case $${ret} in (0|3) ;; (*) exit 1;; esac; \
> +	} && \

I generally agree with the idea, even more since the implementation is
really a few-liner. But I have a few remarks.

The first is one of personal coding style taste. I don't like very much
the weird check for values 0 and 3. It does not make any sense unless
one opens check-hash and read the possible return values, which look a
bit like a randomly-ordered enum.

I'd rather prefer if we could call check-hash with a command line flag
to ask that missing hashes generate a warning and return 0 instead of
the default behavior. This would mean check-hash always returns 0 for
"go" and non-zero for "abort".

The second remark is about the output of 'make legal-info'. With this
patch the output is:

$ make legal-info
>>>   Collecting legal info
WARNING: no hash file for COPYING
WARNING: no hash file for COPYING
WARNING: no hash file for COPYING3
WARNING: no hash file for COPYING.LIB
WARNING: no hash file for COPYING.LESSERv3
WARNING: no hash file for COPYINGv2
...

There's no mention of the package involved, which doesn't help
in fixing it. It should print the package name, e.g.:

$ make legal-info
>>>   Collecting legal info
WARNING: binutils: no hash file for COPYING
WARNING: mpc:      no hash file for COPYING
...

To achieve this I guess we'll need to pass the package name to
check-hash. It doesn't hurt if we add the package name unconditionally,
even to currently existing messages where it is not needed.

-- 
Luca

[Buildroot] [PATCH 0/3] core: check hashes of license files

[Luca Ceresoli]

On 20/06/2017 17:28, Yann E. MORIN wrote:
> Thomas, All,
> 
> On 2017-06-19 21:32 +0200, Thomas De Schampheleire spake thusly:
>> 2017-06-19 19:47 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>:
>>> On 2017-06-19 22:47 +0530, Rahul Bedarkar spake thusly:
>>>> On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>>>>>
>>>>> Hello All!
>>>>>
>>>>> This small series is a proposal to check the hashes of the license files
>>>>> during legal-info, to catch the packages whose license changes but where
>>>>> the text of the new license is in the same file.
>>>>
>>>> Thanks for this series. Checking hashes of the license files during
>>>> legal-info stage looks logical but we discussed about doing that after
>>>> downloading sources so that change in license file is noticed early
>>>> (as a part of build test after version bump).
>>>
>>> It is not possible to do at download time. It can only be done after
>>> the package has been extracted and patched.
>>>
>>> That is why, when you run legal-info on a non-built (but configured)
>>> tree, you'll notice that Buildroot extracts and patches the packages
>>> before saving their legal-info.
>>>
>>> Besides, if one uses the support/scripts/test-pkg script to test the
>>> version bump, then legal-info is run by the script.
>>>
>>> So, I still believe it is better done during legal-info.
>>>
>>
>> Yann, I think Rahul means that the checking of the hashing should be
>> checked as part of the standard 'make pkg' target, whichever subtarget
>> it is, be it -build, -install or what not.
> 
> OK, I see.
> 
> Still, I believe it is better suited to keep that for during the
> legal-info step.
> 
> Regards,
> Yann E. MORIN.
> 
>> But, I don't think we should mix such topics: legal info topics should
>> stay in the -legal-info target.
>> One solution could be to make '-legal-info' part of the standard build
>> process, although it will slow down the build and some/many people
>> will not like that.
>> An alternative is to split '-legal-info' in two parts:
>> -legal-info-checks and actual -legal-info. The first part would verify
>> some important things, i.e. presence of valid LICENSE, presence of all
>> files specified in LICENSE_FILES, hash checking on these files. It
>> could be added to the standard 'make pkg' group. The second part would
>> do the actual creation of the manifest, copying the sources, etc. and
>> remains on-demand only.

Thomas' analysis is technically correct, but implementing what he
describes it is a bit complex for the benefits it grants IMO. So I agree
with Yann to keep it simple and require patch submitters to use test-pkg
before submitting patches. And of course we can change this choice in
the future.

-- 
Luca

[Buildroot] [PATCH 3/3] docs/manual: document hashes for license files

[Luca Ceresoli]

Hi Yann,

On 18/06/2017 10:01, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> ---
>  docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
> index 08f5d42f91..5007d5368d 100644
> --- a/docs/manual/adding-packages-directory.txt
> +++ b/docs/manual/adding-packages-directory.txt
> @@ -443,7 +443,7 @@ Optionally, you can add a third file, named +libfoo.hash+, that contains
>  the hashes of the downloaded files for the +libfoo+ package.
>  
>  The hashes stored in that file are used to validate the integrity of the
> -downloaded files.
> +downloaded files and of the license files.
>  
>  The format of this file is one line for each file for which to check the
>  hash, each line being space-separated, with these three fields:
> @@ -458,7 +458,10 @@ hash, each line being space-separated, with these three fields:
>  ** for +sha256+, 64 hexadecimal characters
>  ** for +sha384+, 96 hexadecimal characters
>  ** for +sha512+, 128 hexadecimal characters
> -* the name of the file, without any directory component
> +* the name of the file:
> +** for a source archive: the basename of the file, without any directory
> +   component,
> +** for a license file: the path as it appears in +FOO_LICENSE_FILES+.
>  
>  Lines starting with a +#+ sign are considered comments, and ignored. Empty
>  lines are ignored.
> @@ -476,6 +479,11 @@ strong hash yourself (preferably +sha256+, but not +md5+), and mention
>  this in a comment line above the hashes.
>  
>  .Note
> +The hashes for license files are used to detect a license change when a
> +package version is bumped, so a (relatively) weak hash like +sha1+ is
> +enough for license files.

I wouldn't spend words to say people can use a weak hash in this case.
What's the advantage if they use weak hashes? Not computational time,
hash files are usually small. Not ease of use: typing 'sha256sum' is not
more difficult than 'md5sum'.

So I'd just drop that note.

-- 
Luca

[Buildroot] [PATCH 3/3] docs/manual: document hashes for license files

[Yann E. MORIN]

Luca, All,

On 2017-06-23 23:57 +0200, Luca Ceresoli spake thusly:
> 
> On 18/06/2017 10:01, Yann E. MORIN wrote:
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> > Cc: Luca Ceresoli <luca@lucaceresoli.net>
> > Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
> > Cc: Peter Korsgaard <peter@korsgaard.com>
> > ---
> >  docs/manual/adding-packages-directory.txt | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
> > index 08f5d42f91..5007d5368d 100644
> > --- a/docs/manual/adding-packages-directory.txt
> > +++ b/docs/manual/adding-packages-directory.txt
[--SNIP--]
> > @@ -476,6 +479,11 @@ strong hash yourself (preferably +sha256+, but not +md5+), and mention
> >  this in a comment line above the hashes.
> >  
> >  .Note
> > +The hashes for license files are used to detect a license change when a
> > +package version is bumped, so a (relatively) weak hash like +sha1+ is
> > +enough for license files.
> 
> I wouldn't spend words to say people can use a weak hash in this case.
> What's the advantage if they use weak hashes? Not computational time,
> hash files are usually small. Not ease of use: typing 'sha256sum' is not
> more difficult than 'md5sum'.
> 
> So I'd just drop that note.

OK, done. 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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 2/3] core/pkg-utils: check hashes of license files

[Yann E. MORIN]

Luca, All,

On 2017-06-23 23:49 +0200, Luca Ceresoli spake thusly:
> On 18/06/2017 10:01, Yann E. MORIN wrote:
> > This will help catch a change of license even if the filename does
> > not change.
> > 
> > For now, a missing hash for the license files is not a fatal error, to
> > let people catch up and add them. When we switch to make it mandatory,
> > we can simplify the code by just removing the case statement.
> > 
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Luca Ceresoli <luca@lucaceresoli.net>
> > Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
> > Cc: Peter Korsgaard <peter@korsgaard.com>
> > ---
> >  package/pkg-utils.mk | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> > index e9ac56276f..accf48c464 100644
> > --- a/package/pkg-utils.mk
> > +++ b/package/pkg-utils.mk
> > @@ -85,5 +85,10 @@ endef
> >  
> >  define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
> >  	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
> > +	{ \
> > +		support/download/check-hash $(3)/$(1).hash $(5) $(4); \
> > +		ret=$${?}; \
> > +		case $${ret} in (0|3) ;; (*) exit 1;; esac; \
> > +	} && \
> 
> I generally agree with the idea, even more since the implementation is
> really a few-liner. But I have a few remarks.
> 
> The first is one of personal coding style taste. I don't like very much
> the weird check for values 0 and 3. It does not make any sense unless
> one opens check-hash and read the possible return values, which look a
> bit like a randomly-ordered enum.

Meh... It was not randomly ordered. At least, I tried to copme up with a
meaningful ordering, where the higher the error code, the more critical
the error...

> I'd rather prefer if we could call check-hash with a command line flag
> to ask that missing hashes generate a warning and return 0 instead of
> the default behavior. This would mean check-hash always returns 0 for
> "go" and non-zero for "abort".

Hmmm... Let's see...

> The second remark is about the output of 'make legal-info'. With this
> patch the output is:
> 
> $ make legal-info
> >>>   Collecting legal info
> WARNING: no hash file for COPYING
> WARNING: no hash file for COPYING
> WARNING: no hash file for COPYING3
> WARNING: no hash file for COPYING.LIB
> WARNING: no hash file for COPYING.LESSERv3
> WARNING: no hash file for COPYINGv2
> ...
> 
> There's no mention of the package involved, which doesn't help
> in fixing it. It should print the package name, e.g.:
> 
> $ make legal-info
> >>>   Collecting legal info
> WARNING: binutils: no hash file for COPYING
> WARNING: mpc:      no hash file for COPYING
> ...
> 
> To achieve this I guess we'll need to pass the package name to
> check-hash. It doesn't hurt if we add the package name unconditionally,
> even to currently existing messages where it is not needed.

Or just change legal-info to call MESSAGE, like so:

   838 $(1)-legal-info: PKG=$(2)
   839 $(1)-legal-info:
   840 ?   @$$(call MESSAGE,"Collecting legal info")

Which provides an output like:

    >>> busybox 1.26.2 Collecting legal info
    ERROR: No hash found for LICENSE

Would that be OK for you?

Note however that running legal-info from within a clean (configured but
not built) tree yields a lot of output, because the packages are
extrated and patched, and the warnings during legal-info are lost across
the whole mess and difficult to catch.

Regards,
Yann E. MORIN.

> -- 
> Luca

-- 
.-----------------.--------------------.------------------.--------------------.
|  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 0/3] core: check hashes of license files

[Yann E. MORIN]

Luca, All,

On 2017-06-23 23:50 +0200, Luca Ceresoli spake thusly:
> On 20/06/2017 17:28, Yann E. MORIN wrote:
> > Thomas, All,
> > 
> > On 2017-06-19 21:32 +0200, Thomas De Schampheleire spake thusly:
> >> 2017-06-19 19:47 GMT+02:00 Yann E. MORIN <yann.morin.1998@free.fr>:
> >>> On 2017-06-19 22:47 +0530, Rahul Bedarkar spake thusly:
> >>>> On Sun, Jun 18, 2017 at 1:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >>>>>
> >>>>> Hello All!
> >>>>>
> >>>>> This small series is a proposal to check the hashes of the license files
> >>>>> during legal-info, to catch the packages whose license changes but where
> >>>>> the text of the new license is in the same file.
> >>>>
> >>>> Thanks for this series. Checking hashes of the license files during
> >>>> legal-info stage looks logical but we discussed about doing that after
> >>>> downloading sources so that change in license file is noticed early
> >>>> (as a part of build test after version bump).
> >>>
> >>> It is not possible to do at download time. It can only be done after
> >>> the package has been extracted and patched.
> >>>
> >>> That is why, when you run legal-info on a non-built (but configured)
> >>> tree, you'll notice that Buildroot extracts and patches the packages
> >>> before saving their legal-info.
> >>>
> >>> Besides, if one uses the support/scripts/test-pkg script to test the
> >>> version bump, then legal-info is run by the script.
> >>>
> >>> So, I still believe it is better done during legal-info.
> >>>
> >>
> >> Yann, I think Rahul means that the checking of the hashing should be
> >> checked as part of the standard 'make pkg' target, whichever subtarget
> >> it is, be it -build, -install or what not.
> > 
> > OK, I see.
> > 
> > Still, I believe it is better suited to keep that for during the
> > legal-info step.
> > 
> > Regards,
> > Yann E. MORIN.
> > 
> >> But, I don't think we should mix such topics: legal info topics should
> >> stay in the -legal-info target.
> >> One solution could be to make '-legal-info' part of the standard build
> >> process, although it will slow down the build and some/many people
> >> will not like that.
> >> An alternative is to split '-legal-info' in two parts:
> >> -legal-info-checks and actual -legal-info. The first part would verify
> >> some important things, i.e. presence of valid LICENSE, presence of all
> >> files specified in LICENSE_FILES, hash checking on these files. It
> >> could be added to the standard 'make pkg' group. The second part would
> >> do the actual creation of the manifest, copying the sources, etc. and
> >> remains on-demand only.
> 
> Thomas' analysis is technically correct, but implementing what he
> describes it is a bit complex for the benefits it grants IMO.

Well, I don't think if is very complex either.

My position is about keeping sheeps together: checking the license files
is part of legal-info.

And especially since what we really want is to check them at the very
moment we want to aggregate the legal-info structure: only then do we
have to ensure their correctness.

Regards,
Yann E. MORIN.

> So I agree
> with Yann to keep it simple and require patch submitters to use test-pkg
> before submitting patches. And of course we can change this choice in
> the future.


-- 
.-----------------.--------------------.------------------.--------------------.
|  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 2/3] core/pkg-utils: check hashes of license files

[Luca Ceresoli]

Hi Yann,

On 25/06/2017 20:09, Yann E. MORIN wrote:
> Luca, All,
> 
> On 2017-06-23 23:49 +0200, Luca Ceresoli spake thusly:
>> On 18/06/2017 10:01, Yann E. MORIN wrote:
>>> This will help catch a change of license even if the filename does
>>> not change.
>>>
>>> For now, a missing hash for the license files is not a fatal error, to
>>> let people catch up and add them. When we switch to make it mandatory,
>>> we can simplify the code by just removing the case statement.
>>>
>>> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>>> Cc: Luca Ceresoli <luca@lucaceresoli.net>
>>> Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
>>> Cc: Peter Korsgaard <peter@korsgaard.com>
>>> ---
>>>  package/pkg-utils.mk | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
>>> index e9ac56276f..accf48c464 100644
>>> --- a/package/pkg-utils.mk
>>> +++ b/package/pkg-utils.mk
>>> @@ -85,5 +85,10 @@ endef
>>>  
>>>  define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
>>>  	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
>>> +	{ \
>>> +		support/download/check-hash $(3)/$(1).hash $(5) $(4); \
>>> +		ret=$${?}; \
>>> +		case $${ret} in (0|3) ;; (*) exit 1;; esac; \
>>> +	} && \
>>
>> I generally agree with the idea, even more since the implementation is
>> really a few-liner. But I have a few remarks.
>>
>> The first is one of personal coding style taste. I don't like very much
>> the weird check for values 0 and 3. It does not make any sense unless
>> one opens check-hash and read the possible return values, which look a
>> bit like a randomly-ordered enum.
> 
> Meh... It was not randomly ordered. At least, I tried to copme up with a
> meaningful ordering, where the higher the error code, the more critical
> the error...

Apologies, I hadn't read the error codes very carefully to get the logic
behind their order. I did it now, and I think giving them a meaningful
order is non trivial anyway.

>> I'd rather prefer if we could call check-hash with a command line flag
>> to ask that missing hashes generate a warning and return 0 instead of
>> the default behavior. This would mean check-hash always returns 0 for
>> "go" and non-zero for "abort".

Still my main point is unchanged: I prefer that we use the return value
as a go/nogo boolean, and add flags to change what is considered a nogo
condition. Take as an example the '-i' flag to grep: it changes the
matching logic to also accept matches of different case; it does not
return a special return value for matches-with-different-case, that
would be annoying for users to check.

But as an afterthought, if the plan is to consider return value 3 as an
error at some point in the foreseeable future, then this line is
temporary as you stated in the patch comment and I think I can tolerate
it. :)

>> The second remark is about the output of 'make legal-info'. With this
>> patch the output is:
>>
>> $ make legal-info
>>>>>   Collecting legal info
>> WARNING: no hash file for COPYING
>> WARNING: no hash file for COPYING
>> WARNING: no hash file for COPYING3
>> WARNING: no hash file for COPYING.LIB
>> WARNING: no hash file for COPYING.LESSERv3
>> WARNING: no hash file for COPYINGv2
>> ...
>>
>> There's no mention of the package involved, which doesn't help
>> in fixing it. It should print the package name, e.g.:
>>
>> $ make legal-info
>>>>>   Collecting legal info
>> WARNING: binutils: no hash file for COPYING
>> WARNING: mpc:      no hash file for COPYING
>> ...
>>
>> To achieve this I guess we'll need to pass the package name to
>> check-hash. It doesn't hurt if we add the package name unconditionally,
>> even to currently existing messages where it is not needed.
> 
> Or just change legal-info to call MESSAGE, like so:
> 
>    838 $(1)-legal-info: PKG=$(2)
>    839 $(1)-legal-info:
>    840 ?   @$$(call MESSAGE,"Collecting legal info")
> 
> Which provides an output like:
> 
>     >>> busybox 1.26.2 Collecting legal info
>     ERROR: No hash found for LICENSE
> 
> Would that be OK for you?

Sure, good idea.

> Note however that running legal-info from within a clean (configured but
> not built) tree yields a lot of output, because the packages are
> extrated and patched, and the warnings during legal-info are lost across
> the whole mess and difficult to catch.

I don't think we can do much about this. They won't be lost anymore when
these warnings will become errors.

-- 
Luca

[Buildroot] [PATCH 3/3] docs/manual: document hashes for license files

[Yann E. MORIN]

Ricardo, All,

On 2017-06-22 23:28 -0300, Ricardo Martincoski spake thusly:
> On Sun, Jun 18, 2017 at 05:01 AM, Yann E. MORIN wrote:
> > +# Hash for license files:
> > +sha1  c47a888f2be626e1197b6f651dee966ef882077d  COPYING
> > +sha1  e101765734390d664b59325b2d644d80d9a6bd9a  doc/COPYING.LGPL
> 
> This will need a small change in check-package to not generate false warnings
> like this: "use filename without directory component".
> See HashFilename in checkpackagelib/lib_hash.py
> 
> 'make package-dirclean package-source' already generates 'ERROR: No hash found'
> if someone mistakenly adds a directory, for example dl/, to the name of the
> tarball in the hash file, so we are safe to just remove the entire HashFilename
> class.

Indeed, good catch!

Will fix for the next iteration.

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