[Buildroot] [PATCH 1/2] core.br2-external: fix reporting errors

[Romain Naour]

Yann,

Le 14/06/2020 à 17:55, Yann E. MORIN a écrit :
> Romain, All,
> 
> On 2020-06-14 17:33 +0200, Romain Naour spake thusly:
>> Le 22/05/2020 à 23:40, Yann E. MORIN a écrit :
>>> When a br2-external tree has an issue, e.g. a missing file, or does not
>>> have a name, or the name uses invalid cahrs, we report that condition by
>>> setting the variable BR2_EXTERNAL_ERROR.
>>>
>>> That variable is defined in the script support/scripts/br2-external,
>>> which outputs it on stdout, and checked by the Makefile.
>>>
>>> Before d027cd75d09, stdout was explicitly redirected to the generated
>>> .mk file, with   exec >"${ofile}"   as the Makefile and Kconfig
>>> fragments were generated each with their own call to the script, and o
>>> the validation phase would emit the BR2_EXTERNAL_ERROR variable in the
>>> Makefile fragment.
>>>
>>> But with d027cd75d09, both the Makefile and Kconfig fragmetns were now
>>> generated with a single call to the script, and as such the semantics of
>>> the scripts changed, and only each of the actual generators, do_mk and
>>> do_kconfig, ahd their out put redirected. Which left do_validate with
>>> the default stdout. Which would emit BR2_EXTERNAL_ERROR on stdout.
>>>
>>> In turn, the stdout of the script would be interpreted by as part of the
>>> Makefile. But this does not end up very well when a br2-external tree
>>> indeed has an error:
>>>
>>>     Makefile:184: *** multiple target patterns.  Stop.
>>
>> When external.desc exist but empty, I get another error (I'm currently on master)
>>
>> $ echo "" > support/testing/tests/download/br2-external/git-hash/external.desc
>> $ make BR2_EXTERNAL=$PWD/support/testing/tests/download/br2-external/git-hash/
>> menuconfig
>>
>> Config.in:22: can't open file "output/.br2-external.in.paths"
> 
> Ah yeah, this one is also fixed! ;-)
> 
>> But Indeed, I get the same error when external.desc is missing.
>>
>>
>> Here is the error message displayed with the series applied:
>>
>> case: missing external.desc
>>
>>   does not have an 'external.desc'. See
>> https://buildroot.org/manual.html#br2-external-converting.
>>
>> case: empty external.desc
>>
>>   does not define the name.
>>
>> While at it you can use MANUAL_URL in the error message when the external name
>> is missing.
> 
> I disagree. The first test is for testing that an old br2-external, from
> before the multi-br2-external support, in which case we want to point
> the user to the manual for the conversion step.
> 
> However, when external.desc exists, this is not a conversion, and the
> user is already aware of the requirements for external.desc; they just
> made a mistake and should just refer to the manual.
> 
>> With that fixed(improved :p):
>> Reviewed-by: Romain Naour <romain.naour at gmail.com>
>> Tested-by: Romain Naour <romain.naour at gmail.com>
> 
> Does that still stand with the patch as-is?

Ok as-is.

> 
>> Also, it would be great to have some tests in the testsuite for such cases. It's
>> easy to make a mistake while adding a br2_external tree.
> 
> As already shared on IRC, I hae a set of test-cases that I wrote back in
> the days I was working on multi-br2-external support:
> 
>     https://git.buildroot.org/~ymorin/git/br2-external-tests/
> 
> Feel free to grab those and turn them into actual tests in our testing
> infra! ;-)

Best regards,
Romain

> 
> Regards,
> Yann E. MORIN.
> 
>> Best regards,
>> Romain
>>
>>
>>>
>>> So we must redirect the output of the validation step to the
>>> Makefile fragment.
>>>
>>> Note that we don't need to append in do_mk, and we can do an overwrite
>>> redirection: if we go so far as to call do_mk, it means there was no
>>> error, and thus the fragment is empty.
>>>
>>> Signed-off-by: Yann E. MORIN <yann.morin.1998 at free.fr>
>>> ---
>>>  support/scripts/br2-external | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/support/scripts/br2-external b/support/scripts/br2-external
>>> index 171526f8c8..fdea5aa251 100755
>>> --- a/support/scripts/br2-external
>>> +++ b/support/scripts/br2-external
>>> @@ -33,9 +33,8 @@ main() {
>>>      # Trap any unexpected error to generate a meaningful error message
>>>      trap "error 'unexpected error while generating ${ofile}\n'" ERR
>>>  
>>> -    do_validate ${@//:/ }
>>> -
>>>      mkdir -p "${outputdir}"
>>> +    do_validate "${outputdir}" ${@//:/ }
>>>      do_mk "${outputdir}"
>>>      do_kconfig "${outputdir}"
>>>  }
>>> @@ -51,7 +50,9 @@ main() {
>>>  # snippet means that there were no error.
>>>  #
>>>  do_validate() {
>>> +    local outputdir="${1}"
>>>      local br2_ext
>>> +    shift
>>>  
>>>      if [ ${#} -eq 0 ]; then
>>>          # No br2-external tree is valid
>>> @@ -60,7 +61,7 @@ do_validate() {
>>>  
>>>      for br2_ext in "${@}"; do
>>>          do_validate_one "${br2_ext}"
>>> -    done
>>> +    done >"${outputdir}/.br2-external.mk"
>>>  }
>>>  
>>>  do_validate_one() {
>>>
>>
>