Re: [PATCH 5/5] kunit: tool: simplify code since build_dir can't be None

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com wrote:

--build_dir is set to a default of '.kunit' since commit ddbd60c779b4 ("kunit: use --build_dir=.kunit as default"), but even before then it was explicitly set to ''.

So outside of one unit test, there was no way for the build_dir to be ever be None, and we can simplify code by fixing the unit test and enforcing that via updated type annotations.

E.g. this lets us drop `get_file_path()` since it's now exactly equivalent to os.path.join().

Note: there's some `if build_dir` checks that also fail if build_dir is explicitly set to '' that just guard against passing "O=" to make. But running `make O=` works just fine, so drop these checks.

Signed-off-by: Daniel Latypov dlatypov@google.com

Looks good to me. Passing "--build_dir=" to KUnit didn't work before, as well, as "" is not considered a valid path. You've got to specify --build-dir=. to get anything useful to happen.

Reviewed-by: David Gow davidgow@google.com

-- David

tools/testing/kunit/kunit_json.py | 8 ++-- tools/testing/kunit/kunit_kernel.py | 51 ++++++++++---------------- tools/testing/kunit/kunit_tool_test.py | 2 +- 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py index 61091878f51e..24d103049bca 100644 --- a/tools/testing/kunit/kunit_json.py +++ b/tools/testing/kunit/kunit_json.py @@ -12,12 +12,11 @@ import os import kunit_parser

from kunit_parser import Test, TestStatus -from typing import Any, Dict, Optional +from typing import Any, Dict

JsonObj = Dict[str, Any]

-def _get_group_json(test: Test, def_config: str,

•           build_dir: Optional[str]) -> JsonObj:
  

+def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj: sub_groups = [] # List[JsonObj] test_cases = [] # List[JsonObj]

@@ -50,8 +49,7 @@ def _get_group_json(test: Test, def_config: str, } return test_group

-def get_json_result(test: Test, def_config: str,

•           build_dir: Optional[str]) -> str:
  

+def get_json_result(test: Test, def_config: str, build_dir: str) -> str: test_group = _get_group_json(test, def_config, build_dir) test_group["name"] = "KUnit Test Group" return json.dumps(test_group, indent=4) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 44bbe54f25f1..fe159e7ff697 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -28,11 +28,6 @@ OUTFILE_PATH = 'test.log' ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__)) QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs')

-def get_file_path(build_dir, default):

•   if build_dir:
  

•           default = os.path.join(build_dir, default)
  

•   return default
  

class ConfigError(Exception): """Represents an error trying to configure the Linux kernel."""

@@ -59,17 +54,15 @@ class LinuxSourceTreeOperations(object): def make_arch_qemuconfig(self, kconfig: kunit_config.Kconfig) -> None: pass

•   def make_allyesconfig(self, build_dir, make_options) -> None:
  

•   def make_allyesconfig(self, build_dir: str, make_options) -> None:
            raise ConfigError('Only the "um" arch is supported for alltests')
  
  

•   def make_olddefconfig(self, build_dir, make_options) -> None:
  

•           command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
  

•   def make_olddefconfig(self, build_dir: str, make_options) -> None:
  

•           command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, 'olddefconfig']
            if self._cross_compile:
                    command += ['CROSS_COMPILE=' + self._cross_compile]
            if make_options:
                    command.extend(make_options)
  

•           if build_dir:
  

•                   command += ['O=' + build_dir]
            print('Populating config with:\n$', ' '.join(command))
            try:
                    subprocess.check_output(command, stderr=subprocess.STDOUT)
  

@@ -78,14 +71,12 @@ class LinuxSourceTreeOperations(object): except subprocess.CalledProcessError as e: raise ConfigError(e.output.decode())

•   def make(self, jobs, build_dir, make_options) -> None:
  

•           command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
  

•   def make(self, jobs, build_dir: str, make_options) -> None:
  

•           command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, '--jobs=' + str(jobs)]
            if make_options:
                    command.extend(make_options)
            if self._cross_compile:
                    command += ['CROSS_COMPILE=' + self._cross_compile]
  

•           if build_dir:
  

•                   command += ['O=' + build_dir]
            print('Building with:\n$', ' '.join(command))
            try:
                    proc = subprocess.Popen(command,
  

@@ -143,14 +134,12 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): def __init__(self, cross_compile=None): super().__init__(linux_arch='um', cross_compile=cross_compile)

•   def make_allyesconfig(self, build_dir, make_options) -> None:
  

•   def make_allyesconfig(self, build_dir: str, make_options) -> None:
            kunit_parser.print_with_timestamp(
                    'Enabling all CONFIGs for UML...')
  

•           command = ['make', 'ARCH=um', 'allyesconfig']
  

•           command = ['make', 'ARCH=um', 'O=' + build_dir, 'allyesconfig']
            if make_options:
                    command.extend(make_options)
  

•           if build_dir:
  

•                   command += ['O=' + build_dir]
            process = subprocess.Popen(
                    command,
                    stdout=subprocess.DEVNULL,
  

@@ -167,24 +156,24 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):

    def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
            """Runs the Linux UML binary. Must be named 'linux'."""

•           linux_bin = get_file_path(build_dir, 'linux')
  

•           linux_bin = os.path.join(build_dir, 'linux')
            return subprocess.Popen([linux_bin] + params,
                                       stdin=subprocess.PIPE,
                                       stdout=subprocess.PIPE,
                                       stderr=subprocess.STDOUT,
                                       text=True, errors='backslashreplace')
  
  

-def get_kconfig_path(build_dir) -> str:

•   return get_file_path(build_dir, KCONFIG_PATH)
  

+def get_kconfig_path(build_dir: str) -> str:

•   return os.path.join(build_dir, KCONFIG_PATH)
  
  

-def get_kunitconfig_path(build_dir) -> str:

•   return get_file_path(build_dir, KUNITCONFIG_PATH)
  

+def get_kunitconfig_path(build_dir: str) -> str:

•   return os.path.join(build_dir, KUNITCONFIG_PATH)
  
  

-def get_old_kunitconfig_path(build_dir) -> str:

•   return get_file_path(build_dir, OLD_KUNITCONFIG_PATH)
  

+def get_old_kunitconfig_path(build_dir: str) -> str:

•   return os.path.join(build_dir, OLD_KUNITCONFIG_PATH)
  
  

-def get_outfile_path(build_dir) -> str:

•   return get_file_path(build_dir, OUTFILE_PATH)
  

+def get_outfile_path(build_dir: str) -> str:

•   return os.path.join(build_dir, OUTFILE_PATH)
  
  

def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceTreeOperations: config_path = os.path.join(QEMU_CONFIGS_DIR, arch + '.py') @@ -268,7 +257,7 @@ class LinuxSourceTree(object): return False return True

•   def validate_config(self, build_dir) -> bool:
  

•   def validate_config(self, build_dir: str) -> bool:
            kconfig_path = get_kconfig_path(build_dir)
            validated_kconfig = kunit_config.parse_file(kconfig_path)
            if self._kconfig.is_subset_of(validated_kconfig):
  

@@ -283,7 +272,7 @@ class LinuxSourceTree(object): logging.error(message) return False

•   def build_config(self, build_dir, make_options) -> bool:
  

•   def build_config(self, build_dir: str, make_options) -> bool:
            kconfig_path = get_kconfig_path(build_dir)
            if build_dir and not os.path.exists(build_dir):
                    os.mkdir(build_dir)
  

@@ -311,7 +300,7 @@ class LinuxSourceTree(object): old_kconfig = kunit_config.parse_file(old_path) return old_kconfig.entries() != self._kconfig.entries()

•   def build_reconfig(self, build_dir, make_options) -> bool:
  

•   def build_reconfig(self, build_dir: str, make_options) -> bool:
            """Creates a new .config if it is not a subset of the .kunitconfig."""
            kconfig_path = get_kconfig_path(build_dir)
            if not os.path.exists(kconfig_path):
  

@@ -326,7 +315,7 @@ class LinuxSourceTree(object): os.remove(kconfig_path) return self.build_config(build_dir, make_options)

•   def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
  

•   def build_kernel(self, alltests, jobs, build_dir: str, make_options) -> bool:
            try:
                    if alltests:
                            self._ops.make_allyesconfig(build_dir, make_options)
  

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index f7cbc248a405..a3c036a620b2 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -469,7 +469,7 @@ class KUnitJsonTest(unittest.TestCase): json_obj = kunit_json.get_json_result( test=test_result, def_config='kunit_defconfig',

•                           build_dir=None)
  

•                           build_dir='.kunit')
            return json.loads(json_obj)
  
    def test_failed_test_json(self):
  

-- 2.34.1.703.g22d0c6ccf7-goog