| Message ID | 20230517100913.96055-1-peron.clem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,1/3] runfvp: make fvp runner to hold the config | expand |
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 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 >
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! 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 >>
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 > > > >
Hi Jon, Peter, On Wed, 24 May 2023 at 01:36, Jon Mason <jdmason@kudzu.us> wrote: > > 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. That would be great, the last missing piece is the gator-daemon. I'm now building a FVP image and running it inside Docker to get a "almost deterministic" ARM benchmark running on different x86 hardware, that's cool :). I'm not sure how people deal with benchmarks in CI, but this one seems really stable. Regards, Clement > > 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 > > > > > >
On Wed, 17 May 2023 12:09:11 +0200, Clément Péron wrote: > At the moment the config is load and pass to FVPRunner. > > Change the ownership to FVPRunner. Applied, thanks! [1/3] runfvp: make fvp runner to hold the config commit: 272359be5dbf8047294e519a512f4dd6348bde5d [2/3] fvp: runner: execute fvp process in the same working directory as fvpconf commit: 1fa602ad3ba9356110c085cb9d92286d691294f9 [3/3] runfvp: update filepath in fvpconf to relative path commit: cb31d9e5981074f083ea287b6cf8fb3460722db8 Best regards,
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__":
At the moment the config is load and pass to FVPRunner. Change the ownership to FVPRunner. 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(-)