Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220118190922.1557074-1-dlatypov@google.com> <20220118190922.1557074-5-dlatypov@google.com>
In-Reply-To: <20220118190922.1557074-5-dlatypov@google.com>
From:   David Gow <davidgow@google.com>
Date:   Thu, 20 Jan 2022 16:30:16 +0800
Message-ID: <CABVgOSk621CwPZ9LMdLuY7WkmDfoS6_zpeyphCubWPGGiWLvKg@mail.gmail.com>
Subject: Re: [PATCH 5/5] kunit: tool: simplify code since build_dir can't be None
To:     Daniel Latypov <dlatypov@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>