[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