On 09/20/2017 06:41 PM, Stephen Warren wrote:
> On 09/19/2017 02:04 PM, Heinrich Schuchardt wrote:
>> The necessary parameters for running Python tests on qemu are
>> tedious to find.
>>
>> The patch adds a complete example for running the Python tests for
>> qemu-x86_defconfig on an X86 system.
>
>> diff --git a/test/py/README.md b/test/py/README.md
>
>> +#### Running tests for qemu-x86\_defconfig on an x86 system
>> +
>> +We build u-boot.rom with
>> +
>> + export BUILD_ROM=y
>> + make mrproper
>> + make qemu-x86_defconfig
>> + make
>
> Nit: I'd rather write that as:
>
> BUILD_ROM=y make
>
> That way, we don't have to set BUILD_ROM permanently in the current
> shell's environment, so it won't affect anything else that's done later
> in the shell.
>
> To avoid polluting the source tree with build results, why not build
> with O=./build-qemu-x86, and use that as the --build-dir argument to
> test/py?
Building in the source tree is the default for U-Boot.
Why should we make the example more complicated?
>
>> +We create directories `$HOME/ubtest/bin` and `$HOME/ubtest/py` for
>> the script
>> +files below and `../tftp` for the tftp server.
>
> Why not $HOME/ubtest/tftp? The meaning of "../tftp" can change depending
> on current directory.
I can give it a try.
>
>> +File `u-boot-test-console` chmod 755
>> +
>> + #!/bin/sh
>> + touch /tmp/u-boot-monitor-socket
>
> This creates a regular file not a socket. I believe you can remove this
> command.
>
> Ah, there's a race condition here. By the time u-boot-test-reset is run
> the first time, qemu typically hasn't run far enough to create
> /tmp/u-boot-monitor-socket, and so u-boot-test-reset fails to open it.
> If I fix that by making u-boot-test-reset wait until the control socket
> actually does exist, then test/py fails because it sees two signon
> messages from U-Boot and thinks it crashed; one from when qemu started
> and booted U-Boot, and one because the first invocation of
> u-boot-test-reset resets the system which causes another signon to
> appear. Perhaps this is why Tom Rini's test/py+qemu integration spawns a
> new instance of qemu every time u-boot-test-console is run, and make
> u-boot-test-reset a no-op.
>
> To solve this, I was going to say:
>
> a) Add -S to the qemu command. This causes qemu to perform all
> initialization (e.g. creating/opening the monitor socket) but doesn't
> actually allow the target to run. This prevents the undesired immediate
> boot of the system.
>
> b) Add the following to the top of u-boot-test-reset so that it waits
> for the monitor socket to exist before attempting to use it:
>
> for i in $(seq 50); do
> if [ -f /tmp/u-boot-monitor-socket ]; then
> break
> fi
> sleep 0.1
> done
Why make the example more complicated?
>
> c) Make u-boot-test-reset send U-Boot command "c" to start U-Boot
> executing instead of or in additionk to "system_reset" to cause a reset.
>
> However, none of that is necessary.
Some test scripts explicitly reset U-Boot:
test/py/tests/test_fit.py:362: cons.restart_uboot()
test/py/tests/test_fit.py:387: cons.restart_uboot()
test/py/tests/test_fit.py:399: cons.restart_uboot()
test/py/tests/test_fit.py:411: cons.restart_uboot()
test/py/tests/test_fit.py:428: cons.restart_uboot()
test/py/tests/test_vboot.py:70: cons.restart_uboot()
test/py/tests/test_vboot.py:198: cons.restart_uboot()
test/py/tests/test_efi_selftest.py:25: u_boot_console.restart_uboot();
> u-boot-test-console is re-executed
> every time test/py wants to connect to the target, which happens (a)
> when test/py starts the first test, and (b) any time a test fails, and
> test/py wants to clear all state and reconnect to the target (e.g. the
> console command might have failed rather than the target, so the console
> command is restarted too). As such, there's no need to implement
> u-boot-test-reset at all with qemu; just make it a complete no-op. That
> way, you also don't need qemu's -monitor at all.
>
>> + exec qemu-system-x86_64 -bios u-boot.rom -nographic -netdev \
>> +
>> user,id=eth0,tftp=../tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \
>> + -device e1000,netdev=eth0 -machine pc-i440fx-2.8 \
>
> Is machine version 2.8 strictly necessary? The qemu packaged in Ubuntu
> 14.04 doesn't support that version, which restricts the usefulness of
> the example. Can version 2.0 work for example? It seems to work fine for
> me. My qemu supports the following i440fx:
It is time to upgrade your Linux distribution ;)
I can change that to pc-i440fx-2.0.
>
> qemu-system-x86_64 -machine help | grep i440fx
> pc-i440fx-2.0 Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-1.4 Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-1.5 Standard PC (i440FX + PIIX, 1996)
> pc Ubuntu 14.04 PC (i440FX + PIIX, 1996) (alias of
> pc-i440fx-trusty)
> pc-i440fx-trusty Ubuntu 14.04 PC (i440FX + PIIX, 1996) (default)
> pc-i440fx-1.7 Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-1.5-saucy Standard PC (i440FX + PIIX, 1996) (alias of
> pc-i440fx-1.5-qemu-kvm)
> pc-i440fx-1.5-qemu-kvm Standard PC (i440FX + PIIX, 1996)
> pc-i440fx-1.6 Standard PC (i440FX + PIIX, 1996)
>
>> +File `u-boot-test-quit` chmod 755
>> +
>> + #!/bin/sh
>> + echo quit | socat - UNIX-CONNECT:/tmp/u-boot-monitor-socket
>> +
>> +This script is called when all tests are completed. The `quit`
>> command is
>> +passed to the qemu control console to terminate the qemu session.
>
> test/py doesn't run u-boot-test-quit. Do you have a local change that
> makes it do that? My knee-jerk reaction is that it's OK to enhance
> test/py to run such a script, but let's not document it in the example
> until it does. That said, I'd assume qemu should exit as soon as test/py
> exits since stdin will go away, so this shouldn't actually be needed in
> this case.
>
I will remove u-boot-test-quit.
>> +File `u-boot-test-reset` chmod 755
>> +
>> + #!/bin/sh
>> + echo system_reset | socat - UNIX-CONNECT:/tmp/u-boot-monitor-socket
>> + true
>
> As mentioned previously, the true doesn't appear to be required, and
> hides legitimate errors.
Without true all tests fail on my machine (Debian Stretch) which uses
dash to implement /bin/sh. So we should keep it. It does no harm.
>
>> +In the `$HOME/ubtest/py` directory we create file
>> `u_boot_boardenv_qemu_x86.py`
>
> Since the file mode is mentioned for all the other files, let's mention
> it here too.
644
>
>> + env__net_dhcp_server = True
>> + env__efi_loader_helloworld_file = {
>> + "fn": "helloworld.efi",
>> + "size": 4298,
>> + "crc32": "55d96ef8",
>> + }
>> +
>> +The parameter `env__net_dhcp_server` enables the network tests. The
>> parameter
>> +`env__efi_loader_helloworld_file` is needed to make the file
>> `helloworld.efi`
>> +available which is loaded from the tftp server in `test_efi_loader.py`.
>> +
>> +The fields size and crc32 have to be updated to match the actual
>> values. The
>> +`crc32` command can be used to determine the latter.
>
> Why not have Python determine those values automatically? Here's a
> snippet from my uboot-test-hooks repo that does that. Since people will
> simply be copy-and-pasting the example, it shouldn't matter that it's
> non-trivial:
>
> {
> "fn": file_name,
> "size": os.path.getsize(file_full),
> "crc32": hex(binascii.crc32(open(file_full, 'rb').read()) & \
> 0xffffffff)[2:],
> }
What does file_full refer to? I cannot find this string in U-Boot.
Are the necessary packages imported when the python script is included?
I doubt it. There is no string binascii in U-Boot.
We cannot hard code the full path to helloworld.efi here because the
value of $HOME_PATH is not known.
I would prefer to keep the example simple.
>
>> +We now can run the Python tests with
>> +
>> + export PATH=$HOME/ubtest/bin:/usr/bin:/bin
>
> Let's not hard-code /usr/bin:/bin; some people may rely on binaries
> elsewhere. Also I know that the recently added gpt tests rely on sgdisk
> which is in sbin on my system at least. Also quote to avoid issues with
> spaces in path names. How about:
>
> PATH="$HOME/ubtest/bin:$PATH"
I can change it.
>
>> + export PYTHONPATH=$HOME/ubtest/py
>
> People might have PYTHONPATH set already (e.g. if using a virtualenv or
> having installed some packages in their home directory). How about:
>
> PYTHONPATH="$HOME/ubtest/py:$PYTHONPATH"
>
Together with your suggestion below this is fine.
>> + ./test/py/test.py --bd=qemu-x86 --build-dir=.
>
> As above, I'd suggest setting environment variables just for the one
> command rather than permanently changing them:
>
> PATH="$HOME/ubtest/bin:$PATH" \
> PYTHONPATH="$HOME/ubtest/py:$PYTHONPATH" \
> ./test/py/test.py --bd=qemu-x86 --build-dir=./build-qemu-x86
>
That makes sense.
Thank you for reviewing.
Best regards
Heinrich
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
