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

Luca Ceresoli luca at lucaceresoli.net
Sun Jun 25 21:49:16 UTC 2017


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 at free.fr>
>>> Cc: Luca Ceresoli <luca at lucaceresoli.net>
>>> Cc: Rahul Bedarkar <rahulbedarkar89 at gmail.com>
>>> Cc: Peter Korsgaard <peter at 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



More information about the buildroot mailing list