[Buildroot] [PATCH] core/legal-info: use hash file from version sub-dir

[Buildroot] [PATCH] core/legal-info: use hash file from version sub-dir

[Yann E. MORIN]

When we have multiple versions for a package, and the licensing terms
depend on the version actually selected (e.g. like Qt5), storing the
hashes for those license files in the .hash file is broken: the infra
will ensure that all hashes for a file do match, which would not be the
case here.

We fix that by first looking for a hash file in the version sub-dir
first, and if that directory does not exist, then we use the main hash
file. Note that if the version sub-dir exists, the main hash file is
not used even if there is no per-version hash file.

Update the documentation accordingly.

Reported-by: Joshua Henderson <joshua.henderson@microchip.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
---
 docs/manual/adding-packages-directory.txt | 4 +++-
 package/pkg-utils.mk                      | 9 ++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
index 804946c504..809cc97389 100644
--- a/docs/manual/adding-packages-directory.txt
+++ b/docs/manual/adding-packages-directory.txt
@@ -482,7 +482,9 @@ 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.
+package version is bumped. For a package with multiple versions (like Qt5),
+create the hash file in a subdirectory +<packageversion>+ of that package
+(see also xref:patch-apply-order[]).
 
 .Note
 The number of spaces does not matter, so one can use spaces (or tabs) to
diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index accf48c464..f285395267 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -86,9 +86,12 @@ 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; \
+		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
+			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
+		else \
+			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
+		fi; \
+		case $${?} in (0|3) ;; (*) exit 1;; esac; \
 	} && \
 	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
-- 
2.11.0

[Buildroot] [PATCH] core/legal-info: use hash file from version sub-dir

[Joshua Henderson]

Yann,

On 07/13/2017 03:19 PM, Yann E. MORIN wrote:

> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> index accf48c464..f285395267 100644
> --- a/package/pkg-utils.mk
> +++ b/package/pkg-utils.mk
> @@ -86,9 +86,12 @@ 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; \
> +		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
> +			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
> +		else \
> +			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
> +		fi; \
> +		case $${?} in (0|3) ;; (*) exit 1;; esac; \
>  	} && \
>  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
>  endef
> 

I think this has unintended side effects.  If there is already a $(PKG)_VERSION directory for patches, 
it will look for the hash file there even if there are no hash file differences, which results in missing
license file hash.

I believe it would work if you test -f on the actual .hash file instead of just the directory.  But, do note
that if there are multiple license files per package, and only one of them is different, this still results
in putting all hashes including duplicate ones in separate hash files.

Josh

[Buildroot] [PATCH] core/legal-info: use hash file from version sub-dir

[Luca Ceresoli]

Hi Yann, Joshua,

On 14/07/2017 01:32, Joshua Henderson wrote:
> Yann,
> 
> On 07/13/2017 03:19 PM, Yann E. MORIN wrote:
> 
>> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
>> index accf48c464..f285395267 100644
>> --- a/package/pkg-utils.mk
>> +++ b/package/pkg-utils.mk
>> @@ -86,9 +86,12 @@ 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; \
>> +		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
>> +			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
>> +		else \
>> +			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
>> +		fi; \
>> +		case $${?} in (0|3) ;; (*) exit 1;; esac; \
>>  	} && \
>>  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
>>  endef
>>
> 
> I think this has unintended side effects.  If there is already a $(PKG)_VERSION directory for patches, 
> it will look for the hash file there even if there are no hash file differences, which results in missing
> license file hash.
> 
> I believe it would work if you test -f on the actual .hash file instead of just the directory.  But, do note
> that if there are multiple license files per package, and only one of them is different, this still results
> in putting all hashes including duplicate ones in separate hash files.

I agree with Josh here.

A cleaner solution would be to put only version-bound hashes in
version-named subdirs, and leave all other hashes in the base directory.
This would remove all duplication and avoid unintended disabling of all
hashes when one <version>/*.hash file is created, which would happen
only for those versions that actually have that file.

I think this can be achieved in a simple way by cat-ing together
$(3)/$(1).hash and $(3)/$($(PKG)_VERSION)/$(1).hash in a temporary file
and feed that to check-hash. Additionally if check-hash were able to
read the hashes from stdin, no temporary file would be even needed, but
this is not necessarily a good idea.

-- 
Luca

[Buildroot] [PATCH] core/legal-info: use hash file from version sub-dir

[Yann E. MORIN]

Joshua, All,

On 2017-07-13 16:32 -0700, Joshua Henderson spake thusly:
> On 07/13/2017 03:19 PM, Yann E. MORIN wrote:
> 
> > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> > index accf48c464..f285395267 100644
> > --- a/package/pkg-utils.mk
> > +++ b/package/pkg-utils.mk
> > @@ -86,9 +86,12 @@ 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; \
> > +		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
> > +			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
> > +		else \
> > +			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
> > +		fi; \
> > +		case $${?} in (0|3) ;; (*) exit 1;; esac; \
> >  	} && \
> >  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
> >  endef
> > 
> 
> I think this has unintended side effects.  If there is already a $(PKG)_VERSION directory for patches, 
> it will look for the hash file there even if there are no hash file differences, which results in missing
> license file hash.

This is the intended behaviour, yes: if there is a version choice, then
we _want_ the license hashes to be per version, even if they are the
same across versions. We do not want them in the 'main' hash file.

Yes, this means duplicated entries.

I was also wondering if we should also move the hashes for the
downlaoded files to be per-version as well, to keep things together.

Ie., we would have something like:

    package/qt5/qt5base/Config.in
    package/qt5/qt5base/qt5base.mk
    package/qt5/qt5base/5.6.2/*.patch
    package/qt5/qt5base/5.6.2/qt5base.hash
    package/qt5/qt5base/5.8.0/*.patch
    package/qt5/qt5base/5.8.0/qt5base.hash



> I believe it would work if you test -f on the actual .hash file instead of just the directory.

Again, if the directory exists, I intended that the main hash file was
not used at all.

>  But, do note
> that if there are multiple license files per package, and only one of them is different, this still results
> in putting all hashes including duplicate ones in separate hash files.

Yes, this is what I had in mind in this case.

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] core/legal-info: use hash file from version sub-dir

[Yann E. MORIN]

Luca, All,

On 2017-07-14 09:39 +0200, Luca Ceresoli spake thusly:
> On 14/07/2017 01:32, Joshua Henderson wrote:
> > Yann,
> > 
> > On 07/13/2017 03:19 PM, Yann E. MORIN wrote:
> > 
> >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> >> index accf48c464..f285395267 100644
> >> --- a/package/pkg-utils.mk
> >> +++ b/package/pkg-utils.mk
> >> @@ -86,9 +86,12 @@ 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; \
> >> +		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
> >> +			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
> >> +		else \
> >> +			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
> >> +		fi; \
> >> +		case $${?} in (0|3) ;; (*) exit 1;; esac; \
> >>  	} && \
> >>  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
> >>  endef
> >>
> > 
> > I think this has unintended side effects.  If there is already a $(PKG)_VERSION directory for patches, 
> > it will look for the hash file there even if there are no hash file differences, which results in missing
> > license file hash.
> > 
> > I believe it would work if you test -f on the actual .hash file instead of just the directory.  But, do note
> > that if there are multiple license files per package, and only one of them is different, this still results
> > in putting all hashes including duplicate ones in separate hash files.
> 
> I agree with Josh here.
> 
> A cleaner solution would be to put only version-bound hashes in
> version-named subdirs, and leave all other hashes in the base directory.
> This would remove all duplication and avoid unintended disabling of all
> hashes when one <version>/*.hash file is created, which would happen
> only for those versions that actually have that file.

See my reply to Joshua: I would favour the opposite, in fact, to move
all hashes to the per-version directory, even if there is duplication.

> I think this can be achieved in a simple way by cat-ing together
> $(3)/$(1).hash and $(3)/$($(PKG)_VERSION)/$(1).hash in a temporary file
> and feed that to check-hash. Additionally if check-hash were able to
> read the hashes from stdin, no temporary file would be even needed, but
> this is not necessarily a good idea.

cat-ing to a temporary file will bneed that we arrange for a trickier
code, because we need a unique filename for when we want to support
TLPB.

As for stdin, this would be doable quite easily, we just need to
recognise '-' as stdin, like almost all of the rest of the world. ;-)

But still, my preference would be to move hashes to the per-version
directory. All hashes, not just license ones.

In the meantime, I've marked the patch as 'changes requested'.

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] core/legal-info: use hash file from version sub-dir

[Arnout Vandecappelle]

On 14-07-17 10:38, Yann E. MORIN wrote:
> Joshua, All,
> 
> On 2017-07-13 16:32 -0700, Joshua Henderson spake thusly:
>> On 07/13/2017 03:19 PM, Yann E. MORIN wrote:
>>
>>> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
>>> index accf48c464..f285395267 100644
>>> --- a/package/pkg-utils.mk
>>> +++ b/package/pkg-utils.mk
>>> @@ -86,9 +86,12 @@ 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; \
>>> +		if [ -d $(3)/$($(PKG)_VERSION) ]; then \
>>> +			support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \
>>> +		else \
>>> +			support/download/check-hash $(3)/$(1).hash $(5) $(4); \
>>> +		fi; \
>>> +		case $${?} in (0|3) ;; (*) exit 1;; esac; \
>>>  	} && \
>>>  	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
>>>  endef
>>>
>>
>> I think this has unintended side effects.  If there is already a $(PKG)_VERSION directory for patches, 
>> it will look for the hash file there even if there are no hash file differences, which results in missing
>> license file hash.
> 
> This is the intended behaviour, yes: if there is a version choice, then
> we _want_ the license hashes to be per version, even if they are the
> same across versions. We do not want them in the 'main' hash file.
> 
> Yes, this means duplicated entries.
> 
> I was also wondering if we should also move the hashes for the
> downlaoded files to be per-version as well, to keep things together.
> 
> Ie., we would have something like:
> 
>     package/qt5/qt5base/Config.in
>     package/qt5/qt5base/qt5base.mk
>     package/qt5/qt5base/5.6.2/*.patch
>     package/qt5/qt5base/5.6.2/qt5base.hash
>     package/qt5/qt5base/5.8.0/*.patch
>     package/qt5/qt5base/5.8.0/qt5base.hash

 This indeed sounds like the right approach. But then both uses of the hash file
(download and legal-info) should use the same approach, as you say.

 In fact, this can be a workaround for cases where we often don't have a hash
because of custom version specification, like the kernel...


 Perhaps the nicest approach is to move the intelligence down into check-hash
itself (the less shell code in the makefiles the better). So check-hash would
not get a hash file as argument, but it would get package directory and version.


 Note by the way that pkg-stats should probably be updated as well.


 Sorry to give you more work :-)


 Regards,
 Arnout

> 
> 
> 
>> I believe it would work if you test -f on the actual .hash file instead of just the directory.
> 
> Again, if the directory exists, I intended that the main hash file was
> not used at all.
> 
>>  But, do note
>> that if there are multiple license files per package, and only one of them is different, this still results
>> in putting all hashes including duplicate ones in separate hash files.
> 
> Yes, this is what I had in mind in this case.
> 
> Regards,
> Yann E. MORIN.
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF