Re: [RFC PATCH v2 1/3] runfvp: make fvp runner to hold the config

[Jon Mason <jdmason@kudzu.us>]

On Tue, May 23, 2023 at 05:06:41PM +0100, Peter Hoyes wrote:
> Hi Clement,
> 
> On 23/05/2023 12:40, Cl�ment P�ron wrote:
> > Hi Peter,
> > 
> > On Wed, 17 May 2023 at 12:09, Cl�ment P�ron <peron.clem@gmail.com> wrote:
> > > At the moment the config is load and pass to FVPRunner.
> > > 
> > > Change the ownership to FVPRunner.
> > What do you think about this patch?
> > 
> > I have moved the ownership of the config to the FVPRunner. Is it what
> > you had in mind?
> > 
> > Thanks for your feedback
> 
> First of all, this skipped my attention last week so apologies for the
> follow-up delay.
> 
> All three patches look great to me, but I'm having trouble applying them
> cleanly to master and I'm seeing CRLF line endings. It might be something at
> my end, but� is it possible you need to rebase and/or use git send-email ?
> 
> @Jon Mason: LGTM if it pases CI!

I applied them to met-arm master-next and it passes CI (when stacked
on top the other two pathes).  See
https://gitlab.com/jonmason00/meta-arm/-/pipelines/874749366

If there aren't any objections, I can apply all five.

Thanks,
Jon

> 
> Peter
> 
> > 
> > Regards,
> > Clement
> > 
> > 
> > > Signed-off-by: Cl�ment P�ron <peron.clem@gmail.com>
> > > ---
> > >   meta-arm/lib/fvp/runner.py                 | 15 +++++--
> > >   meta-arm/lib/oeqa/controllers/fvp.py       | 13 +++---
> > >   meta-arm/lib/oeqa/selftest/cases/runfvp.py | 49 +++++++++++++---------
> > >   scripts/runfvp                             | 11 +++--
> > >   4 files changed, 52 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/meta-arm/lib/fvp/runner.py b/meta-arm/lib/fvp/runner.py
> > > index d957e780..4f5f88ca 100644
> > > --- a/meta-arm/lib/fvp/runner.py
> > > +++ b/meta-arm/lib/fvp/runner.py
> > > @@ -6,7 +6,7 @@ import shutil
> > >   import sys
> > > 
> > >   from .terminal import terminals
> > > -
> > > +from .conffile import load
> > > 
> > >   def cli_from_config(config, terminal_choice):
> > >       cli = []
> > > @@ -83,14 +83,18 @@ class FVPRunner:
> > >           self._fvp_process = None
> > >           self._telnets = []
> > >           self._pexpects = []
> > > +        self._config = None
> > > +
> > > +    def start(self, fvpconf, extra_args=[], terminal_choice="none", stdout=subprocess.PIPE):
> > > +        self._logger.debug(f"Loading {fvpconf}")
> > > +        self._config = load(fvpconf)
> > > 
> > > -    def start(self, config, extra_args=[], terminal_choice="none", stdout=subprocess.PIPE):
> > > -        cli = cli_from_config(config, terminal_choice)
> > > +        cli = cli_from_config(self._config, terminal_choice)
> > >           cli += extra_args
> > > 
> > >           # Pass through environment variables needed for GUI applications, such
> > >           # as xterm, to work.
> > > -        env = config['env']
> > > +        env = self._config['env']
> > >           for name in ('DISPLAY', 'PATH', 'WAYLAND_DISPLAY', 'XAUTHORITY'):
> > >               if name in os.environ:
> > >                   env[name] = os.environ[name]
> > > @@ -140,6 +144,9 @@ class FVPRunner:
> > >       def wait(self, timeout):
> > >           self._fvp_process.wait(timeout)
> > > 
> > > +    def getConfig(self):
> > > +        return self._config
> > > +
> > >       @property
> > >       def stdout(self):
> > >           return self._fvp_process.stdout
> > > diff --git a/meta-arm/lib/oeqa/controllers/fvp.py b/meta-arm/lib/oeqa/controllers/fvp.py
> > > index e8a094f1..38484072 100644
> > > --- a/meta-arm/lib/oeqa/controllers/fvp.py
> > > +++ b/meta-arm/lib/oeqa/controllers/fvp.py
> > > @@ -3,7 +3,7 @@ import pexpect
> > >   import os
> > > 
> > >   from oeqa.core.target.ssh import OESSHTarget
> > > -from fvp import conffile, runner
> > > +from fvp import runner
> > > 
> > > 
> > >   class OEFVPSSHTarget(OESSHTarget):
> > > @@ -19,7 +19,6 @@ class OEFVPSSHTarget(OESSHTarget):
> > >           basename = pathlib.Path(rootfs)
> > >           basename = basename.name.replace("".join(basename.suffixes), "")
> > >           self.fvpconf = image_dir / (basename + ".fvpconf")
> > > -        self.config = conffile.load(self.fvpconf)
> > >           self.bootlog = bootlog
> > > 
> > >           if not self.fvpconf.exists():
> > > @@ -31,7 +30,7 @@ class OEFVPSSHTarget(OESSHTarget):
> > >       def start(self, **kwargs):
> > >           self.fvp_log = self._create_logfile("fvp")
> > >           self.fvp = runner.FVPRunner(self.logger)
> > > -        self.fvp.start(self.config, stdout=self.fvp_log)
> > > +        self.fvp.start(self.fvpconf, stdout=self.fvp_log)
> > >           self.logger.debug(f"Started FVP PID {self.fvp.pid()}")
> > >           self._after_start()
> > > 
> > > @@ -72,8 +71,9 @@ class OEFVPTarget(OEFVPSSHTarget):
> > >       def _after_start(self):
> > >           with open(self.fvp_log.name, 'rb') as logfile:
> > >               parser = runner.ConsolePortParser(logfile)
> > > -            self.logger.debug(f"Awaiting console on terminal {self.config['consoles']['default']}")
> > > -            port = parser.parse_port(self.config['consoles']['default'])
> > > +            config = self.fvp.getConfig()
> > > +            self.logger.debug(f"Awaiting console on terminal {config['consoles']['default']}")
> > > +            port = parser.parse_port(config['consoles']['default'])
> > >               console = self.fvp.create_pexpect(port)
> > >               try:
> > >                   console.expect("login\\:", timeout=self.boot_timeout)
> > > @@ -105,7 +105,8 @@ class OEFVPSerialTarget(OEFVPSSHTarget):
> > >       def _after_start(self):
> > >           with open(self.fvp_log.name, 'rb') as logfile:
> > >               parser = runner.ConsolePortParser(logfile)
> > > -            for name, console in self.config["consoles"].items():
> > > +            config = self.fvp.getConfig()
> > > +            for name, console in config["consoles"].items():
> > >                   logfile = self._create_logfile(name)
> > >                   self.logger.info(f'Creating terminal {name} on {console}')
> > >                   port = parser.parse_port(console)
> > > diff --git a/meta-arm/lib/oeqa/selftest/cases/runfvp.py b/meta-arm/lib/oeqa/selftest/cases/runfvp.py
> > > index 46941ca9..2d2cdc80 100644
> > > --- a/meta-arm/lib/oeqa/selftest/cases/runfvp.py
> > > +++ b/meta-arm/lib/oeqa/selftest/cases/runfvp.py
> > > @@ -1,5 +1,6 @@
> > >   import asyncio
> > >   import os
> > > +import json
> > >   import pathlib
> > >   import subprocess
> > >   import tempfile
> > > @@ -88,16 +89,20 @@ class RunnerTests(OESelftestTestCase):
> > >           from fvp import runner
> > >           with self.create_mock() as m:
> > >               fvp = runner.FVPRunner(self.logger)
> > > -            fvp.start({
> > > -                "fvp-bindir": "/usr/bin",
> > > -                "exe": "FVP_Binary",
> > > -                "parameters": {'foo': 'bar'},
> > > -                "data": ['data1'],
> > > -                "applications": {'a1': 'file'},
> > > -                "terminals": {},
> > > -                "args": ['--extra-arg'],
> > > -                "env": {"FOO": "BAR"}
> > > -            })
> > > +            config = {"fvp-bindir": "/usr/bin",
> > > +                    "exe": "FVP_Binary",
> > > +                    "parameters": {'foo': 'bar'},
> > > +                    "data": ['data1'],
> > > +                    "applications": {'a1': 'file'},
> > > +                    "terminals": {},
> > > +                    "args": ['--extra-arg'],
> > > +                    "env": {"FOO": "BAR"}
> > > +                     }
> > > +
> > > +            with tempfile.NamedTemporaryFile('w') as fvpconf:
> > > +                json.dump(config, fvpconf)
> > > +                fvpconf.flush()
> > > +                fvp.start(fvpconf.name)
> > > 
> > >               m.assert_called_once_with(['/usr/bin/FVP_Binary',
> > >                   '--parameter', 'foo=bar',
> > > @@ -114,16 +119,20 @@ class RunnerTests(OESelftestTestCase):
> > >           from fvp import runner
> > >           with self.create_mock() as m:
> > >               fvp = runner.FVPRunner(self.logger)
> > > -            fvp.start({
> > > -                "fvp-bindir": "/usr/bin",
> > > -                "exe": "FVP_Binary",
> > > -                "parameters": {},
> > > -                "data": [],
> > > -                "applications": {},
> > > -                "terminals": {},
> > > -                "args": [],
> > > -                "env": {"FOO": "BAR"}
> > > -            })
> > > +            config = {"fvp-bindir": "/usr/bin",
> > > +                      "exe": "FVP_Binary",
> > > +                      "parameters": {},
> > > +                      "data": [],
> > > +                      "applications": {},
> > > +                      "terminals": {},
> > > +                      "args": [],
> > > +                      "env": {"FOO": "BAR"}
> > > +                     }
> > > +
> > > +            with tempfile.NamedTemporaryFile('w') as fvpconf:
> > > +                json.dump(config, fvpconf)
> > > +                fvpconf.flush()
> > > +                fvp.start(fvpconf.name)
> > > 
> > >               m.assert_called_once_with(['/usr/bin/FVP_Binary'],
> > >                   stdin=unittest.mock.ANY,
> > > diff --git a/scripts/runfvp b/scripts/runfvp
> > > index e4b00abc..c2e536c8 100755
> > > --- a/scripts/runfvp
> > > +++ b/scripts/runfvp
> > > @@ -14,7 +14,7 @@ logger = logging.getLogger("RunFVP")
> > >   libdir = pathlib.Path(__file__).parents[1] / "meta-arm" / "lib"
> > >   sys.path.insert(0, str(libdir))
> > > 
> > > -from fvp import terminal, runner, conffile
> > > +from fvp import terminal, runner
> > > 
> > >   def parse_args(arguments):
> > >       import argparse
> > > @@ -49,12 +49,13 @@ def parse_args(arguments):
> > >       logger.debug(f"FVP arguments: {fvp_args}")
> > >       return args, fvp_args
> > > 
> > > -def start_fvp(args, config, extra_args):
> > > +def start_fvp(args, fvpconf, extra_args):
> > >       fvp = runner.FVPRunner(logger)
> > >       try:
> > > -        fvp.start(config, extra_args, args.terminals)
> > > +        fvp.start(fvpconf, extra_args, args.terminals)
> > > 
> > >           if args.console:
> > > +            config = fvp.getConfig()
> > >               expected_terminal = config["consoles"].get("default")
> > >               if expected_terminal is None:
> > >                   logger.error("--console used but FVP_CONSOLE not set in machine configuration")
> > > @@ -87,9 +88,7 @@ def runfvp(cli_args):
> > >           config_file = args.config
> > >       else:
> > >           config_file = conffile.find(args.config)
> > > -    logger.debug(f"Loading {config_file}")
> > > -    config = conffile.load(config_file)
> > > -    start_fvp(args, config, extra_args)
> > > +    start_fvp(args, config_file, extra_args)
> > > 
> > > 
> > >   if __name__ == "__main__":
> > > --
> > > 2.40.1
> > > 
>