From 3b5d269a8285e2da1bf3346c3acd6071efcd6aec Mon Sep 17 00:00:00 2001 From: Bastian Krause Date: Thu, 25 Sep 2025 16:08:54 +0200 Subject: [PATCH 1/4] tests/client: move place creation to new create_place() factory fixture This allows tests to create multiple places on demand. The places are released and deleted afterwards. Signed-off-by: Bastian Krause --- tests/test_client.py | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index 34297e7eb..2351bb252 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -9,22 +9,33 @@ def test_startup(coordinator): pass @pytest.fixture(scope='function') -def place(coordinator): - with pexpect.spawn('python -m labgrid.remote.client -p test create') as spawn: - spawn.expect(pexpect.EOF) - spawn.close() +def place(create_place): + create_place('test') + +@pytest.fixture(scope='function') +def create_place(coordinator): + place_names = [] + + def _create_place(place_name): + with pexpect.spawn(f'python -m labgrid.remote.client -p {place_name} create') as spawn: + spawn.expect(pexpect.EOF) assert spawn.exitstatus == 0, spawn.before.strip() - with pexpect.spawn('python -m labgrid.remote.client -p test set-tags board=123board') as spawn: - spawn.expect(pexpect.EOF) - spawn.close() + place_names.append(place_name) + + with pexpect.spawn(f'python -m labgrid.remote.client -p {place_name} set-tags board=123board') as spawn: + spawn.expect(pexpect.EOF) assert spawn.exitstatus == 0, spawn.before.strip() - yield + yield _create_place - with pexpect.spawn('python -m labgrid.remote.client -p test delete') as spawn: - spawn.expect(pexpect.EOF) - spawn.close() + for place_name in place_names: + # clean up + with pexpect.spawn(f'python -m labgrid.remote.client -p {place_name} release') as spawn: + spawn.expect(pexpect.EOF) + + with pexpect.spawn(f'python -m labgrid.remote.client -p {place_name} delete') as spawn: + spawn.expect(pexpect.EOF) assert spawn.exitstatus == 0, spawn.before.strip() @pytest.fixture(scope='function') From 8f94eb64f22538e1cbad029634985387cb390a83 Mon Sep 17 00:00:00 2001 From: Bastian Krause Date: Thu, 11 Sep 2025 17:28:56 +0200 Subject: [PATCH 2/4] remote/client: prepare handling of multiple Errors A future commit will introduce ClientSession code that needs to handle multiple Error exceptions, aggregated into a dedicated group. Python 3.11 introduced support for exception groups [1]. This allows raising multiple exceptions (not caused by one another) while keeping their individual tracebacks etc. intact (i.e. `labgrid-client -d` will still show all individual tracebacks). Since labgrid will drop Python 3.10 support in October 2026 earliest, use the backported version of exception groups until then. While at it, add a Error.__str__() method, so Error and ErrorGroup can use the same print to stderr statement in the except clause. [1] https://docs.python.org/3/library/exceptions.html#exception-groups Signed-off-by: Bastian Krause --- labgrid/remote/client.py | 17 ++++++++++++++--- pyproject.toml | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/labgrid/remote/client.py b/labgrid/remote/client.py index 27108a7b4..e1048045a 100755 --- a/labgrid/remote/client.py +++ b/labgrid/remote/client.py @@ -28,6 +28,9 @@ import attr import grpc +# TODO: drop if Python >= 3.11 guaranteed +from exceptiongroup import ExceptionGroup # pylint: disable=redefined-builtin + from .common import ( ResourceEntry, ResourceMatch, @@ -57,7 +60,8 @@ class Error(Exception): - pass + def __str__(self): + return f"Error: {' '.join(self.args)}" class UserError(Error): @@ -72,6 +76,13 @@ class InteractiveCommandError(Error): pass +class ErrorGroup(ExceptionGroup): + def __str__(self): + # TODO: drop pylint disable once https://github.com/pylint-dev/pylint/issues/8985 is fixed + errors_combined = "\n".join(f"- {' '.join(e.args)}" for e in self.exceptions) # pylint: disable=not-an-iterable + return f"{self.message}:\n{errors_combined}" + + @attr.s(eq=False) class ClientSession: """The ClientSession encapsulates all the actions a Client can invoke on @@ -2212,11 +2223,11 @@ def main(): if args.debug: traceback.print_exc(file=sys.stderr) exitcode = e.exitcode - except Error as e: + except (Error, ErrorGroup) as e: if args.debug: traceback.print_exc(file=sys.stderr) else: - print(f"{parser.prog}: error: {e}", file=sys.stderr) + print(f"{parser.prog}: {e}", file=sys.stderr) exitcode = 1 except KeyboardInterrupt: exitcode = 1 diff --git a/pyproject.toml b/pyproject.toml index 33caa6f31..424226533 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,6 +33,7 @@ classifiers = [ ] dependencies = [ "attrs>=21.4.0", + "exceptiongroup>=1.3.0", # TODO: drop if Python >= 3.11 guaranteed "grpcio>=1.64.1, <2.0.0", "grpcio-reflection>=1.64.1, <2.0.0", "protobuf>=5.27.0", From a9b59e4e88d7e4c24a1966f3167fa7709a42d05e Mon Sep 17 00:00:00 2001 From: Bastian Krause Date: Thu, 11 Sep 2025 17:38:24 +0200 Subject: [PATCH 3/4] remote/client: make acquire/release operate on all RemotePlaces in env If an environment config with multiple RemotePlaces is given via `-c`/`--config`, only the first RemotePlace is used on acquire/lock and release/unlock. Having multiple RemotePlaces is meant for multi place testing, so all RemotePlaces need to be acquired/released. This is implemented in a best effort way: If an error occurs, the remaining places are still tried to be acquired/released. A combined error message is shown if the operation failed on multiple places. When called with `-d`, tracebacks of all errors are shown. If any of the operations failed, the process exits with 1. CI pipelines should use this pattern: ```shell export LG_ENV=env.yaml labgrid-client unlock || true labgrid-client lock ``` Signed-off-by: Bastian Krause --- labgrid/remote/client.py | 43 ++++++++++++++++++++++++++++++++++++++-- tests/test_client.py | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/labgrid/remote/client.py b/labgrid/remote/client.py index e1048045a..735636020 100755 --- a/labgrid/remote/client.py +++ b/labgrid/remote/client.py @@ -489,6 +489,17 @@ def get_place(self, place=None): raise UserError(f"pattern {pattern} matches multiple places ({', '.join(places)})") return self.places[places[0]] + def get_place_names_from_env(self): + """Returns a list of RemotePlace names found in the environment config.""" + places = [] + for role_config in self.env.config.get_targets().values(): + resources, _ = target_factory.normalize_config(role_config) + remote_places = resources.get("RemotePlace", []) + for place in remote_places: + places.append(place) + + return places + def get_idle_place(self, place=None): place = self.get_place(place) if place.acquired: @@ -692,8 +703,22 @@ def check_matches(self, place): raise UserError(f"Match {match} has no matching remote resource") async def acquire(self): + errors = [] + places = self.get_place_names_from_env() if self.env else [self.args.place] + for place in places: + try: + await self._acquire_place(place) + except Error as e: + errors.append(e) + + if errors: + if len(errors) == 1: + raise errors[0] + raise ErrorGroup("Multiple errors occurred during acquire", errors) + + async def _acquire_place(self, place): """Acquire a place, marking it unavailable for other clients""" - place = self.get_place() + place = self.get_place(place) if place.acquired: host, user = place.acquired.split("/") allowhelp = f"'labgrid-client -p {place.name} allow {self.gethostname()}/{self.getuser()}' on {host}." @@ -738,8 +763,22 @@ async def acquire(self): raise ServerError(e.details()) async def release(self): + errors = [] + places = self.get_place_names_from_env() if self.env else [self.args.place] + for place in places: + try: + await self._release_place(place) + except Error as e: + errors.append(e) + + if errors: + if len(errors) == 1: + raise errors[0] + raise ErrorGroup("Multiple errors occurred during release", errors) + + async def _release_place(self, place): """Release a previously acquired place""" - place = self.get_place() + place = self.get_place(place) if not place.acquired: raise UserError(f"place {place.name} is not acquired") _, user = place.acquired.split("/") diff --git a/tests/test_client.py b/tests/test_client.py index 2351bb252..8d2cc1c9f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -162,6 +162,49 @@ def test_place_acquire(place): spawn.close() assert spawn.exitstatus == 0, spawn.before.strip() +def test_place_acquire_multiple(create_place, tmpdir): + # create multiple places + place_names = ['test1', 'test2'] + for place_name in place_names: + create_place(place_name) + + # create env config with multiple RemotePlaces + p = tmpdir.join('config.yaml') + p.write('targets:') + for place_name in place_names: + p.write( + f""" + {place_name}: + resources: + RemotePlace: + name: {place_name} + """, + mode='a', + ) + + # acquire all places in env config + with pexpect.spawn(f'python -m labgrid.remote.client -c {p} acquire') as spawn: + spawn.expect(pexpect.EOF) + assert spawn.exitstatus == 0, spawn.before.strip() + + # check 'who' + with pexpect.spawn('python -m labgrid.remote.client who') as spawn: + spawn.expect(pexpect.EOF) + for place_name in place_names: + assert place_name.encode('utf-8') in spawn.before + + assert spawn.exitstatus == 0, spawn.before.strip() + + # release all places in env config + with pexpect.spawn(f'python -m labgrid.remote.client -c {p} release') as spawn: + spawn.expect(pexpect.EOF) + assert spawn.exitstatus == 0, spawn.before.strip() + + # check 'who' again + with pexpect.spawn('python -m labgrid.remote.client who') as spawn: + spawn.expect('User.*Host.*Place.*Changed\r\n') + assert not spawn.before, spawn.before + def test_place_acquire_enforce(place): with pexpect.spawn('python -m labgrid.remote.client -p test add-match does/not/exist') as spawn: spawn.expect(pexpect.EOF) From 829079f215225159df88d4304e27f3f9b325b4a1 Mon Sep 17 00:00:00 2001 From: Bastian Krause Date: Wed, 10 Sep 2025 17:35:35 +0200 Subject: [PATCH 4/4] remote/client: include place name in acquire error messages When acquiring multiple places at once via an environment config, the user needs to know which places were affected by the error. Signed-off-by: Bastian Krause --- labgrid/remote/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/labgrid/remote/client.py b/labgrid/remote/client.py index 735636020..5a6c550c9 100755 --- a/labgrid/remote/client.py +++ b/labgrid/remote/client.py @@ -724,10 +724,10 @@ async def _acquire_place(self, place): allowhelp = f"'labgrid-client -p {place.name} allow {self.gethostname()}/{self.getuser()}' on {host}." if self.getuser() == user: if self.gethostname() == host: - raise UserError("You have already acquired this place.") + raise UserError(f"You have already acquired place {place.name}.") else: raise UserError( - f"You have already acquired this place on {host}. To work simultaneously, execute {allowhelp}" + f"You have already acquired place {place.name} on {host}. To work simultaneously, execute {allowhelp}" ) else: raise UserError(