toolchain/helpers: make sure we bail out when kernel headers check fails
diff mbox series

Message ID 20191112202118.56136-1-thomas.petazzoni@bootlin.com
State Accepted
Headers show
Series
  • toolchain/helpers: make sure we bail out when kernel headers check fails
Related show

Commit Message

Thomas Petazzoni Nov. 12, 2019, 8:21 p.m. UTC
In commit 6136765b23abd9faba610dd54ed276a777811575 ("toolchain:
generate check-headers program under $(BUILD_DIR)"), the
check_kernel_headers_version function was simplified to not check the
return value of the check-kernel-headers.sh script, assuming that
"make" does bail out on the first failing command.

However, check_kernel_headers_version when used in $(2)_CONFIGURE_CMDS
from pkg-toolchain-external.mk, is called in a sequence of commands,
where the return value of each command is not checked. Therefore, a
failure of check-kernel-headers.sh no longer aborts the build.

Since all other macros are using this principle of calling "exit 1",
we revert back to the same for check_kernel_headers_version, as it was
done prior to 6136765b23abd9faba610dd54ed276a777811575.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Carlos Santos <unixmania@gmail.com>
---
 toolchain/helpers.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Carlos Santos Nov. 12, 2019, 9:21 p.m. UTC | #1
On Tue, Nov 12, 2019 at 5:21 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> In commit 6136765b23abd9faba610dd54ed276a777811575 ("toolchain:
> generate check-headers program under $(BUILD_DIR)"), the
> check_kernel_headers_version function was simplified to not check the
> return value of the check-kernel-headers.sh script, assuming that
> "make" does bail out on the first failing command.
>
> However, check_kernel_headers_version when used in $(2)_CONFIGURE_CMDS
> from pkg-toolchain-external.mk, is called in a sequence of commands,
> where the return value of each command is not checked. Therefore, a
> failure of check-kernel-headers.sh no longer aborts the build.
>
> Since all other macros are using this principle of calling "exit 1",
> we revert back to the same for check_kernel_headers_version, as it was
> done prior to 6136765b23abd9faba610dd54ed276a777811575.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Carlos Santos <unixmania@gmail.com>
> ---
>  toolchain/helpers.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 42e5522060..996cc70d44 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -163,7 +163,9 @@ copy_toolchain_sysroot = \
>  # $3: kernel version string, in the form: X.Y
>  #
>  check_kernel_headers_version = \
> -       support/scripts/check-kernel-headers.sh $(1) $(2) $(3)
> +       if ! support/scripts/check-kernel-headers.sh $(1) $(2) $(3); then \
> +               exit 1; \
> +       fi
>
>  #
>  # Check the specific gcc version actually matches the version in the
> --
> 2.23.0
>

The function became a one-liner after commit 6136765b23. Wouldn't it
be simpler and more readable to run the script and test the result,
both in linux-headers.mk and pkg-toolchain-external.mk?
Thomas Petazzoni Nov. 12, 2019, 9:25 p.m. UTC | #2
On Tue, 12 Nov 2019 18:21:15 -0300
Carlos Santos <unixmania@gmail.com> wrote:

> > diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> > index 42e5522060..996cc70d44 100644
> > --- a/toolchain/helpers.mk
> > +++ b/toolchain/helpers.mk
> > @@ -163,7 +163,9 @@ copy_toolchain_sysroot = \
> >  # $3: kernel version string, in the form: X.Y
> >  #
> >  check_kernel_headers_version = \
> > -       support/scripts/check-kernel-headers.sh $(1) $(2) $(3)
> > +       if ! support/scripts/check-kernel-headers.sh $(1) $(2) $(3); then \
> > +               exit 1; \
> > +       fi
> >
> >  #
> >  # Check the specific gcc version actually matches the version in the
> > --
> > 2.23.0
> >  
> 
> The function became a one-liner after commit 6136765b23. Wouldn't it
> be simpler and more readable to run the script and test the result,
> both in linux-headers.mk and pkg-toolchain-external.mk?

Yes, we could certainly do that. The aim of my patch was really just to
get back to where we were before, to fix the immediate issue. This of
course doesn't mean we can't improve things further.

I'll let Peter/Arnout/Yann decide what they want to do (i.e apply my
patch as-is, or have a more elaborate version).

Thanks!

Thomas
Carlos Santos Nov. 12, 2019, 9:30 p.m. UTC | #3
On Tue, Nov 12, 2019 at 6:25 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Tue, 12 Nov 2019 18:21:15 -0300
> Carlos Santos <unixmania@gmail.com> wrote:
>
> > > diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> > > index 42e5522060..996cc70d44 100644
> > > --- a/toolchain/helpers.mk
> > > +++ b/toolchain/helpers.mk
> > > @@ -163,7 +163,9 @@ copy_toolchain_sysroot = \
> > >  # $3: kernel version string, in the form: X.Y
> > >  #
> > >  check_kernel_headers_version = \
> > > -       support/scripts/check-kernel-headers.sh $(1) $(2) $(3)
> > > +       if ! support/scripts/check-kernel-headers.sh $(1) $(2) $(3); then \
> > > +               exit 1; \
> > > +       fi
> > >
> > >  #
> > >  # Check the specific gcc version actually matches the version in the
> > > --
> > > 2.23.0
> > >
> >
> > The function became a one-liner after commit 6136765b23. Wouldn't it
> > be simpler and more readable to run the script and test the result,
> > both in linux-headers.mk and pkg-toolchain-external.mk?
>
> Yes, we could certainly do that. The aim of my patch was really just to
> get back to where we were before, to fix the immediate issue. This of
> course doesn't mean we can't improve things further.
>
> I'll let Peter/Arnout/Yann decide what they want to do (i.e apply my
> patch as-is, or have a more elaborate version).
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Hum, perhaps we should keep the design pattern, considering that other
functions (e.g. check_gcc_version) are used the same way, so

Reviewed-by: Carlos Santos <unixmania@gmail.com>
Yann E. MORIN Nov. 13, 2019, 9:41 p.m. UTC | #4
Thomas, All,

On 2019-11-12 21:21 +0100, Thomas Petazzoni spake thusly:
> In commit 6136765b23abd9faba610dd54ed276a777811575 ("toolchain:
> generate check-headers program under $(BUILD_DIR)"), the
> check_kernel_headers_version function was simplified to not check the
> return value of the check-kernel-headers.sh script, assuming that
> "make" does bail out on the first failing command.
> 
> However, check_kernel_headers_version when used in $(2)_CONFIGURE_CMDS
> from pkg-toolchain-external.mk, is called in a sequence of commands,
> where the return value of each command is not checked. Therefore, a
> failure of check-kernel-headers.sh no longer aborts the build.
> 
> Since all other macros are using this principle of calling "exit 1",
> we revert back to the same for check_kernel_headers_version, as it was
> done prior to 6136765b23abd9faba610dd54ed276a777811575.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Carlos Santos <unixmania@gmail.com>

Applied to master, thanks.

Regards,
Yann E. MORIN.

> ---
>  toolchain/helpers.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 42e5522060..996cc70d44 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -163,7 +163,9 @@ copy_toolchain_sysroot = \
>  # $3: kernel version string, in the form: X.Y
>  #
>  check_kernel_headers_version = \
> -	support/scripts/check-kernel-headers.sh $(1) $(2) $(3)
> +	if ! support/scripts/check-kernel-headers.sh $(1) $(2) $(3); then \
> +		exit 1; \
> +	fi
>  
>  #
>  # Check the specific gcc version actually matches the version in the
> -- 
> 2.23.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Peter Korsgaard Nov. 19, 2019, 7:55 a.m. UTC | #5
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 > On 2019-11-12 21:21 +0100, Thomas Petazzoni spake thusly:
 >> In commit 6136765b23abd9faba610dd54ed276a777811575 ("toolchain:
 >> generate check-headers program under $(BUILD_DIR)"), the
 >> check_kernel_headers_version function was simplified to not check the
 >> return value of the check-kernel-headers.sh script, assuming that
 >> "make" does bail out on the first failing command.
 >> 
 >> However, check_kernel_headers_version when used in $(2)_CONFIGURE_CMDS
 >> from pkg-toolchain-external.mk, is called in a sequence of commands,
 >> where the return value of each command is not checked. Therefore, a
 >> failure of check-kernel-headers.sh no longer aborts the build.
 >> 
 >> Since all other macros are using this principle of calling "exit 1",
 >> we revert back to the same for check_kernel_headers_version, as it was
 >> done prior to 6136765b23abd9faba610dd54ed276a777811575.
 >> 
 >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
 >> Cc: Carlos Santos <unixmania@gmail.com>

 > Applied to master, thanks.

Committed to 2019.02.x and 2019.08.x, thanks.

Patch
diff mbox series

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index 42e5522060..996cc70d44 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -163,7 +163,9 @@  copy_toolchain_sysroot = \
 # $3: kernel version string, in the form: X.Y
 #
 check_kernel_headers_version = \
-	support/scripts/check-kernel-headers.sh $(1) $(2) $(3)
+	if ! support/scripts/check-kernel-headers.sh $(1) $(2) $(3); then \
+		exit 1; \
+	fi
 
 #
 # Check the specific gcc version actually matches the version in the