[v3,08/19] test: Introduce the concept of a role

Message ID	20240623203213.1571666-9-sjg@chromium.org
State	Changes Requested
Delegated to:	Tom Rini
Headers	show Return-Path: <u-boot-bounces@lists.denx.de> From: Simon Glass <sjg@chromium.org> To: U-Boot Mailing List <u-boot@lists.denx.de> Cc: Tom Rini <trini@konsulko.com>, Simon Glass <sjg@chromium.org> Subject: [PATCH v3 08/19] test: Introduce the concept of a role Date: Sun, 23 Jun 2024 14:32:02 -0600 Message-Id: <20240623203213.1571666-9-sjg@chromium.org> In-Reply-To: <20240623203213.1571666-1-sjg@chromium.org> References: <20240623203213.1571666-1-sjg@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" <u-boot-bounces@lists.denx.de>
Series	labgrid: Provide an integration with Labgrid \| expand [v3,00/19] labgrid: Provide an integration with Labgrid [v3,01/19] test: Allow signaling that U-Boot is ready [v3,02/19] test: Use a constant for the test timeout [v3,03/19] test: Pass stderr to stdout [v3,04/19] test: Release board after tests complete [v3,05/19] test: Allow connecting to a running board [v3,06/19] test: Avoid failing skipped tests [v3,07/19] test: Create a common function to get the config [v3,08/19] test: Introduce the concept of a role [v3,09/19] test: Move the receive code into a function [v3,10/19] test: Separate out the exception handling [v3,11/19] test: Detect dead connections [v3,12/19] test: Tidy up remaining exceptions [v3,13/19] test: Introduce lab mode [v3,14/19] test: Improve handling of sending commands [v3,15/19] test: Fix mulptiplex_log typo [v3,16/19] test: Avoid double echo when starting up [v3,17/19] test: Try to shut down the lab console gracefully [v3,18/19] test: Add a section for closing the connection [v3,19/19] CI: Allow running tests on sjg lab

Simon Glass June 23, 2024, 8:32 p.m. UTC

In Labgrid there is the concept of a 'role', which is similar to the
U-Boot board ID in U-Boot's pytest subsystem.

The role indicates both the target and information about the U-Boot
build to use. It can also provide any amount of other configuration.
The information is obtained using the 'labgrid-client query' operation.

Make use of this in tests, so that only the role is required in gitlab
and other situations. The board type and other things can be queried
as needed.

Use a new 'u-boot-test-getrole' script to obtain the requested
information.

With this it is possible to run lab tests in gitlab with just a single
'ROLE' variable for each board.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 test/py/conftest.py | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

Tom Rini June 24, 2024, 6:13 p.m. UTC | #1

On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:

> In Labgrid there is the concept of a 'role', which is similar to the
> U-Boot board ID in U-Boot's pytest subsystem.
> 
> The role indicates both the target and information about the U-Boot
> build to use. It can also provide any amount of other configuration.
> The information is obtained using the 'labgrid-client query' operation.
> 
> Make use of this in tests, so that only the role is required in gitlab
> and other situations. The board type and other things can be queried
> as needed.
> 
> Use a new 'u-boot-test-getrole' script to obtain the requested
> information.
> 
> With this it is possible to run lab tests in gitlab with just a single
> 'ROLE' variable for each board.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/test/py/conftest.py b/test/py/conftest.py
> index 6547c6922c6..5de8d7b0e23 100644
> --- a/test/py/conftest.py
> +++ b/test/py/conftest.py
> @@ -23,6 +23,7 @@ from pathlib import Path
>  import pytest
>  import re
>  from _pytest.runner import runtestprotocol
> +import subprocess
>  import sys
>  
>  # Globals: The HTML log file, and the connection to the U-Boot console.
> @@ -79,6 +80,7 @@ def pytest_addoption(parser):
>      parser.addoption('--gdbserver', default=None,
>          help='Run sandbox under gdbserver. The argument is the channel '+
>          'over which gdbserver should communicate, e.g. localhost:1234')
> +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
>      parser.addoption('--no-prompt-wait', default=False, action='store_true',
>          help="Assume that U-Boot is ready and don't wait for a prompt")
>  
> @@ -130,12 +132,33 @@ def get_details(config):
>              str: Build directory
>              str: Source directory
>      """
> -    board_type = config.getoption('board_type')
> -    board_identity = config.getoption('board_identity')
> +    role = config.getoption('role')
>      build_dir = config.getoption('build_dir')
> +    if role:
> +        board_identity = role
> +        cmd = ['u-boot-test-getrole', role, '--configure']
> +        env = os.environ.copy()
> +        if build_dir:
> +            env['U_BOOT_BUILD_DIR'] = build_dir
> +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> +                              env=env)
> +        if proc.returncode:
> +            raise ValueError(proc.stderr)
> +        print('conftest: lab:', proc.stdout)
> +        vals = {}
> +        for line in proc.stdout.splitlines():
> +            item, value = line.split(' ', maxsplit=1)
> +            k = item.split(':')[-1]
> +            vals[k] = value
> +        print('conftest: lab info:', vals)
> +        board_type, default_build_dir, source_dir = (vals['board'],
> +            vals['build_dir'], vals['source_dir'])
> +    else:
> +        board_type = config.getoption('board_type')
> +        board_identity = config.getoption('board_identity')
>  
> -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> -    default_build_dir = source_dir + '/build-' + board_type
> +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> +        default_build_dir = source_dir + '/build-' + board_type
>      if not build_dir:
>          build_dir = default_build_dir

I'm a little confused here. Why can't we construct "role" from
board_type+board_identity and then we have the board
conf.${board_type}_${board_identity} file set whatever needs setting to
be "labgrid" ?

Simon Glass June 25, 2024, 12:38 p.m. UTC | #2

Hi Tom,

On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
>
> > In Labgrid there is the concept of a 'role', which is similar to the
> > U-Boot board ID in U-Boot's pytest subsystem.
> >
> > The role indicates both the target and information about the U-Boot
> > build to use. It can also provide any amount of other configuration.
> > The information is obtained using the 'labgrid-client query' operation.
> >
> > Make use of this in tests, so that only the role is required in gitlab
> > and other situations. The board type and other things can be queried
> > as needed.
> >
> > Use a new 'u-boot-test-getrole' script to obtain the requested
> > information.
> >
> > With this it is possible to run lab tests in gitlab with just a single
> > 'ROLE' variable for each board.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > index 6547c6922c6..5de8d7b0e23 100644
> > --- a/test/py/conftest.py
> > +++ b/test/py/conftest.py
> > @@ -23,6 +23,7 @@ from pathlib import Path
> >  import pytest
> >  import re
> >  from _pytest.runner import runtestprotocol
> > +import subprocess
> >  import sys
> >
> >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> >      parser.addoption('--gdbserver', default=None,
> >          help='Run sandbox under gdbserver. The argument is the channel '+
> >          'over which gdbserver should communicate, e.g. localhost:1234')
> > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> >          help="Assume that U-Boot is ready and don't wait for a prompt")
> >
> > @@ -130,12 +132,33 @@ def get_details(config):
> >              str: Build directory
> >              str: Source directory
> >      """
> > -    board_type = config.getoption('board_type')
> > -    board_identity = config.getoption('board_identity')
> > +    role = config.getoption('role')
> >      build_dir = config.getoption('build_dir')
> > +    if role:
> > +        board_identity = role
> > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > +        env = os.environ.copy()
> > +        if build_dir:
> > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > +                              env=env)
> > +        if proc.returncode:
> > +            raise ValueError(proc.stderr)
> > +        print('conftest: lab:', proc.stdout)
> > +        vals = {}
> > +        for line in proc.stdout.splitlines():
> > +            item, value = line.split(' ', maxsplit=1)
> > +            k = item.split(':')[-1]
> > +            vals[k] = value
> > +        print('conftest: lab info:', vals)
> > +        board_type, default_build_dir, source_dir = (vals['board'],
> > +            vals['build_dir'], vals['source_dir'])
> > +    else:
> > +        board_type = config.getoption('board_type')
> > +        board_identity = config.getoption('board_identity')
> >
> > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > -    default_build_dir = source_dir + '/build-' + board_type
> > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > +        default_build_dir = source_dir + '/build-' + board_type
> >      if not build_dir:
> >          build_dir = default_build_dir
>
> I'm a little confused here. Why can't we construct "role" from
> board_type+board_identity and then we have the board
> conf.${board_type}_${board_identity} file set whatever needs setting to
> be "labgrid" ?

The role is equivalent to the board identity, not the combination of
the U-Boot board type and the board identity. I went this way to avoid
having to type long and complicated roles when connecting to boards.
It is similar to how things work today, except that the board type is
implied by the 'role'.

For boards which have multiple identities (e.g. can support two
different board types), Labgrid handles acquiring and releasing the
shares resources, to avoid any problems.

Regards,
Simon

Tom Rini June 25, 2024, 2:27 p.m. UTC | #3

On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> >
> > > In Labgrid there is the concept of a 'role', which is similar to the
> > > U-Boot board ID in U-Boot's pytest subsystem.
> > >
> > > The role indicates both the target and information about the U-Boot
> > > build to use. It can also provide any amount of other configuration.
> > > The information is obtained using the 'labgrid-client query' operation.
> > >
> > > Make use of this in tests, so that only the role is required in gitlab
> > > and other situations. The board type and other things can be queried
> > > as needed.
> > >
> > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > information.
> > >
> > > With this it is possible to run lab tests in gitlab with just a single
> > > 'ROLE' variable for each board.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > index 6547c6922c6..5de8d7b0e23 100644
> > > --- a/test/py/conftest.py
> > > +++ b/test/py/conftest.py
> > > @@ -23,6 +23,7 @@ from pathlib import Path
> > >  import pytest
> > >  import re
> > >  from _pytest.runner import runtestprotocol
> > > +import subprocess
> > >  import sys
> > >
> > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > >      parser.addoption('--gdbserver', default=None,
> > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > >
> > > @@ -130,12 +132,33 @@ def get_details(config):
> > >              str: Build directory
> > >              str: Source directory
> > >      """
> > > -    board_type = config.getoption('board_type')
> > > -    board_identity = config.getoption('board_identity')
> > > +    role = config.getoption('role')
> > >      build_dir = config.getoption('build_dir')
> > > +    if role:
> > > +        board_identity = role
> > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > +        env = os.environ.copy()
> > > +        if build_dir:
> > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > +                              env=env)
> > > +        if proc.returncode:
> > > +            raise ValueError(proc.stderr)
> > > +        print('conftest: lab:', proc.stdout)
> > > +        vals = {}
> > > +        for line in proc.stdout.splitlines():
> > > +            item, value = line.split(' ', maxsplit=1)
> > > +            k = item.split(':')[-1]
> > > +            vals[k] = value
> > > +        print('conftest: lab info:', vals)
> > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > +            vals['build_dir'], vals['source_dir'])
> > > +    else:
> > > +        board_type = config.getoption('board_type')
> > > +        board_identity = config.getoption('board_identity')
> > >
> > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > -    default_build_dir = source_dir + '/build-' + board_type
> > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > +        default_build_dir = source_dir + '/build-' + board_type
> > >      if not build_dir:
> > >          build_dir = default_build_dir
> >
> > I'm a little confused here. Why can't we construct "role" from
> > board_type+board_identity and then we have the board
> > conf.${board_type}_${board_identity} file set whatever needs setting to
> > be "labgrid" ?
> 
> The role is equivalent to the board identity, not the combination of
> the U-Boot board type and the board identity. I went this way to avoid
> having to type long and complicated roles when connecting to boards.
> It is similar to how things work today, except that the board type is
> implied by the 'role'.
> 
> For boards which have multiple identities (e.g. can support two
> different board types), Labgrid handles acquiring and releasing the
> shares resources, to avoid any problems.

I guess I have two sets of questions. First, if it's basically the
board identity why can't we just use that as the role name, in your
setup?

But second, I'm not sure why we need this. The labgrid support we worked
up here (but, sigh, it's not really clean enough to post) has:
$ cat bin/lootbox/conf.rpi_4_na
console_impl=labgrid
reset_impl=labgrid
flash_impl=labgrid.rpi_4
flash_writer=labgrid.rpi_4

For example and:
$ cat bin/writer.labgrid.rpi_4
set -e

build=${U_BOOT_BUILD_DIR}

$LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img

So I don't know what the "role" part you have is for. And yes, there end
up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
(such as beagleplay) we have:
$ cat bin/writer.labgrid.ti-k3
set -e
build=${U_BOOT_BUILD_DIR}

if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
    echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
    echo "per the board documentation."
    exit 1
fi
echo "Writing build at ${build}"
$LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
$LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
$LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
echo "Done writing build"

In all cases we first build U-Boot and then pass the build directory to
test.py, in something like:
export LG_PLACE=rpi4
./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
  --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
  --exitfirst

The only U-Boot side changes I needed to make were for counting the SPL
banner instances, and that was regardless of framework (a particularly
fun platform will show it 3 times).

Simon Glass June 26, 2024, 8 a.m. UTC | #4

Hi Tom,

On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > >
> > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > >
> > > > The role indicates both the target and information about the U-Boot
> > > > build to use. It can also provide any amount of other configuration.
> > > > The information is obtained using the 'labgrid-client query' operation.
> > > >
> > > > Make use of this in tests, so that only the role is required in gitlab
> > > > and other situations. The board type and other things can be queried
> > > > as needed.
> > > >
> > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > information.
> > > >
> > > > With this it is possible to run lab tests in gitlab with just a single
> > > > 'ROLE' variable for each board.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > --- a/test/py/conftest.py
> > > > +++ b/test/py/conftest.py
> > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > >  import pytest
> > > >  import re
> > > >  from _pytest.runner import runtestprotocol
> > > > +import subprocess
> > > >  import sys
> > > >
> > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > >      parser.addoption('--gdbserver', default=None,
> > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > >
> > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > >              str: Build directory
> > > >              str: Source directory
> > > >      """
> > > > -    board_type = config.getoption('board_type')
> > > > -    board_identity = config.getoption('board_identity')
> > > > +    role = config.getoption('role')
> > > >      build_dir = config.getoption('build_dir')
> > > > +    if role:
> > > > +        board_identity = role
> > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > +        env = os.environ.copy()
> > > > +        if build_dir:
> > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > +                              env=env)
> > > > +        if proc.returncode:
> > > > +            raise ValueError(proc.stderr)
> > > > +        print('conftest: lab:', proc.stdout)
> > > > +        vals = {}
> > > > +        for line in proc.stdout.splitlines():
> > > > +            item, value = line.split(' ', maxsplit=1)
> > > > +            k = item.split(':')[-1]
> > > > +            vals[k] = value
> > > > +        print('conftest: lab info:', vals)
> > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > +            vals['build_dir'], vals['source_dir'])
> > > > +    else:
> > > > +        board_type = config.getoption('board_type')
> > > > +        board_identity = config.getoption('board_identity')
> > > >
> > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > >      if not build_dir:
> > > >          build_dir = default_build_dir
> > >
> > > I'm a little confused here. Why can't we construct "role" from
> > > board_type+board_identity and then we have the board
> > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > be "labgrid" ?
> >
> > The role is equivalent to the board identity, not the combination of
> > the U-Boot board type and the board identity. I went this way to avoid
> > having to type long and complicated roles when connecting to boards.
> > It is similar to how things work today, except that the board type is
> > implied by the 'role'.
> >
> > For boards which have multiple identities (e.g. can support two
> > different board types), Labgrid handles acquiring and releasing the
> > shares resources, to avoid any problems.
>
> I guess I have two sets of questions. First, if it's basically the
> board identity why can't we just use that as the role name, in your
> setup?

Yes, that's what I am doing. If you look in console.labgrid you can
see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.

>
> But second, I'm not sure why we need this. The labgrid support we worked
> up here (but, sigh, it's not really clean enough to post) has:
> $ cat bin/lootbox/conf.rpi_4_na
> console_impl=labgrid
> reset_impl=labgrid
> flash_impl=labgrid.rpi_4
> flash_writer=labgrid.rpi_4
>
> For example and:
> $ cat bin/writer.labgrid.rpi_4
> set -e
>
> build=${U_BOOT_BUILD_DIR}
>
> $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
>
> So I don't know what the "role" part you have is for. And yes, there end
> up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> (such as beagleplay) we have:
> $ cat bin/writer.labgrid.ti-k3
> set -e
> build=${U_BOOT_BUILD_DIR}
>
> if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
>     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
>     echo "per the board documentation."
>     exit 1
> fi
> echo "Writing build at ${build}"
> $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> echo "Done writing build"
>
> In all cases we first build U-Boot and then pass the build directory to
> test.py, in something like:
> export LG_PLACE=rpi4
> ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
>   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
>   --exitfirst
>
> The only U-Boot side changes I needed to make were for counting the SPL
> banner instances, and that was regardless of framework (a particularly
> fun platform will show it 3 times).

Yes it is possible to build U-Boot separately. Of course that involved
various blobs and so on, so every board is different. The approach I
have taken here is to have Labgrid call buildman to build what is
needed, with the blobs defined in the environment.

You can use the -B flag to use a pre-built U-Boot, with the scripts
I've included.

Regards,
Simon

Tom Rini June 26, 2024, 2:29 p.m. UTC | #5

On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > >
> > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > >
> > > > > The role indicates both the target and information about the U-Boot
> > > > > build to use. It can also provide any amount of other configuration.
> > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > >
> > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > and other situations. The board type and other things can be queried
> > > > > as needed.
> > > > >
> > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > information.
> > > > >
> > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > 'ROLE' variable for each board.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > --- a/test/py/conftest.py
> > > > > +++ b/test/py/conftest.py
> > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > >  import pytest
> > > > >  import re
> > > > >  from _pytest.runner import runtestprotocol
> > > > > +import subprocess
> > > > >  import sys
> > > > >
> > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > >      parser.addoption('--gdbserver', default=None,
> > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > >
> > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > >              str: Build directory
> > > > >              str: Source directory
> > > > >      """
> > > > > -    board_type = config.getoption('board_type')
> > > > > -    board_identity = config.getoption('board_identity')
> > > > > +    role = config.getoption('role')
> > > > >      build_dir = config.getoption('build_dir')
> > > > > +    if role:
> > > > > +        board_identity = role
> > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > +        env = os.environ.copy()
> > > > > +        if build_dir:
> > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > +                              env=env)
> > > > > +        if proc.returncode:
> > > > > +            raise ValueError(proc.stderr)
> > > > > +        print('conftest: lab:', proc.stdout)
> > > > > +        vals = {}
> > > > > +        for line in proc.stdout.splitlines():
> > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > +            k = item.split(':')[-1]
> > > > > +            vals[k] = value
> > > > > +        print('conftest: lab info:', vals)
> > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > +    else:
> > > > > +        board_type = config.getoption('board_type')
> > > > > +        board_identity = config.getoption('board_identity')
> > > > >
> > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > >      if not build_dir:
> > > > >          build_dir = default_build_dir
> > > >
> > > > I'm a little confused here. Why can't we construct "role" from
> > > > board_type+board_identity and then we have the board
> > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > be "labgrid" ?
> > >
> > > The role is equivalent to the board identity, not the combination of
> > > the U-Boot board type and the board identity. I went this way to avoid
> > > having to type long and complicated roles when connecting to boards.
> > > It is similar to how things work today, except that the board type is
> > > implied by the 'role'.
> > >
> > > For boards which have multiple identities (e.g. can support two
> > > different board types), Labgrid handles acquiring and releasing the
> > > shares resources, to avoid any problems.
> >
> > I guess I have two sets of questions. First, if it's basically the
> > board identity why can't we just use that as the role name, in your
> > setup?
> 
> Yes, that's what I am doing. If you look in console.labgrid you can
> see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.

Then why do we need this?

> > But second, I'm not sure why we need this. The labgrid support we worked
> > up here (but, sigh, it's not really clean enough to post) has:
> > $ cat bin/lootbox/conf.rpi_4_na
> > console_impl=labgrid
> > reset_impl=labgrid
> > flash_impl=labgrid.rpi_4
> > flash_writer=labgrid.rpi_4
> >
> > For example and:
> > $ cat bin/writer.labgrid.rpi_4
> > set -e
> >
> > build=${U_BOOT_BUILD_DIR}
> >
> > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> >
> > So I don't know what the "role" part you have is for. And yes, there end
> > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > (such as beagleplay) we have:
> > $ cat bin/writer.labgrid.ti-k3
> > set -e
> > build=${U_BOOT_BUILD_DIR}
> >
> > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> >     echo "per the board documentation."
> >     exit 1
> > fi
> > echo "Writing build at ${build}"
> > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > echo "Done writing build"
> >
> > In all cases we first build U-Boot and then pass the build directory to
> > test.py, in something like:
> > export LG_PLACE=rpi4
> > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> >   --exitfirst
> >
> > The only U-Boot side changes I needed to make were for counting the SPL
> > banner instances, and that was regardless of framework (a particularly
> > fun platform will show it 3 times).
> 
> Yes it is possible to build U-Boot separately. Of course that involved
> various blobs and so on, so every board is different. The approach I
> have taken here is to have Labgrid call buildman to build what is
> needed, with the blobs defined in the environment.
> 
> You can use the -B flag to use a pre-built U-Boot, with the scripts
> I've included.

OK. I personally think pre-built (or outside of buildman built) U-Boot
will be an important case, since everything needs more options enabled
to test more features, but on real hardware. For example,
CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
issues are resolved. And I need to add CONFIG_FIT to some platforms so
that I can then use the kernel test. And some platforms I end up
enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).

Simon Glass June 27, 2024, 8:37 a.m. UTC | #6

Hi Tom,

On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > >
> > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > >
> > > > > > The role indicates both the target and information about the U-Boot
> > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > >
> > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > and other situations. The board type and other things can be queried
> > > > > > as needed.
> > > > > >
> > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > information.
> > > > > >
> > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > 'ROLE' variable for each board.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > --- a/test/py/conftest.py
> > > > > > +++ b/test/py/conftest.py
> > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > >  import pytest
> > > > > >  import re
> > > > > >  from _pytest.runner import runtestprotocol
> > > > > > +import subprocess
> > > > > >  import sys
> > > > > >
> > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > >
> > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > >              str: Build directory
> > > > > >              str: Source directory
> > > > > >      """
> > > > > > -    board_type = config.getoption('board_type')
> > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > +    role = config.getoption('role')
> > > > > >      build_dir = config.getoption('build_dir')
> > > > > > +    if role:
> > > > > > +        board_identity = role
> > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > +        env = os.environ.copy()
> > > > > > +        if build_dir:
> > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > +                              env=env)
> > > > > > +        if proc.returncode:
> > > > > > +            raise ValueError(proc.stderr)
> > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > +        vals = {}
> > > > > > +        for line in proc.stdout.splitlines():
> > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > +            k = item.split(':')[-1]
> > > > > > +            vals[k] = value
> > > > > > +        print('conftest: lab info:', vals)
> > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > +    else:
> > > > > > +        board_type = config.getoption('board_type')
> > > > > > +        board_identity = config.getoption('board_identity')
> > > > > >
> > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > >      if not build_dir:
> > > > > >          build_dir = default_build_dir
> > > > >
> > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > board_type+board_identity and then we have the board
> > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > be "labgrid" ?
> > > >
> > > > The role is equivalent to the board identity, not the combination of
> > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > having to type long and complicated roles when connecting to boards.
> > > > It is similar to how things work today, except that the board type is
> > > > implied by the 'role'.
> > > >
> > > > For boards which have multiple identities (e.g. can support two
> > > > different board types), Labgrid handles acquiring and releasing the
> > > > shares resources, to avoid any problems.
> > >
> > > I guess I have two sets of questions. First, if it's basically the
> > > board identity why can't we just use that as the role name, in your
> > > setup?
> >
> > Yes, that's what I am doing. If you look in console.labgrid you can
> > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
>
> Then why do we need this?

We need to pass a role to Labgrid, since it determines the board
identity to use. It also (indirectly) determines the U-Boot build to
use, since each board identity / role is a particular board with a
particular build.

For example:
role / identify = samus   - uses 'samus' board with build chromebook_samus
role / identify = samus_tpl   - uses 'samus' board with build
chromebook_samus_tpl

Basically, as I understand it, the 'role' is the thing we want.

Labgrid environment:

  samus:
    resources:
      RemotePlace:
        name: samus
...
      UBootProviderDriver:
        board: chromebook_samus
        binman_indir: /vid/software/devel/samus/bin

  samus_tpl:
    resources:
      RemotePlace:
        name: samus
      UBootProviderDriver:
        board: chromebook_samus_tpl
        binman_indir: /vid/software/devel/samus/bin

>
> > > But second, I'm not sure why we need this. The labgrid support we worked
> > > up here (but, sigh, it's not really clean enough to post) has:
> > > $ cat bin/lootbox/conf.rpi_4_na
> > > console_impl=labgrid
> > > reset_impl=labgrid
> > > flash_impl=labgrid.rpi_4
> > > flash_writer=labgrid.rpi_4
> > >
> > > For example and:
> > > $ cat bin/writer.labgrid.rpi_4
> > > set -e
> > >
> > > build=${U_BOOT_BUILD_DIR}
> > >
> > > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> > >
> > > So I don't know what the "role" part you have is for. And yes, there end
> > > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > > (such as beagleplay) we have:
> > > $ cat bin/writer.labgrid.ti-k3
> > > set -e
> > > build=${U_BOOT_BUILD_DIR}
> > >
> > > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> > >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> > >     echo "per the board documentation."
> > >     exit 1
> > > fi
> > > echo "Writing build at ${build}"
> > > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > > echo "Done writing build"
> > >
> > > In all cases we first build U-Boot and then pass the build directory to
> > > test.py, in something like:
> > > export LG_PLACE=rpi4
> > > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> > >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> > >   --exitfirst
> > >
> > > The only U-Boot side changes I needed to make were for counting the SPL
> > > banner instances, and that was regardless of framework (a particularly
> > > fun platform will show it 3 times).
> >
> > Yes it is possible to build U-Boot separately. Of course that involved
> > various blobs and so on, so every board is different. The approach I
> > have taken here is to have Labgrid call buildman to build what is
> > needed, with the blobs defined in the environment.
> >
> > You can use the -B flag to use a pre-built U-Boot, with the scripts
> > I've included.
>
> OK. I personally think pre-built (or outside of buildman built) U-Boot
> will be an important case, since everything needs more options enabled
> to test more features, but on real hardware. For example,
> CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
> issues are resolved. And I need to add CONFIG_FIT to some platforms so
> that I can then use the kernel test. And some platforms I end up
> enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
> CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).

Yes that all sounds good. I have figured out how to add QEMU into this
Labgrid integration, but I cannot Debian to boot all the way to a
prompt with -nographic unless I pass something special on the Linux
commandline. So for now I parked that.

Regards,
Simon

Tom Rini July 2, 2024, 11:12 p.m. UTC | #7

On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > >
> > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > >
> > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > >
> > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > and other situations. The board type and other things can be queried
> > > > > > > as needed.
> > > > > > >
> > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > information.
> > > > > > >
> > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > 'ROLE' variable for each board.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > --- a/test/py/conftest.py
> > > > > > > +++ b/test/py/conftest.py
> > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > >  import pytest
> > > > > > >  import re
> > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > +import subprocess
> > > > > > >  import sys
> > > > > > >
> > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > >
> > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > >              str: Build directory
> > > > > > >              str: Source directory
> > > > > > >      """
> > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > +    role = config.getoption('role')
> > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > +    if role:
> > > > > > > +        board_identity = role
> > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > +        env = os.environ.copy()
> > > > > > > +        if build_dir:
> > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > +                              env=env)
> > > > > > > +        if proc.returncode:
> > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > +        vals = {}
> > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > +            k = item.split(':')[-1]
> > > > > > > +            vals[k] = value
> > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > +    else:
> > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > >
> > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > >      if not build_dir:
> > > > > > >          build_dir = default_build_dir
> > > > > >
> > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > board_type+board_identity and then we have the board
> > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > be "labgrid" ?
> > > > >
> > > > > The role is equivalent to the board identity, not the combination of
> > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > having to type long and complicated roles when connecting to boards.
> > > > > It is similar to how things work today, except that the board type is
> > > > > implied by the 'role'.
> > > > >
> > > > > For boards which have multiple identities (e.g. can support two
> > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > shares resources, to avoid any problems.
> > > >
> > > > I guess I have two sets of questions. First, if it's basically the
> > > > board identity why can't we just use that as the role name, in your
> > > > setup?
> > >
> > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> >
> > Then why do we need this?
> 
> We need to pass a role to Labgrid, since it determines the board
> identity to use. It also (indirectly) determines the U-Boot build to
> use, since each board identity / role is a particular board with a
> particular build.

Oh, I get where you're coming from now at least. But this still sounds
like a detail to put in to the conf.${board}_${board_type} file and not
a thing to set here?

> For example:
> role / identify = samus   - uses 'samus' board with build chromebook_samus
> role / identify = samus_tpl   - uses 'samus' board with build
> chromebook_samus_tpl

Yes, or using one physical Pi 4 to test rpi_4_defconfig and
rpi_arm64_defconfig (and if labgrid supported a way to remove files from
the FAT partition, rpi_4_32b_defconfig).

> Basically, as I understand it, the 'role' is the thing we want.
> 
> Labgrid environment:
> 
>   samus:
>     resources:
>       RemotePlace:
>         name: samus
> ...
>       UBootProviderDriver:
>         board: chromebook_samus
>         binman_indir: /vid/software/devel/samus/bin
> 
>   samus_tpl:
>     resources:
>       RemotePlace:
>         name: samus
>       UBootProviderDriver:
>         board: chromebook_samus_tpl
>         binman_indir: /vid/software/devel/samus/bin

I guess the problem here is that from my point of view, this can live in
the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
going to worry about building U-Boot (even if blobs aren't a problem, we
want to enable more features to test more things on HW) but from your
point of view, buildman must provide test.py with the correct build so
we need to know things prior.

> > > > But second, I'm not sure why we need this. The labgrid support we worked
> > > > up here (but, sigh, it's not really clean enough to post) has:
> > > > $ cat bin/lootbox/conf.rpi_4_na
> > > > console_impl=labgrid
> > > > reset_impl=labgrid
> > > > flash_impl=labgrid.rpi_4
> > > > flash_writer=labgrid.rpi_4
> > > >
> > > > For example and:
> > > > $ cat bin/writer.labgrid.rpi_4
> > > > set -e
> > > >
> > > > build=${U_BOOT_BUILD_DIR}
> > > >
> > > > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> > > >
> > > > So I don't know what the "role" part you have is for. And yes, there end
> > > > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > > > (such as beagleplay) we have:
> > > > $ cat bin/writer.labgrid.ti-k3
> > > > set -e
> > > > build=${U_BOOT_BUILD_DIR}
> > > >
> > > > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> > > >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> > > >     echo "per the board documentation."
> > > >     exit 1
> > > > fi
> > > > echo "Writing build at ${build}"
> > > > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > > > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > > > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > > > echo "Done writing build"
> > > >
> > > > In all cases we first build U-Boot and then pass the build directory to
> > > > test.py, in something like:
> > > > export LG_PLACE=rpi4
> > > > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> > > >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> > > >   --exitfirst
> > > >
> > > > The only U-Boot side changes I needed to make were for counting the SPL
> > > > banner instances, and that was regardless of framework (a particularly
> > > > fun platform will show it 3 times).
> > >
> > > Yes it is possible to build U-Boot separately. Of course that involved
> > > various blobs and so on, so every board is different. The approach I
> > > have taken here is to have Labgrid call buildman to build what is
> > > needed, with the blobs defined in the environment.
> > >
> > > You can use the -B flag to use a pre-built U-Boot, with the scripts
> > > I've included.
> >
> > OK. I personally think pre-built (or outside of buildman built) U-Boot
> > will be an important case, since everything needs more options enabled
> > to test more features, but on real hardware. For example,
> > CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
> > issues are resolved. And I need to add CONFIG_FIT to some platforms so
> > that I can then use the kernel test. And some platforms I end up
> > enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
> > CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).
> 
> Yes that all sounds good. I have figured out how to add QEMU into this
> Labgrid integration, but I cannot Debian to boot all the way to a
> prompt with -nographic unless I pass something special on the Linux
> commandline. So for now I parked that.

Putting QEMU in to labgrid too could be interesting, yes. But I lost how
it's related? To be clear, today we can test boot a Linux kernel on
hardware. Somewhere on my TODO list is cycling over what kernel images
to grab and shove in to the docker container so that our existing QEMU
tests can do that too, for some platforms at least.

Simon Glass July 13, 2024, 3:13 p.m. UTC | #8

Hi Tom,

On Wed, 3 Jul 2024 at 00:12, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > > >
> > > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > > >
> > > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > > >
> > > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > > and other situations. The board type and other things can be queried
> > > > > > > > as needed.
> > > > > > > >
> > > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > > information.
> > > > > > > >
> > > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > > 'ROLE' variable for each board.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > (no changes since v1)
> > > > > > > >
> > > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > > --- a/test/py/conftest.py
> > > > > > > > +++ b/test/py/conftest.py
> > > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > > >  import pytest
> > > > > > > >  import re
> > > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > > +import subprocess
> > > > > > > >  import sys
> > > > > > > >
> > > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > > >
> > > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > > >              str: Build directory
> > > > > > > >              str: Source directory
> > > > > > > >      """
> > > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > > +    role = config.getoption('role')
> > > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > > +    if role:
> > > > > > > > +        board_identity = role
> > > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > > +        env = os.environ.copy()
> > > > > > > > +        if build_dir:
> > > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > > +                              env=env)
> > > > > > > > +        if proc.returncode:
> > > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > > +        vals = {}
> > > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > > +            k = item.split(':')[-1]
> > > > > > > > +            vals[k] = value
> > > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > > +    else:
> > > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > > >
> > > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > > >      if not build_dir:
> > > > > > > >          build_dir = default_build_dir
> > > > > > >
> > > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > > board_type+board_identity and then we have the board
> > > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > > be "labgrid" ?
> > > > > >
> > > > > > The role is equivalent to the board identity, not the combination of
> > > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > > having to type long and complicated roles when connecting to boards.
> > > > > > It is similar to how things work today, except that the board type is
> > > > > > implied by the 'role'.
> > > > > >
> > > > > > For boards which have multiple identities (e.g. can support two
> > > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > > shares resources, to avoid any problems.
> > > > >
> > > > > I guess I have two sets of questions. First, if it's basically the
> > > > > board identity why can't we just use that as the role name, in your
> > > > > setup?
> > > >
> > > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> > >
> > > Then why do we need this?
> >
> > We need to pass a role to Labgrid, since it determines the board
> > identity to use. It also (indirectly) determines the U-Boot build to
> > use, since each board identity / role is a particular board with a
> > particular build.
>
> Oh, I get where you're coming from now at least. But this still sounds
> like a detail to put in to the conf.${board}_${board_type} file and not
> a thing to set here?

There are no such files with the Labgrid integration so far. They are
not needed.

>
> > For example:
> > role / identify = samus   - uses 'samus' board with build chromebook_samus
> > role / identify = samus_tpl   - uses 'samus' board with build
> > chromebook_samus_tpl
>
> Yes, or using one physical Pi 4 to test rpi_4_defconfig and
> rpi_arm64_defconfig (and if labgrid supported a way to remove files from
> the FAT partition, rpi_4_32b_defconfig).

Yes, that would be a nice addition.

>
> > Basically, as I understand it, the 'role' is the thing we want.
> >
> > Labgrid environment:
> >
> >   samus:
> >     resources:
> >       RemotePlace:
> >         name: samus
> > ...
> >       UBootProviderDriver:
> >         board: chromebook_samus
> >         binman_indir: /vid/software/devel/samus/bin
> >
> >   samus_tpl:
> >     resources:
> >       RemotePlace:
> >         name: samus
> >       UBootProviderDriver:
> >         board: chromebook_samus_tpl
> >         binman_indir: /vid/software/devel/samus/bin
>
> I guess the problem here is that from my point of view, this can live in
> the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
> going to worry about building U-Boot (even if blobs aren't a problem, we
> want to enable more features to test more things on HW) but from your
> point of view, buildman must provide test.py with the correct build so
> we need to know things prior.

Well, either you already have a build to test, iwc it is fine, or if
you don't you can pass --build to force a build, or rely on Labgrid to
initiate the build.

But in your case, the build must be done before running test.py since
it needs the .config file.

>
> > > > > But second, I'm not sure why we need this. The labgrid support we worked
> > > > > up here (but, sigh, it's not really clean enough to post) has:
> > > > > $ cat bin/lootbox/conf.rpi_4_na
> > > > > console_impl=labgrid
> > > > > reset_impl=labgrid
> > > > > flash_impl=labgrid.rpi_4
> > > > > flash_writer=labgrid.rpi_4
> > > > >
> > > > > For example and:
> > > > > $ cat bin/writer.labgrid.rpi_4
> > > > > set -e
> > > > >
> > > > > build=${U_BOOT_BUILD_DIR}
> > > > >
> > > > > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> > > > >
> > > > > So I don't know what the "role" part you have is for. And yes, there end
> > > > > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > > > > (such as beagleplay) we have:
> > > > > $ cat bin/writer.labgrid.ti-k3
> > > > > set -e
> > > > > build=${U_BOOT_BUILD_DIR}
> > > > >
> > > > > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> > > > >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> > > > >     echo "per the board documentation."
> > > > >     exit 1
> > > > > fi
> > > > > echo "Writing build at ${build}"
> > > > > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > > > > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > > > > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > > > > echo "Done writing build"
> > > > >
> > > > > In all cases we first build U-Boot and then pass the build directory to
> > > > > test.py, in something like:
> > > > > export LG_PLACE=rpi4
> > > > > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> > > > >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> > > > >   --exitfirst
> > > > >
> > > > > The only U-Boot side changes I needed to make were for counting the SPL
> > > > > banner instances, and that was regardless of framework (a particularly
> > > > > fun platform will show it 3 times).
> > > >
> > > > Yes it is possible to build U-Boot separately. Of course that involved
> > > > various blobs and so on, so every board is different. The approach I
> > > > have taken here is to have Labgrid call buildman to build what is
> > > > needed, with the blobs defined in the environment.
> > > >
> > > > You can use the -B flag to use a pre-built U-Boot, with the scripts
> > > > I've included.
> > >
> > > OK. I personally think pre-built (or outside of buildman built) U-Boot
> > > will be an important case, since everything needs more options enabled
> > > to test more features, but on real hardware. For example,
> > > CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
> > > issues are resolved. And I need to add CONFIG_FIT to some platforms so
> > > that I can then use the kernel test. And some platforms I end up
> > > enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
> > > CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).
> >
> > Yes that all sounds good. I have figured out how to add QEMU into this
> > Labgrid integration, but I cannot Debian to boot all the way to a
> > prompt with -nographic unless I pass something special on the Linux
> > commandline. So for now I parked that.
>
> Putting QEMU in to labgrid too could be interesting, yes. But I lost how
> it's related? To be clear, today we can test boot a Linux kernel on
> hardware. Somewhere on my TODO list is cycling over what kernel images
> to grab and shove in to the docker container so that our existing QEMU
> tests can do that too, for some platforms at least.

It's just a nice way of allowing 'ub-int qemu-x86' and getting to a
U-Boot prompt. Yes there are other ways to do it, and in fact it works
today if you set up your conf files for the machine you are on.

Regards,
Simon

Tom Rini July 13, 2024, 4:57 p.m. UTC | #9

On Sat, Jul 13, 2024 at 04:13:55PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 3 Jul 2024 at 00:12, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > > > >
> > > > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > > > >
> > > > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > > > and other situations. The board type and other things can be queried
> > > > > > > > > as needed.
> > > > > > > > >
> > > > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > > > information.
> > > > > > > > >
> > > > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > > > 'ROLE' variable for each board.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > (no changes since v1)
> > > > > > > > >
> > > > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > > > --- a/test/py/conftest.py
> > > > > > > > > +++ b/test/py/conftest.py
> > > > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > > > >  import pytest
> > > > > > > > >  import re
> > > > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > > > +import subprocess
> > > > > > > > >  import sys
> > > > > > > > >
> > > > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > > > >
> > > > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > > > >              str: Build directory
> > > > > > > > >              str: Source directory
> > > > > > > > >      """
> > > > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > > > +    role = config.getoption('role')
> > > > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > > > +    if role:
> > > > > > > > > +        board_identity = role
> > > > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > > > +        env = os.environ.copy()
> > > > > > > > > +        if build_dir:
> > > > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > > > +                              env=env)
> > > > > > > > > +        if proc.returncode:
> > > > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > > > +        vals = {}
> > > > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > > > +            k = item.split(':')[-1]
> > > > > > > > > +            vals[k] = value
> > > > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > > > +    else:
> > > > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > > > >
> > > > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > >      if not build_dir:
> > > > > > > > >          build_dir = default_build_dir
> > > > > > > >
> > > > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > > > board_type+board_identity and then we have the board
> > > > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > > > be "labgrid" ?
> > > > > > >
> > > > > > > The role is equivalent to the board identity, not the combination of
> > > > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > > > having to type long and complicated roles when connecting to boards.
> > > > > > > It is similar to how things work today, except that the board type is
> > > > > > > implied by the 'role'.
> > > > > > >
> > > > > > > For boards which have multiple identities (e.g. can support two
> > > > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > > > shares resources, to avoid any problems.
> > > > > >
> > > > > > I guess I have two sets of questions. First, if it's basically the
> > > > > > board identity why can't we just use that as the role name, in your
> > > > > > setup?
> > > > >
> > > > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> > > >
> > > > Then why do we need this?
> > >
> > > We need to pass a role to Labgrid, since it determines the board
> > > identity to use. It also (indirectly) determines the U-Boot build to
> > > use, since each board identity / role is a particular board with a
> > > particular build.
> >
> > Oh, I get where you're coming from now at least. But this still sounds
> > like a detail to put in to the conf.${board}_${board_type} file and not
> > a thing to set here?
> 
> There are no such files with the Labgrid integration so far. They are
> not needed.

They're needed in my case since I do not (cannot) use buildman to then
kick off the tests.

[snip]
> > > Basically, as I understand it, the 'role' is the thing we want.
> > >
> > > Labgrid environment:
> > >
> > >   samus:
> > >     resources:
> > >       RemotePlace:
> > >         name: samus
> > > ...
> > >       UBootProviderDriver:
> > >         board: chromebook_samus
> > >         binman_indir: /vid/software/devel/samus/bin
> > >
> > >   samus_tpl:
> > >     resources:
> > >       RemotePlace:
> > >         name: samus
> > >       UBootProviderDriver:
> > >         board: chromebook_samus_tpl
> > >         binman_indir: /vid/software/devel/samus/bin
> >
> > I guess the problem here is that from my point of view, this can live in
> > the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
> > going to worry about building U-Boot (even if blobs aren't a problem, we
> > want to enable more features to test more things on HW) but from your
> > point of view, buildman must provide test.py with the correct build so
> > we need to know things prior.
> 
> Well, either you already have a build to test, iwc it is fine, or if
> you don't you can pass --build to force a build, or rely on Labgrid to
> initiate the build.

No, neither buildman nor labgrid can initiate a functional build. Have
you integrated the beagleplay in to your lab? That I believe
demonstrates one of the problems (you need to build both
am62x_beagleplay_a53 and am62x_beagleplay_r5 and write files from both,
to test a given rev on the platform).

> But in your case, the build must be done before running test.py since
> it needs the .config file.

Yes, I build first and pass test.py the build directory.

> > > > > > But second, I'm not sure why we need this. The labgrid support we worked
> > > > > > up here (but, sigh, it's not really clean enough to post) has:
> > > > > > $ cat bin/lootbox/conf.rpi_4_na
> > > > > > console_impl=labgrid
> > > > > > reset_impl=labgrid
> > > > > > flash_impl=labgrid.rpi_4
> > > > > > flash_writer=labgrid.rpi_4
> > > > > >
> > > > > > For example and:
> > > > > > $ cat bin/writer.labgrid.rpi_4
> > > > > > set -e
> > > > > >
> > > > > > build=${U_BOOT_BUILD_DIR}
> > > > > >
> > > > > > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> > > > > >
> > > > > > So I don't know what the "role" part you have is for. And yes, there end
> > > > > > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > > > > > (such as beagleplay) we have:
> > > > > > $ cat bin/writer.labgrid.ti-k3
> > > > > > set -e
> > > > > > build=${U_BOOT_BUILD_DIR}
> > > > > >
> > > > > > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> > > > > >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> > > > > >     echo "per the board documentation."
> > > > > >     exit 1
> > > > > > fi
> > > > > > echo "Writing build at ${build}"
> > > > > > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > > > > > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > > > > > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > > > > > echo "Done writing build"
> > > > > >
> > > > > > In all cases we first build U-Boot and then pass the build directory to
> > > > > > test.py, in something like:
> > > > > > export LG_PLACE=rpi4
> > > > > > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> > > > > >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> > > > > >   --exitfirst
> > > > > >
> > > > > > The only U-Boot side changes I needed to make were for counting the SPL
> > > > > > banner instances, and that was regardless of framework (a particularly
> > > > > > fun platform will show it 3 times).
> > > > >
> > > > > Yes it is possible to build U-Boot separately. Of course that involved
> > > > > various blobs and so on, so every board is different. The approach I
> > > > > have taken here is to have Labgrid call buildman to build what is
> > > > > needed, with the blobs defined in the environment.
> > > > >
> > > > > You can use the -B flag to use a pre-built U-Boot, with the scripts
> > > > > I've included.
> > > >
> > > > OK. I personally think pre-built (or outside of buildman built) U-Boot
> > > > will be an important case, since everything needs more options enabled
> > > > to test more features, but on real hardware. For example,
> > > > CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
> > > > issues are resolved. And I need to add CONFIG_FIT to some platforms so
> > > > that I can then use the kernel test. And some platforms I end up
> > > > enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
> > > > CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).
> > >
> > > Yes that all sounds good. I have figured out how to add QEMU into this
> > > Labgrid integration, but I cannot Debian to boot all the way to a
> > > prompt with -nographic unless I pass something special on the Linux
> > > commandline. So for now I parked that.
> >
> > Putting QEMU in to labgrid too could be interesting, yes. But I lost how
> > it's related? To be clear, today we can test boot a Linux kernel on
> > hardware. Somewhere on my TODO list is cycling over what kernel images
> > to grab and shove in to the docker container so that our existing QEMU
> > tests can do that too, for some platforms at least.
> 
> It's just a nice way of allowing 'ub-int qemu-x86' and getting to a
> U-Boot prompt. Yes there are other ways to do it, and in fact it works
> today if you set up your conf files for the machine you are on.

Yes, I've locally included qemu hosts as needed. I guess this was just
as an aside? Because yes, it would be good to run the net_boot tests on
more platforms, automatically, including/especially QEMU.

Simon Glass July 15, 2024, 7:11 a.m. UTC | #10

Hi Tom,

On Sat, 13 Jul 2024 at 17:57, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Jul 13, 2024 at 04:13:55PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 3 Jul 2024 at 00:12, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > > > > >
> > > > > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > > > > >
> > > > > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > > > > and other situations. The board type and other things can be queried
> > > > > > > > > > as needed.
> > > > > > > > > >
> > > > > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > > > > information.
> > > > > > > > > >
> > > > > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > > > > 'ROLE' variable for each board.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > (no changes since v1)
> > > > > > > > > >
> > > > > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > > > > --- a/test/py/conftest.py
> > > > > > > > > > +++ b/test/py/conftest.py
> > > > > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > > > > >  import pytest
> > > > > > > > > >  import re
> > > > > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > > > > +import subprocess
> > > > > > > > > >  import sys
> > > > > > > > > >
> > > > > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > > > > >
> > > > > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > > > > >              str: Build directory
> > > > > > > > > >              str: Source directory
> > > > > > > > > >      """
> > > > > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > > > > +    role = config.getoption('role')
> > > > > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > > > > +    if role:
> > > > > > > > > > +        board_identity = role
> > > > > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > > > > +        env = os.environ.copy()
> > > > > > > > > > +        if build_dir:
> > > > > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > > > > +                              env=env)
> > > > > > > > > > +        if proc.returncode:
> > > > > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > > > > +        vals = {}
> > > > > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > > > > +            k = item.split(':')[-1]
> > > > > > > > > > +            vals[k] = value
> > > > > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > > > > +    else:
> > > > > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > > > > >
> > > > > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > >      if not build_dir:
> > > > > > > > > >          build_dir = default_build_dir
> > > > > > > > >
> > > > > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > > > > board_type+board_identity and then we have the board
> > > > > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > > > > be "labgrid" ?
> > > > > > > >
> > > > > > > > The role is equivalent to the board identity, not the combination of
> > > > > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > > > > having to type long and complicated roles when connecting to boards.
> > > > > > > > It is similar to how things work today, except that the board type is
> > > > > > > > implied by the 'role'.
> > > > > > > >
> > > > > > > > For boards which have multiple identities (e.g. can support two
> > > > > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > > > > shares resources, to avoid any problems.
> > > > > > >
> > > > > > > I guess I have two sets of questions. First, if it's basically the
> > > > > > > board identity why can't we just use that as the role name, in your
> > > > > > > setup?
> > > > > >
> > > > > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > > > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> > > > >
> > > > > Then why do we need this?
> > > >
> > > > We need to pass a role to Labgrid, since it determines the board
> > > > identity to use. It also (indirectly) determines the U-Boot build to
> > > > use, since each board identity / role is a particular board with a
> > > > particular build.
> > >
> > > Oh, I get where you're coming from now at least. But this still sounds
> > > like a detail to put in to the conf.${board}_${board_type} file and not
> > > a thing to set here?
> >
> > There are no such files with the Labgrid integration so far. They are
> > not needed.
>
> They're needed in my case since I do not (cannot) use buildman to then
> kick off the tests.

OK...is your environment upstream so I can compare with mine?

>
> [snip]
> > > > Basically, as I understand it, the 'role' is the thing we want.
> > > >
> > > > Labgrid environment:
> > > >
> > > >   samus:
> > > >     resources:
> > > >       RemotePlace:
> > > >         name: samus
> > > > ...
> > > >       UBootProviderDriver:
> > > >         board: chromebook_samus
> > > >         binman_indir: /vid/software/devel/samus/bin
> > > >
> > > >   samus_tpl:
> > > >     resources:
> > > >       RemotePlace:
> > > >         name: samus
> > > >       UBootProviderDriver:
> > > >         board: chromebook_samus_tpl
> > > >         binman_indir: /vid/software/devel/samus/bin
> > >
> > > I guess the problem here is that from my point of view, this can live in
> > > the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
> > > going to worry about building U-Boot (even if blobs aren't a problem, we
> > > want to enable more features to test more things on HW) but from your
> > > point of view, buildman must provide test.py with the correct build so
> > > we need to know things prior.
> >
> > Well, either you already have a build to test, iwc it is fine, or if
> > you don't you can pass --build to force a build, or rely on Labgrid to
> > initiate the build.
>
> No, neither buildman nor labgrid can initiate a functional build. Have
> you integrated the beagleplay in to your lab? That I believe
> demonstrates one of the problems (you need to build both
> am62x_beagleplay_a53 and am62x_beagleplay_r5 and write files from both,
> to test a given rev on the platform).

Actually I was about to do that. Will get back to it in a few weeks.
Labgrid can initiate two builds and copy files from both.

>
> > But in your case, the build must be done before running test.py since
> > it needs the .config file.
>
> Yes, I build first and pass test.py the build directory.
>
> > > > > > > But second, I'm not sure why we need this. The labgrid support we worked
> > > > > > > up here (but, sigh, it's not really clean enough to post) has:
> > > > > > > $ cat bin/lootbox/conf.rpi_4_na
> > > > > > > console_impl=labgrid
> > > > > > > reset_impl=labgrid
> > > > > > > flash_impl=labgrid.rpi_4
> > > > > > > flash_writer=labgrid.rpi_4
> > > > > > >
> > > > > > > For example and:
> > > > > > > $ cat bin/writer.labgrid.rpi_4
> > > > > > > set -e
> > > > > > >
> > > > > > > build=${U_BOOT_BUILD_DIR}
> > > > > > >
> > > > > > > $LG_CLIENT write-files -T ${build}/u-boot.bin kernel8.img
> > > > > > >
> > > > > > > So I don't know what the "role" part you have is for. And yes, there end
> > > > > > > up being multiple writer.labgrid.FOO scripts because for TI K3 platforms
> > > > > > > (such as beagleplay) we have:
> > > > > > > $ cat bin/writer.labgrid.ti-k3
> > > > > > > set -e
> > > > > > > build=${U_BOOT_BUILD_DIR}
> > > > > > >
> > > > > > > if [ -z "${tispl}" -o -z "${uboot}" -o -z "${tiboot3}" ]; then
> > > > > > >     echo "Must configure tispl, uboot, tiboot3 and optionally sysfw"
> > > > > > >     echo "per the board documentation."
> > > > > > >     exit 1
> > > > > > > fi
> > > > > > > echo "Writing build at ${build}"
> > > > > > > $LG_CLIENT write-files -T ${build}/${tispl} tispl.bin
> > > > > > > $LG_CLIENT write-files -T ${build}/${uboot} u-boot.img
> > > > > > > $LG_CLIENT write-files -T ${build/_a??/_r5}/${tiboot3} tiboot3.bin
> > > > > > > echo "Done writing build"
> > > > > > >
> > > > > > > In all cases we first build U-Boot and then pass the build directory to
> > > > > > > test.py, in something like:
> > > > > > > export LG_PLACE=rpi4
> > > > > > > ./test/py/test.py -ra --bd rpi_4 --build-dir .../build-rpi4 \
> > > > > > >   --results-dir .../results-rpi4 --persistent-data-dir .../pd-rpi4 \
> > > > > > >   --exitfirst
> > > > > > >
> > > > > > > The only U-Boot side changes I needed to make were for counting the SPL
> > > > > > > banner instances, and that was regardless of framework (a particularly
> > > > > > > fun platform will show it 3 times).
> > > > > >
> > > > > > Yes it is possible to build U-Boot separately. Of course that involved
> > > > > > various blobs and so on, so every board is different. The approach I
> > > > > > have taken here is to have Labgrid call buildman to build what is
> > > > > > needed, with the blobs defined in the environment.
> > > > > >
> > > > > > You can use the -B flag to use a pre-built U-Boot, with the scripts
> > > > > > I've included.
> > > > >
> > > > > OK. I personally think pre-built (or outside of buildman built) U-Boot
> > > > > will be an important case, since everything needs more options enabled
> > > > > to test more features, but on real hardware. For example,
> > > > > CONFIG_UNIT_TEST should be on for everyone, in testing, once the build
> > > > > issues are resolved. And I need to add CONFIG_FIT to some platforms so
> > > > > that I can then use the kernel test. And some platforms I end up
> > > > > enabling CONFIG_CMD_BOOTEFI_HELLO on (but others disabling
> > > > > CONFIG_CMD_BOOTEFI_SELFTEST as that fails and that's just A Thing).
> > > >
> > > > Yes that all sounds good. I have figured out how to add QEMU into this
> > > > Labgrid integration, but I cannot Debian to boot all the way to a
> > > > prompt with -nographic unless I pass something special on the Linux
> > > > commandline. So for now I parked that.
> > >
> > > Putting QEMU in to labgrid too could be interesting, yes. But I lost how
> > > it's related? To be clear, today we can test boot a Linux kernel on
> > > hardware. Somewhere on my TODO list is cycling over what kernel images
> > > to grab and shove in to the docker container so that our existing QEMU
> > > tests can do that too, for some platforms at least.
> >
> > It's just a nice way of allowing 'ub-int qemu-x86' and getting to a
> > U-Boot prompt. Yes there are other ways to do it, and in fact it works
> > today if you set up your conf files for the machine you are on.
>
> Yes, I've locally included qemu hosts as needed. I guess this was just
> as an aside? Because yes, it would be good to run the net_boot tests on
> more platforms, automatically, including/especially QEMU.

Indeed.

Regards,
SImon

Tom Rini July 15, 2024, 9:03 p.m. UTC | #11

On Mon, Jul 15, 2024 at 08:11:28AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 13 Jul 2024 at 17:57, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Jul 13, 2024 at 04:13:55PM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 3 Jul 2024 at 00:12, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > > > > > >
> > > > > > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > > > > > >
> > > > > > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > > > > > >
> > > > > > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > > > > > and other situations. The board type and other things can be queried
> > > > > > > > > > > as needed.
> > > > > > > > > > >
> > > > > > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > > > > > information.
> > > > > > > > > > >
> > > > > > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > > > > > 'ROLE' variable for each board.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > (no changes since v1)
> > > > > > > > > > >
> > > > > > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > > > > > --- a/test/py/conftest.py
> > > > > > > > > > > +++ b/test/py/conftest.py
> > > > > > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > > > > > >  import pytest
> > > > > > > > > > >  import re
> > > > > > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > > > > > +import subprocess
> > > > > > > > > > >  import sys
> > > > > > > > > > >
> > > > > > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > > > > > >
> > > > > > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > > > > > >              str: Build directory
> > > > > > > > > > >              str: Source directory
> > > > > > > > > > >      """
> > > > > > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > > > > > +    role = config.getoption('role')
> > > > > > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > > > > > +    if role:
> > > > > > > > > > > +        board_identity = role
> > > > > > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > > > > > +        env = os.environ.copy()
> > > > > > > > > > > +        if build_dir:
> > > > > > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > > > > > +                              env=env)
> > > > > > > > > > > +        if proc.returncode:
> > > > > > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > > > > > +        vals = {}
> > > > > > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > > > > > +            k = item.split(':')[-1]
> > > > > > > > > > > +            vals[k] = value
> > > > > > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > > > > > +    else:
> > > > > > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > > > > > >
> > > > > > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > > >      if not build_dir:
> > > > > > > > > > >          build_dir = default_build_dir
> > > > > > > > > >
> > > > > > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > > > > > board_type+board_identity and then we have the board
> > > > > > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > > > > > be "labgrid" ?
> > > > > > > > >
> > > > > > > > > The role is equivalent to the board identity, not the combination of
> > > > > > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > > > > > having to type long and complicated roles when connecting to boards.
> > > > > > > > > It is similar to how things work today, except that the board type is
> > > > > > > > > implied by the 'role'.
> > > > > > > > >
> > > > > > > > > For boards which have multiple identities (e.g. can support two
> > > > > > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > > > > > shares resources, to avoid any problems.
> > > > > > > >
> > > > > > > > I guess I have two sets of questions. First, if it's basically the
> > > > > > > > board identity why can't we just use that as the role name, in your
> > > > > > > > setup?
> > > > > > >
> > > > > > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > > > > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> > > > > >
> > > > > > Then why do we need this?
> > > > >
> > > > > We need to pass a role to Labgrid, since it determines the board
> > > > > identity to use. It also (indirectly) determines the U-Boot build to
> > > > > use, since each board identity / role is a particular board with a
> > > > > particular build.
> > > >
> > > > Oh, I get where you're coming from now at least. But this still sounds
> > > > like a detail to put in to the conf.${board}_${board_type} file and not
> > > > a thing to set here?
> > >
> > > There are no such files with the Labgrid integration so far. They are
> > > not needed.
> >
> > They're needed in my case since I do not (cannot) use buildman to then
> > kick off the tests.
> 
> OK...is your environment upstream so I can compare with mine?

The engineer here that was working on it is unfortunately leaving
shortly and I forget if he got everything labgrid related posted. The
other half of the environment is that none of my tests treat the hooks
repository any different than before. And note that when I say cannot
above I mean that because:
1) All of the TI platforms that require an Cortex-R and Cortex-A builds.
You can (nominally) stick with only upgrading one part at a time, and so
just test an even smaller subset on the R core, and once that passes,
test the subset of tests that run on HW on the A core.
2) Enable further options. I enable CMD_BOOTMENU, CMD_LOG globally,
CMD_TFTPPUT and FIT+FIT_SIGNATURE globally and BOOTSTAGE (+ stash) on
Pi, and I'm going to cover TI K3 platforms next (since they too can
easily share a stash addr).

The latter could be solved if buildman had some native config-fragment
support and we didn't have the #include games we have today. No, I don't
have a good idea on solving that either, only noting that today I use
scripts/kconfig/merge_config.sh to combine defconfig + the above.


> > [snip]
> > > > > Basically, as I understand it, the 'role' is the thing we want.
> > > > >
> > > > > Labgrid environment:
> > > > >
> > > > >   samus:
> > > > >     resources:
> > > > >       RemotePlace:
> > > > >         name: samus
> > > > > ...
> > > > >       UBootProviderDriver:
> > > > >         board: chromebook_samus
> > > > >         binman_indir: /vid/software/devel/samus/bin
> > > > >
> > > > >   samus_tpl:
> > > > >     resources:
> > > > >       RemotePlace:
> > > > >         name: samus
> > > > >       UBootProviderDriver:
> > > > >         board: chromebook_samus_tpl
> > > > >         binman_indir: /vid/software/devel/samus/bin
> > > >
> > > > I guess the problem here is that from my point of view, this can live in
> > > > the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
> > > > going to worry about building U-Boot (even if blobs aren't a problem, we
> > > > want to enable more features to test more things on HW) but from your
> > > > point of view, buildman must provide test.py with the correct build so
> > > > we need to know things prior.
> > >
> > > Well, either you already have a build to test, iwc it is fine, or if
> > > you don't you can pass --build to force a build, or rely on Labgrid to
> > > initiate the build.
> >
> > No, neither buildman nor labgrid can initiate a functional build. Have
> > you integrated the beagleplay in to your lab? That I believe
> > demonstrates one of the problems (you need to build both
> > am62x_beagleplay_a53 and am62x_beagleplay_r5 and write files from both,
> > to test a given rev on the platform).
> 
> Actually I was about to do that. Will get back to it in a few weeks.
> Labgrid can initiate two builds and copy files from both.

I'm very interested in what this all looks like once that works too.

Simon Glass July 31, 2024, 2:39 p.m. UTC | #12

Hi Tom,

On Mon, 15 Jul 2024 at 15:03, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jul 15, 2024 at 08:11:28AM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sat, 13 Jul 2024 at 17:57, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Jul 13, 2024 at 04:13:55PM +0100, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 3 Jul 2024 at 00:12, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jun 27, 2024 at 09:37:18AM +0100, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Wed, 26 Jun 2024 at 15:29, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jun 26, 2024 at 09:00:33AM +0100, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Tue, 25 Jun 2024 at 15:27, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jun 25, 2024 at 01:38:08PM +0100, Simon Glass wrote:
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > On Mon, 24 Jun 2024 at 19:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Sun, Jun 23, 2024 at 02:32:02PM -0600, Simon Glass wrote:
> > > > > > > > > > >
> > > > > > > > > > > > In Labgrid there is the concept of a 'role', which is similar to the
> > > > > > > > > > > > U-Boot board ID in U-Boot's pytest subsystem.
> > > > > > > > > > > >
> > > > > > > > > > > > The role indicates both the target and information about the U-Boot
> > > > > > > > > > > > build to use. It can also provide any amount of other configuration.
> > > > > > > > > > > > The information is obtained using the 'labgrid-client query' operation.
> > > > > > > > > > > >
> > > > > > > > > > > > Make use of this in tests, so that only the role is required in gitlab
> > > > > > > > > > > > and other situations. The board type and other things can be queried
> > > > > > > > > > > > as needed.
> > > > > > > > > > > >
> > > > > > > > > > > > Use a new 'u-boot-test-getrole' script to obtain the requested
> > > > > > > > > > > > information.
> > > > > > > > > > > >
> > > > > > > > > > > > With this it is possible to run lab tests in gitlab with just a single
> > > > > > > > > > > > 'ROLE' variable for each board.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > (no changes since v1)
> > > > > > > > > > > >
> > > > > > > > > > > >  test/py/conftest.py | 31 +++++++++++++++++++++++++++----
> > > > > > > > > > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/test/py/conftest.py b/test/py/conftest.py
> > > > > > > > > > > > index 6547c6922c6..5de8d7b0e23 100644
> > > > > > > > > > > > --- a/test/py/conftest.py
> > > > > > > > > > > > +++ b/test/py/conftest.py
> > > > > > > > > > > > @@ -23,6 +23,7 @@ from pathlib import Path
> > > > > > > > > > > >  import pytest
> > > > > > > > > > > >  import re
> > > > > > > > > > > >  from _pytest.runner import runtestprotocol
> > > > > > > > > > > > +import subprocess
> > > > > > > > > > > >  import sys
> > > > > > > > > > > >
> > > > > > > > > > > >  # Globals: The HTML log file, and the connection to the U-Boot console.
> > > > > > > > > > > > @@ -79,6 +80,7 @@ def pytest_addoption(parser):
> > > > > > > > > > > >      parser.addoption('--gdbserver', default=None,
> > > > > > > > > > > >          help='Run sandbox under gdbserver. The argument is the channel '+
> > > > > > > > > > > >          'over which gdbserver should communicate, e.g. localhost:1234')
> > > > > > > > > > > > +    parser.addoption('--role', help='U-Boot board role (for Labgrid)')
> > > > > > > > > > > >      parser.addoption('--no-prompt-wait', default=False, action='store_true',
> > > > > > > > > > > >          help="Assume that U-Boot is ready and don't wait for a prompt")
> > > > > > > > > > > >
> > > > > > > > > > > > @@ -130,12 +132,33 @@ def get_details(config):
> > > > > > > > > > > >              str: Build directory
> > > > > > > > > > > >              str: Source directory
> > > > > > > > > > > >      """
> > > > > > > > > > > > -    board_type = config.getoption('board_type')
> > > > > > > > > > > > -    board_identity = config.getoption('board_identity')
> > > > > > > > > > > > +    role = config.getoption('role')
> > > > > > > > > > > >      build_dir = config.getoption('build_dir')
> > > > > > > > > > > > +    if role:
> > > > > > > > > > > > +        board_identity = role
> > > > > > > > > > > > +        cmd = ['u-boot-test-getrole', role, '--configure']
> > > > > > > > > > > > +        env = os.environ.copy()
> > > > > > > > > > > > +        if build_dir:
> > > > > > > > > > > > +            env['U_BOOT_BUILD_DIR'] = build_dir
> > > > > > > > > > > > +        proc = subprocess.run(cmd, capture_output=True, encoding='utf-8',
> > > > > > > > > > > > +                              env=env)
> > > > > > > > > > > > +        if proc.returncode:
> > > > > > > > > > > > +            raise ValueError(proc.stderr)
> > > > > > > > > > > > +        print('conftest: lab:', proc.stdout)
> > > > > > > > > > > > +        vals = {}
> > > > > > > > > > > > +        for line in proc.stdout.splitlines():
> > > > > > > > > > > > +            item, value = line.split(' ', maxsplit=1)
> > > > > > > > > > > > +            k = item.split(':')[-1]
> > > > > > > > > > > > +            vals[k] = value
> > > > > > > > > > > > +        print('conftest: lab info:', vals)
> > > > > > > > > > > > +        board_type, default_build_dir, source_dir = (vals['board'],
> > > > > > > > > > > > +            vals['build_dir'], vals['source_dir'])
> > > > > > > > > > > > +    else:
> > > > > > > > > > > > +        board_type = config.getoption('board_type')
> > > > > > > > > > > > +        board_identity = config.getoption('board_identity')
> > > > > > > > > > > >
> > > > > > > > > > > > -    source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > > > -    default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > > > > +        source_dir = os.path.dirname(os.path.dirname(TEST_PY_DIR))
> > > > > > > > > > > > +        default_build_dir = source_dir + '/build-' + board_type
> > > > > > > > > > > >      if not build_dir:
> > > > > > > > > > > >          build_dir = default_build_dir
> > > > > > > > > > >
> > > > > > > > > > > I'm a little confused here. Why can't we construct "role" from
> > > > > > > > > > > board_type+board_identity and then we have the board
> > > > > > > > > > > conf.${board_type}_${board_identity} file set whatever needs setting to
> > > > > > > > > > > be "labgrid" ?
> > > > > > > > > >
> > > > > > > > > > The role is equivalent to the board identity, not the combination of
> > > > > > > > > > the U-Boot board type and the board identity. I went this way to avoid
> > > > > > > > > > having to type long and complicated roles when connecting to boards.
> > > > > > > > > > It is similar to how things work today, except that the board type is
> > > > > > > > > > implied by the 'role'.
> > > > > > > > > >
> > > > > > > > > > For boards which have multiple identities (e.g. can support two
> > > > > > > > > > different board types), Labgrid handles acquiring and releasing the
> > > > > > > > > > shares resources, to avoid any problems.
> > > > > > > > >
> > > > > > > > > I guess I have two sets of questions. First, if it's basically the
> > > > > > > > > board identity why can't we just use that as the role name, in your
> > > > > > > > > setup?
> > > > > > > >
> > > > > > > > Yes, that's what I am doing. If you look in console.labgrid you can
> > > > > > > > see that it is passing U_BOOT_BOARD_IDENTITY as the -r argument.
> > > > > > >
> > > > > > > Then why do we need this?
> > > > > >
> > > > > > We need to pass a role to Labgrid, since it determines the board
> > > > > > identity to use. It also (indirectly) determines the U-Boot build to
> > > > > > use, since each board identity / role is a particular board with a
> > > > > > particular build.
> > > > >
> > > > > Oh, I get where you're coming from now at least. But this still sounds
> > > > > like a detail to put in to the conf.${board}_${board_type} file and not
> > > > > a thing to set here?
> > > >
> > > > There are no such files with the Labgrid integration so far. They are
> > > > not needed.
> > >
> > > They're needed in my case since I do not (cannot) use buildman to then
> > > kick off the tests.
> >
> > OK...is your environment upstream so I can compare with mine?
>
> The engineer here that was working on it is unfortunately leaving
> shortly and I forget if he got everything labgrid related posted. The
> other half of the environment is that none of my tests treat the hooks
> repository any different than before. And note that when I say cannot
> above I mean that because:
> 1) All of the TI platforms that require an Cortex-R and Cortex-A builds.
> You can (nominally) stick with only upgrading one part at a time, and so
> just test an even smaller subset on the R core, and once that passes,
> test the subset of tests that run on HW on the A core.
> 2) Enable further options. I enable CMD_BOOTMENU, CMD_LOG globally,
> CMD_TFTPPUT and FIT+FIT_SIGNATURE globally and BOOTSTAGE (+ stash) on
> Pi, and I'm going to cover TI K3 platforms next (since they too can
> easily share a stash addr).

OK I see. To support that with my integration I would need to provide
a way to enable config options in the Labgrid environment. I suppose
that is pretty easy.

>
> The latter could be solved if buildman had some native config-fragment
> support and we didn't have the #include games we have today. No, I don't
> have a good idea on solving that either, only noting that today I use
> scripts/kconfig/merge_config.sh to combine defconfig + the above.

We have an issue for config fragments [1] but I haven't looked at it,
other than proposing a possible option.

>
>
> > > [snip]
> > > > > > Basically, as I understand it, the 'role' is the thing we want.
> > > > > >
> > > > > > Labgrid environment:
> > > > > >
> > > > > >   samus:
> > > > > >     resources:
> > > > > >       RemotePlace:
> > > > > >         name: samus
> > > > > > ...
> > > > > >       UBootProviderDriver:
> > > > > >         board: chromebook_samus
> > > > > >         binman_indir: /vid/software/devel/samus/bin
> > > > > >
> > > > > >   samus_tpl:
> > > > > >     resources:
> > > > > >       RemotePlace:
> > > > > >         name: samus
> > > > > >       UBootProviderDriver:
> > > > > >         board: chromebook_samus_tpl
> > > > > >         binman_indir: /vid/software/devel/samus/bin
> > > > >
> > > > > I guess the problem here is that from my point of view, this can live in
> > > > > the u-boot-test-hooks/bin/<host>/conf.<machine> file since we're never
> > > > > going to worry about building U-Boot (even if blobs aren't a problem, we
> > > > > want to enable more features to test more things on HW) but from your
> > > > > point of view, buildman must provide test.py with the correct build so
> > > > > we need to know things prior.
> > > >
> > > > Well, either you already have a build to test, iwc it is fine, or if
> > > > you don't you can pass --build to force a build, or rely on Labgrid to
> > > > initiate the build.
> > >
> > > No, neither buildman nor labgrid can initiate a functional build. Have
> > > you integrated the beagleplay in to your lab? That I believe
> > > demonstrates one of the problems (you need to build both
> > > am62x_beagleplay_a53 and am62x_beagleplay_r5 and write files from both,
> > > to test a given rev on the platform).
> >
> > Actually I was about to do that. Will get back to it in a few weeks.
> > Labgrid can initiate two builds and copy files from both.
>
> I'm very interested in what this all looks like once that works too.

OK, I pushed it to [2]. There are no changes to the U-Boot side though.

>
> --
> Tom

Regards,
Simon

[1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/9
[2] https://github.com/labgrid-project/labgrid/pull/1411

[v3,08/19] test: Introduce the concept of a role

Commit Message

Comments

Patch