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

From: David Gow
Date: Thu Jan 20 2022 - 03:31:04 EST

Next message: kernel test robot: "[intel-tdx:guest-upstream 15/35] ld: Unexpected run-time relocations (.rel) detected!"
Previous message: David Gow: "Re: [PATCH 4/5] kunit: tool: drop last uses of collections.namedtuple"
In reply to: Daniel Latypov: "[PATCH 5/5] kunit: tool: simplify code since build_dir can't be None"
Next in thread: David Gow: "Re: [PATCH 1/5] kunit: tool: drop mostly unused KunitResult.result field"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---

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@xxxxxxxxxx>

-- 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
>

Next message: kernel test robot: "[intel-tdx:guest-upstream 15/35] ld: Unexpected run-time relocations (.rel) detected!"
Previous message: David Gow: "Re: [PATCH 4/5] kunit: tool: drop last uses of collections.namedtuple"
In reply to: Daniel Latypov: "[PATCH 5/5] kunit: tool: simplify code since build_dir can't be None"
Next in thread: David Gow: "Re: [PATCH 1/5] kunit: tool: drop mostly unused KunitResult.result field"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]