| Message ID | 22d695ca68ad664cbe5af125683b61c80f828d35.1590183628.git.yann.morin.1998@free.fr |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] core.br2-external: fix reporting errors | expand |
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" 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. With that fixed(improved :p): Reviewed-by: Romain Naour <romain.naour@gmail.com> Tested-by: Romain Naour <romain.naour@gmail.com> 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. 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@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() { >
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@gmail.com> > Tested-by: Romain Naour <romain.naour@gmail.com> Does that still stand with the patch 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! ;-) 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@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() { > > >
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@gmail.com> >> Tested-by: Romain Naour <romain.naour@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@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() { >>> >> >
Romain, All, On 2020-06-14 18:47 +0200, Romain Naour spake thusly: > Le 14/06/2020 à 17:55, Yann E. MORIN a écrit : > > On 2020-06-14 17:33 +0200, Romain Naour spake thusly: [--SNIP--] > >> With that fixed(improved :p): > >> Reviewed-by: Romain Naour <romain.naour@gmail.com> > >> Tested-by: Romain Naour <romain.naour@gmail.com> > > Does that still stand with the patch as-is? > Ok as-is. Applied to master; thanks for the review. Regards, Yann E. MORIN.
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > 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. > 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@free.fr> Committed to 2020.02.x and 2020.05.x, thanks.
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() {
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. 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@free.fr> --- support/scripts/br2-external | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)