[6/7] support/scripts/qemu-boot: gitlab tests for Qemu

Enables to check various qemu architectures build states.
These scripts were inspired from toolchain builder :
https://github.com/bootlin/toolchains-builder/blob/master/build.sh
to test qemu's build process.
This allows to troubleshoot different issues that may be
associated with defective qemu builds by lanching a qemu machine,
sending root password, waiting for login shell and then perform
a shutdown.

The script expect.sh relies on expect package to automate
the tests.
We should mention that python-pexpect can be tweeked for the same
job but seems like it does hide the automation process as well
as any errors that may be encountered.

The script qemu-boot-defconfig_config.sh is required for
architectures that need special configuration before
starting compilation (like setting the correct tty).

On the other side, qemu-boot-checker.sh is used to read
the qemu command used to launch a qemu machine (by reading
board/qemu/qemu_architecture/readme.txt) as well as setting
the path to the qemu host and calling expect.sh.

Signed-off-by: Jugurtha BELKALEM <jugurtha.belkalem@smile.fr>
---
 support/scripts/expect.sh                     | 22 +++++++++++++++++
 support/scripts/qemu-boot-checker.sh          | 35 +++++++++++++++++++++++++++
 support/scripts/qemu-boot-defconfig_config.sh | 11 +++++++++
 3 files changed, 68 insertions(+)
 create mode 100755 support/scripts/expect.sh
 create mode 100755 support/scripts/qemu-boot-checker.sh
 create mode 100755 support/scripts/qemu-boot-defconfig_config.sh

Hi Jugurtha,

 Thank you for this. It is certainly useful! I have a few comments though.

On 29/04/2019 18:32, Jugurtha BELKALEM wrote:
> Enables to check various qemu architectures build states.
> These scripts were inspired from toolchain builder :
> https://github.com/bootlin/toolchains-builder/blob/master/build.sh
> to test qemu's build process.
> This allows to troubleshoot different issues that may be
> associated with defective qemu builds by lanching a qemu machine,
> sending root password, waiting for login shell and then perform
> a shutdown.
> 
> The script expect.sh relies on expect package to automate
> the tests.
> We should mention that python-pexpect can be tweeked for the same
> job but seems like it does hide the automation process as well
> as any errors that may be encountered.
> 
> The script qemu-boot-defconfig_config.sh is required for
> architectures that need special configuration before
> starting compilation (like setting the correct tty).
> 
> On the other side, qemu-boot-checker.sh is used to read
> the qemu command used to launch a qemu machine (by reading
> board/qemu/qemu_architecture/readme.txt) as well as setting
> the path to the qemu host and calling expect.sh.
> 
> Signed-off-by: Jugurtha BELKALEM <jugurtha.belkalem@smile.fr>
> ---
>  support/scripts/expect.sh                     | 22 +++++++++++++++++
>  support/scripts/qemu-boot-checker.sh          | 35 +++++++++++++++++++++++++++
>  support/scripts/qemu-boot-defconfig_config.sh | 11 +++++++++
>  3 files changed, 68 insertions(+)
>  create mode 100755 support/scripts/expect.sh
>  create mode 100755 support/scripts/qemu-boot-checker.sh
>  create mode 100755 support/scripts/qemu-boot-defconfig_config.sh
> 
> diff --git a/support/scripts/expect.sh b/support/scripts/expect.sh
> new file mode 100755
> index 0000000..6d65752
> --- /dev/null
> +++ b/support/scripts/expect.sh
> @@ -0,0 +1,22 @@
> +#!/usr/bin/expect
> +#
> +
> +set timeout 400
> +
> +log_file /tmp/expect_session.log
> +
> +eval spawn $env(QEMU_COMMAND)
> +
> +expect {
> +    eof {puts "Connection problem, exiting."; exit 1}
> +    timeout {puts "System did not boot in time, exiting."; exit 1}
> +    "buildroot login:"
> +}
> +send "root\r"
> +expect {
> +    eof {puts "Connection problem, exiting."; exit 1}
> +    timeout {puts "No shell, exiting."; exit 1}

 Here, the timeout should be *much* smaller than 400. Like, 2-3 seconds is more
appropriate.

> +    "# "
> +}
> +send "poweroff\r"
> +expect "System halted"

 Can we also have an expectation that the spawned command exits here?

> diff --git a/support/scripts/qemu-boot-checker.sh b/support/scripts/qemu-boot-checker.sh
> new file mode 100755
> index 0000000..f036516
> --- /dev/null
> +++ b/support/scripts/qemu-boot-checker.sh
> @@ -0,0 +1,35 @@
> +#!/bin/bash
> +
> +if [[ $1 = qemu* ]]; then
> +  device_name=$(echo $1  | sed -e 's#^qemu_##; s#_defconfig$##;' | sed -r 's/[_]/-/g')

 We don't really have consistent indentation in shell scripts, but it's either 4
spaces or one tab. I prefer one tab myself.

> +  if [ $device_name == "x86-64" ]; then
> +    device_name="x86_64"

 We should find a better way of handling this. E.g., renaming the directory to
x86-64 :-)

> +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name == "xtensa-lx60-nommu" ]; then
> +    echo "xtensa cannot be tested"

 If that is the case, why do we even have these qemu defconfigs? Max?


> +    exit 0
> +  elif [ $device_name == "m68k-q800" ] || [ $device_name == "ork1k" ]; then
                                                                ^^^^^or1k
> +    archQemuNoSupport
 In that case, no qemu can be built, so I think it's better to just check for
the presence of the qemu binary.

> +  fi
> +
> +  test_qemu_cmd="$(grep qemu-system $2/board/qemu/${device_name}/readme.txt)"
> +  qemu_command="$(echo "${test_qemu_cmd}"|tr -d '\n')"

 Instead of screenscraping this from the readme, I'd much prefer to rewrite it
so it can be called directly. E.g. for each defconfig have a launch command.

 It might even make sense to have the launch command in
package/qemu/Config.in.host so it is part of the defconfig, and then have some
way (e.g. utils/launch-qemu.sh) to launch it.

 But for now, I'd already be happy with board/qemu/*/launch.sh.


> +
> +  export QEMU_COMMAND="${qemu_command}"
> +
> +  export PATH="$2/output/host/bin:$PATH"
> +  echo $PATH
> +
> +  function archQemuNoSupport {

 These functions should be declared before the main functionality of the script.

> +    echo "cannot boot under qemu, support out of tree!"
> +    exit 0
> +  }
> +
> +  function boot_test {
> +    if ! expect expect.sh ; then
> +      echo "  booting test system ... FAILED"
> +      return 1
> +    fi
> +    echo "  booting test system ... SUCCESS"
> +  }
> +  boot_test

 IMO it doesn't make much sense to define a function and then immediately call
it. So you could just do:

	printf "  booting test system ... "
	if ! expect expect.sh ; then
		echo "FAILED"
		exit 1
	fi
	echo "SUCCESS"


 Also, the output of the spawned qemu should be logged somewhere, and that log
file should be a gitlab-ci artifact.

> +fi
> diff --git a/support/scripts/qemu-boot-defconfig_config.sh b/support/scripts/qemu-boot-defconfig_config.sh
> new file mode 100755
> index 0000000..526eee1
> --- /dev/null
> +++ b/support/scripts/qemu-boot-defconfig_config.sh
> @@ -0,0 +1,11 @@
> +#!/bin/bash
> +
> +if [[ $1 = qemu* ]]; then
> +  device_name=$(echo $1  | sed -e 's#^qemu_##; s#_defconfig$##;' | sed -r 's/[_]/-/g') 
> +  if [ $device_name == "x86-64" ]; then
> +    device_name="x86_64"
> +    sed -i "s/tty1/ttyS0/" $2/configs/$1
> +  elif [ $device_name == "x86" ]; then
> +    sed -i "s/tty1/ttyS0/" $2/configs/$1

 This sucks. I think we can just remove BR2_TARGET_GENERIC_GETTY_PORT from these
defconfigs and rely on the console (and make sure the console is set correctly
in the append argument of qemu).

 Regards,
 Arnout


> +  fi
> +fi
>

On Mon, Apr 29, 2019 at 3:32 PM Arnout Vandecappelle <arnout@mind.be> wrote:
> > +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name == "xtensa-lx60-nommu" ]; then
> > +    echo "xtensa cannot be tested"
>
>  If that is the case, why do we even have these qemu defconfigs? Max?

Tested both configurations just now, with the current buildroot tip
(2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
with QEMU command line from the readme.
Curious as well.

Thank you for you comments Arnout, I have corrected the indentation,
timeout and  function declaration issues.

However; I think that duplicating the content of readme.txt of each qemu
architecture would be difficult to maintain (even if it is more convenient
to use), this is why a project like toolchain builder :
https://github.com/bootlin/toolchains-builder/blob/master/build.sh is
reading directly from readme.txt.
Concerning the naming of x86_64 folder in board/qemu, it is the only one
that uses underscore instead of hyphen. Changing the directory name may
cause issues with other works as maybe other scripts as relying on this
name (especially that it exists since a long time ago).

I want to thank you also Max for your answer (We need to investigate more).

Regards.

On Tue, Apr 30, 2019 at 1:38 AM Max Filippov <jcmvbkbc@gmail.com> wrote:

> On Mon, Apr 29, 2019 at 3:32 PM Arnout Vandecappelle <arnout@mind.be>
> wrote:
> > > +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name ==
> "xtensa-lx60-nommu" ]; then
> > > +    echo "xtensa cannot be tested"
> >
> >  If that is the case, why do we even have these qemu defconfigs? Max?
>
> Tested both configurations just now, with the current buildroot tip
> (2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
> with QEMU command line from the readme.
> Curious as well.
>
> --
> Thanks.
> -- Max
>

Hi Jugurtha,

 [Please don't top-post, but reply inline like I do below.]

On 30/04/2019 10:54, Jugurtha BELKALEM wrote:
> Thank you for you comments Arnout, I have corrected the indentation, timeout
> and  function declaration issues.
> 
> However; I think that duplicating the content of readme.txt of each qemu
> architecture would be difficult to maintain (even if it is more convenient to
> use), this is why a project like toolchain builder :
> https://github.com/bootlin/toolchains-builder/blob/master/build.sh is reading
> directly from readme.txt.

 The idea would not be to duplicate it, but to move it. I.e., readme.txt just
says "launch with board/qemu/x86_64/launch.sh", and the actual command is in
that shell script.

 But maybe we should see what other Buildroot maintainers have to say about it.
Thomas, Peter, Yann, Romain?

> Concerning the naming of x86_64 folder in board/qemu, it is the only one that
> uses underscore instead of hyphen. Changing the directory name may cause issues
> with other works as maybe other scripts as relying on this name (especially that
> it exists since a long time ago).

 OK, true. Let's keep that like you have it then.


> I want to thank you also Max for your answer (We need to investigate more).

 If it doesn't work on gitlab-ci, I would just not enable the build of host-qemu
for those (and put some intelligence in the script that it only runs if
host-qemu has been built).


> On Tue, Apr 30, 2019 at 1:38 AM Max Filippov <jcmvbkbc@gmail.com
> <mailto:jcmvbkbc@gmail.com>> wrote:
> 
>     On Mon, Apr 29, 2019 at 3:32 PM Arnout Vandecappelle <arnout@mind.be
>     <mailto:arnout@mind.be>> wrote:
>     > > +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name ==
>     "xtensa-lx60-nommu" ]; then
>     > > +    echo "xtensa cannot be tested"
>     >
>     >  If that is the case, why do we even have these qemu defconfigs? Max?
> 
>     Tested both configurations just now, with the current buildroot tip
>     (2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
>     with QEMU command line from the readme.

 Is this with the system-intalled qemu? I guess so, because Buildroot still has
3.1.0... That might be the issue :-)

 Regards,
 Arnout

On Tue, Apr 30, 2019 at 4:20 AM Arnout Vandecappelle <arnout@mind.be> wrote:
> >     >  If that is the case, why do we even have these qemu defconfigs? Max?
> >
> >     Tested both configurations just now, with the current buildroot tip
> >     (2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
> >     with QEMU command line from the readme.
>
>  Is this with the system-intalled qemu? I guess so, because Buildroot still has
> 3.1.0... That might be the issue :-)

Just tested with that version of QEMU... Both work as expected here.
Still puzzled (:

Arnout, All,

On 2019-04-30 13:20 +0200, Arnout Vandecappelle spake thusly:
> On 30/04/2019 10:54, Jugurtha BELKALEM wrote:
> > Thank you for you comments Arnout, I have corrected the indentation, timeout
> > and  function declaration issues.
> > 
> > However; I think that duplicating the content of readme.txt of each qemu
> > architecture would be difficult to maintain (even if it is more convenient to
> > use), this is why a project like toolchain builder :
> > https://github.com/bootlin/toolchains-builder/blob/master/build.sh is reading
> > directly from readme.txt.
> 
>  The idea would not be to duplicate it, but to move it. I.e., readme.txt just
> says "launch with board/qemu/x86_64/launch.sh", and the actual command is in
> that shell script.
> 
>  But maybe we should see what other Buildroot maintainers have to say about it.
> Thomas, Peter, Yann, Romain?

Ideally, I'd go without the readme.txt altogether, or if we want to keep
"backward compatibility", we'd make it a symlink to launch.sh.

Snf then, launch.sh would be an actual shell script that also contains
the readme (possibly as -h).

> >     > > +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name ==
> >     "xtensa-lx60-nommu" ]; then
> >     > > +    echo "xtensa cannot be tested"
> >     >  If that is the case, why do we even have these qemu defconfigs? Max?
> >     Tested both configurations just now, with the current buildroot tip
> >     (2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
> >     with QEMU command line from the readme.
>  Is this with the system-intalled qemu? I guess so, because Buildroot still has
> 3.1.0... That might be the issue :-)

I've started a test-build ehre, to check.

Regards,
Yann E. MORIN.

Arnout, All,

On 2019-04-30 21:40 +0200, Yann E. MORIN spake thusly:
> On 2019-04-30 13:20 +0200, Arnout Vandecappelle spake thusly:
[--SNIP--]
> > >   > > +  elif [ $device_name == "xtensa-lx60" ] || [ $device_name ==
> > >   "xtensa-lx60-nommu" ]; then
> > >   > > +    echo "xtensa cannot be tested"
> > >   >  If that is the case, why do we even have these qemu defconfigs? Max?
> > >  Tested both configurations just now, with the current buildroot tip
> > >  (2019.02-959-gd54b0e22aef3) and QEMU-4.0. Both build and boot for me
> > >  with QEMU command line from the readme.
> >  Is this with the system-intalled qemu? I guess so, because Buildroot still has
> > 3.1.0... That might be the issue :-)
> 
> I've started a test-build ehre, to check.

And it does not build because:

      UIMAGE  arch/xtensa/boot/uImage
    "mkimage" command not found - U-Boot images will not be built
    arch/xtensa/boot/Makefile:51: recipe for target 'arch/xtensa/boot/uImage' failed

So, it needs to have host-uboot-tools.

Regards,
Yann E. MORIN.

On Tue, Apr 30, 2019 at 12:56 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> And it does not build because:
>
>       UIMAGE  arch/xtensa/boot/uImage
>     "mkimage" command not found - U-Boot images will not be built
>     arch/xtensa/boot/Makefile:51: recipe for target 'arch/xtensa/boot/uImage' failed

Ah, yes, I have mkimage somewhere in my PATH.

> So, it needs to have host-uboot-tools.

But I don't think this is the right thing. What I see is that despite
BR2_LINUX_KERNEL_IMAGE_TARGET_NAME is set to Image,
to only build self-contained ELF kernel, the initial kernel build is
run as 'make ... all', and only the build with the cpio is run as
'make ... Image'. Why?

On Tue, Apr 30, 2019 at 1:09 PM Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> On Tue, Apr 30, 2019 at 12:56 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > And it does not build because:
> >
> >       UIMAGE  arch/xtensa/boot/uImage
> >     "mkimage" command not found - U-Boot images will not be built
> >     arch/xtensa/boot/Makefile:51: recipe for target 'arch/xtensa/boot/uImage' failed
>
> Ah, yes, I have mkimage somewhere in my PATH.
>
> > So, it needs to have host-uboot-tools.
>
> But I don't think this is the right thing. What I see is that despite
> BR2_LINUX_KERNEL_IMAGE_TARGET_NAME is set to Image,
> to only build self-contained ELF kernel, the initial kernel build is
> run as 'make ... all', and only the build with the cpio is run as
> 'make ... Image'. Why?

Ok, I see why: because ffbe46a5295c ("linux: simplify LINUX_BUILD_CMDS").
It's a regression then.

Max, All,

On 2019-04-30 13:23 -0700, Max Filippov spake thusly:
> On Tue, Apr 30, 2019 at 1:09 PM Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 12:56 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > > And it does not build because:
> > >
> > >       UIMAGE  arch/xtensa/boot/uImage
> > >     "mkimage" command not found - U-Boot images will not be built
> > >     arch/xtensa/boot/Makefile:51: recipe for target 'arch/xtensa/boot/uImage' failed
> >
> > Ah, yes, I have mkimage somewhere in my PATH.
> >
> > > So, it needs to have host-uboot-tools.
> >
> > But I don't think this is the right thing. What I see is that despite
> > BR2_LINUX_KERNEL_IMAGE_TARGET_NAME is set to Image,
> > to only build self-contained ELF kernel, the initial kernel build is
> > run as 'make ... all', and only the build with the cpio is run as
> > 'make ... Image'. Why?
> 
> Ok, I see why: because ffbe46a5295c ("linux: simplify LINUX_BUILD_CMDS").
> It's a regression then.

See also 2a7cf511f4 (linux: split calling "all" and "$(LINUX_TARGET_NAME)"
targets) which is an attempt at fixing the regression in ffbe46a5295c.

Furthermore, there were a few patches adding host uboot tools to a few
defconfigs that needed it, like 7cbf9c63e5 (configs/qemu_ppc_virtex_ml507:
kernel build needs mkimage)

Finally, see also the discussion on that series:
    http://lists.busybox.net/pipermail/buildroot/2019-April/248424.html

We may have missed a few. I'll send a patch adding it to the extensa
defconfigs.

Regards,
Yann E. MORIN.

On 30/04/2019 21:40, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2019-04-30 13:20 +0200, Arnout Vandecappelle spake thusly:
>> On 30/04/2019 10:54, Jugurtha BELKALEM wrote:
>>> Thank you for you comments Arnout, I have corrected the indentation, timeout
>>> and  function declaration issues.
>>>
>>> However; I think that duplicating the content of readme.txt of each qemu
>>> architecture would be difficult to maintain (even if it is more convenient to
>>> use), this is why a project like toolchain builder :
>>> https://github.com/bootlin/toolchains-builder/blob/master/build.sh is reading
>>> directly from readme.txt.
>>  The idea would not be to duplicate it, but to move it. I.e., readme.txt just
>> says "launch with board/qemu/x86_64/launch.sh", and the actual command is in
>> that shell script.
>>
>>  But maybe we should see what other Buildroot maintainers have to say about it.
>> Thomas, Peter, Yann, Romain?
> Ideally, I'd go without the readme.txt altogether, or if we want to keep
> "backward compatibility", we'd make it a symlink to launch.sh.

 Sounds good to me. Most of those don't contain any information at all.

 Regards,
 Arnout