Code review cleanup

Author: solidpixel

.github/workflows/build_test.yaml

@@ -39,6 +39,11 @@ jobs:
           python3 -m mypy ./lglpy
           python3 -m mypy ./generator
 
+      - name: Run unit tests
+        # Note: Only run tests that do not require a connected device
+        run: |
+          python3 -m lglpy.ui.test
+
   build-ubuntu-x64-clang:
     name: Ubuntu x64 Clang
     runs-on: ubuntu-22.04

generator/generate_vulkan_common.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 # SPDX-License-Identifier: MIT
 # -----------------------------------------------------------------------------
-# Copyright (c) 2024 Arm Limited
+# Copyright (c) 2024-2025 Arm Limited
 #
 # Permission is hereby granted, free of charge, to any person obtaining a copy
 # of this software and associated documentation files (the "Software"), to

lgl_host_server.py

@@ -1,7 +1,7 @@
 #!/bin/env python3
 # SPDX-License-Identifier: MIT
 # -----------------------------------------------------------------------------
-# Copyright (c) 2024 Arm Limited
+# Copyright (c) 2024-2025 Arm Limited
 #
 # Permission is hereby granted, free of charge, to any person obtaining a copy
 # of this software and associated documentation files (the 'Software'), to

lglpy/android/adb.py

@@ -38,6 +38,17 @@ class ADBConnect:
     A wrapper around adb which can be used to connect to a specific device,
     and run commands as a specific package.
 
+      - adb() runs a command using adb and waits for the result.
+      - adb_async() runs a command using  adb and does not wait for the result.
+      - adb_run() runs a device shell command as the "shell" user and waits for
+        the result.
+      - adb_runas() runs a device shell command as the current package user and
+        waits for the result.
+
+    The current device and package are attributes of the connection instance
+    which can be set at construction, or via the set_device() and set_package()
+    methods.
+
     Attributes:
         device: The name of the connected device, or None for generic use.
         package: The name of the debuggable package, or None for generic use.
@@ -159,9 +170,7 @@ def adb(self, *args: str, text: bool = True, shell: bool = False,
 
         # Invoke the command
         rep = sp.run(packed_commands, check=check, shell=shell, text=text,
-                     stdin=sp.DEVNULL,
-                     stdout=sp.PIPE,
-                     stderr=sp.PIPE)
+                     stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE)
 
         # Return the output
         return rep.stdout
@@ -207,9 +216,7 @@ def adb_async(self, *args: str, text: bool = True, shell: bool = False,
         # pylint: disable=consider-using-with
         process = sp.Popen(packed_commands,
                            text=text, shell=shell,
-                           stdin=sp.DEVNULL,
-                           stdout=output,
-                           stderr=sp.DEVNULL)
+                           stdin=sp.DEVNULL, stdout=output, stderr=sp.DEVNULL)
 
         # Return the output process a user can use to wait, if needed.
         return process
@@ -244,10 +251,9 @@ def adb_run(self, *args: str, text: bool = True, shell: bool = False,
         packed_commands = self.pack_commands(commands, shell, quote)
 
         # Invoke the command
-        rep = sp.run(packed_commands, check=check, shell=shell, text=text,
-                     stdin=sp.DEVNULL,
-                     stdout=sp.PIPE,
-                     stderr=sp.PIPE)
+        rep = sp.run(packed_commands,
+                     check=check, shell=shell, text=text,
+                     stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE)
 
         # Return the output
         return rep.stdout
@@ -285,10 +291,9 @@ def adb_runas(self, *args: str, text: bool = True, shell: bool = False,
         packed_commands = self.pack_commands(commands, shell, quote)
 
         # Invoke the command
-        rep = sp.run(packed_commands, check=check, shell=shell, text=text,
-                     stdin=sp.DEVNULL,
-                     stdout=sp.PIPE,
-                     stderr=sp.PIPE)
+        rep = sp.run(packed_commands,
+                     check=check, shell=shell, text=text,
+                     stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE)
 
         # Return the output
         return rep.stdout

lglpy/android/filesystem.py

@@ -22,8 +22,8 @@
 # -----------------------------------------------------------------------------
 
 '''
-This module implements higher level Android queries and utilities, built on top
-of the low level Android Debug Bridge wrapper.
+This module implements higher level Android filesystem utilities, built on top
+of the low level ADBConnect wrapper around Android Debug Bridge.
 '''
 
 import os
@@ -36,6 +36,11 @@
 class AndroidFilesystem:
     '''
     A library of utility methods for transferring files to/from a device.
+
+    Attributes:
+        TEMP_DIR: The generally accessible device temp directory.
+        DATA_PERM: The file permissions to use for data files.
+        EXEC_PERM: The file permissions to use for executable files.
     '''
 
     TEMP_DIR = '/data/local/tmp'
@@ -49,12 +54,12 @@ def push_file_to_tmp(
         '''
         Push a file to the device temp directory.
 
-        File will be copied to: /data/local/tmp/<file>.
+        File will be copied to: TEMP_DIR/<file>.
 
         Args:
             conn: The adb connection.
             host_path: The path of the file on the host file system.
-            executable: True if the file should be configured as executable.
+            executable: True if the file should have executable permissions.
 
         Returns:
             True if the file was copied, False otherwise.
@@ -65,16 +70,17 @@ def push_file_to_tmp(
 
         try:
             # Remove old file to prevent false success
-            conn.adb('shell', 'rm', '-f', device_path)
+            conn.adb_run('rm', '-f', device_path)
 
             # Push new file
             conn.adb('push', host_path, device_path)
 
             # Check it actually copied
-            conn.adb('shell', 'ls', device_path)
+            conn.adb_run('ls', device_path)
 
             permission = cls.EXEC_PERM if executable else cls.DATA_PERM
-            conn.adb('shell', 'chmod', permission, device_path)
+            conn.adb_run('chmod', permission, device_path)
+
         except sp.CalledProcessError:
             return False
 
@@ -94,7 +100,7 @@ def pull_file_from_tmp(
             file_name: The name of the file in the tmp directory.
             host_path: The destination directory on the host file system.
                Host directory will be created if it doesn't exist.
-            delete: Delete the file on the device after copying it.
+            delete: Should the file on the device be deleted after copying?
 
         Returns:
             True if the file was copied, False otherwise.
@@ -121,7 +127,7 @@ def delete_file_from_tmp(
         '''
         Delete a file from the device temp directory.
 
-        File will be deleted from: /data/local/tmp/<file>.
+        File will be deleted from: TEMP_DIR/<file>.
 
         Args:
             conn: The adb connection.
@@ -135,9 +141,9 @@ def delete_file_from_tmp(
 
         try:
             if error_ok:
-                conn.adb('shell', 'rm', '-f', device_path)
+                conn.adb_run('rm', '-f', device_path)
             else:
-                conn.adb('shell', 'rm', device_path)
+                conn.adb_run('rm', device_path)
         except sp.CalledProcessError:
             return False
 
@@ -155,7 +161,7 @@ def push_file_to_package(
         Args:
             conn: The adb connection.
             host_path: The path of the file on the host file system.
-            executable: True if the file should be configured as executable.
+            executable: True if the file should have executable permissions.
 
         Returns:
             True if the file was copied, False otherwise.
@@ -197,7 +203,7 @@ def pull_file_from_package(
             src_file: The name of the file in the tmp directory.
             host_path: The destination directory on the host file system.
                Host directory will be created if it doesn't exist.
-            delete: Delete the file on the device after copying it.
+            delete: Should the file on the device be deleted after copying?
 
         Returns:
             True if the file was copied, False otherwise.
@@ -218,6 +224,7 @@ def pull_file_from_package(
 
             if delete:
                 cls.delete_file_from_package(conn, src_file)
+
         except sp.SubprocessError:
             return False
 
@@ -227,13 +234,13 @@ def pull_file_from_package(
     def rename_file_in_package(
             cls, conn: ADBConnect, file_name: str, new_file_name: str) -> bool:
         '''
-        Delete a file from the package directory.
+        Rename a file in the package directory.
 
-        File will be deleted from, e.g.: /data/user/0/<package>/<file>
+        File will be renamed to, e.g.: /data/user/0/<package>/<new_file>
 
         Args:
             conn: The adb connection.
-            file_name: The name of the file to rename.
+            file_name: The name of the existing file to rename.
             new_file_name: The new file name to use.
 
         Returns:

lglpy/android/test.py

@@ -40,7 +40,8 @@
 from .utils import AndroidUtils
 from .filesystem import AndroidFilesystem
 
-SLOW_TESTS = False  # Set to True to enable slot tests, False to skip them
+
+SLOW_TESTS = True  # Set to True to enable slow tests, False to skip them
 
 
 def get_script_relative_path(file_name: str) -> str:
@@ -564,6 +565,46 @@ def test_util_copy_to_package_exec(self):
         success = AndroidFilesystem.delete_file_from_package(conn, test_file)
         self.assertTrue(success)
 
+    def test_util_rename_in_package(self):
+        '''
+        Test filesystem rename in package data directory.
+        '''
+        conn = ADBConnect()
+
+        # Fetch some packages that we can use
+        packages = AndroidUtils.get_packages(conn, True, False)
+        self.assertGreater(len(packages), 0)
+        conn.set_package(packages[0])
+
+        test_file = 'test_data.txt'
+        test_path = get_script_relative_path(test_file)
+
+        # Push the file
+        success = AndroidFilesystem.push_file_to_package(conn, test_path)
+        self.assertTrue(success)
+
+        # Validate it pushed OK
+        data = conn.adb_runas('ls', test_file)
+        self.assertEqual(data.strip(), test_file)
+
+        # Rename the file
+        new_test_file = 'test_data_2.txt'
+        success = AndroidFilesystem.rename_file_in_package(
+            conn, test_file, new_test_file)
+        self.assertTrue(success)
+
+        # Validate it was moved - this should fail
+        with self.assertRaises(sp.CalledProcessError):
+            data = conn.adb_runas('ls', test_file)
+
+        data = conn.adb_runas('ls', new_test_file)
+        self.assertEqual(data.strip(), new_test_file)
+
+        # Cleanup tmp - this should fail because the file does not exist
+        success = AndroidFilesystem.delete_file_from_package(
+            conn, new_test_file)
+        self.assertTrue(success)
+
     def test_util_copy_from_package(self):
         '''
         Test filesystem copy from package data directory to host.