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: <20211106013058.2621799-1-dlatypov@google.com>
In-Reply-To: <20211106013058.2621799-1-dlatypov@google.com>
From:   David Gow <davidgow@google.com>
Date:   Sat, 6 Nov 2021 12:06:52 +0800
Message-ID: <CABVgOSnKdo1U6QFvQ3XiMoWmony93auzYyaMXs9f7UXa3GpGgg@mail.gmail.com>
Subject: Re: [PATCH 1/2] kunit: tool: move Kconfig read_from_file/parse_from_string
 to package-level
To:     Daniel Latypov <dlatypov@google.com>
Cc:     brendanhiggins@google.com, linux-kernel@vger.kernel.org,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        skhan@linuxfoundation.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Nov 6, 2021 at 9:31 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> read_from_file() clears its `self` Kconfig object and parses a config
> file.
>
> It is a way to construct Kconfig objects more so than an operation on
> Kconfig objects. This is reflected in the fact its only ever used as:
>   kconfig = kunit_config.Kconfig()
>   kconfig.read_from_file(path)
>
> So clean this up and simplify callers by replacing it with
>   kconfig = kunit_config.parse_file(path)
>
> Do the same thing for the related parse_from_string() function as well.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Looks sensible, works fine.

Reviewed-by: David Gow <davidgow@google.com>

Thanks,
-- David


>  tools/testing/kunit/kunit_config.py    | 61 +++++++++++++-------------
>  tools/testing/kunit/kunit_kernel.py    | 12 ++---
>  tools/testing/kunit/kunit_tool_test.py |  6 +--
>  3 files changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index c77c7d2ef622..677354546156 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -62,33 +62,34 @@ class Kconfig(object):
>                         for entry in self.entries():
>                                 f.write(str(entry) + '\n')
>
> -       def parse_from_string(self, blob: str) -> None:
> -               """Parses a string containing KconfigEntrys and populates this Kconfig."""
> -               self._entries = []
> -               is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN)
> -               config_matcher = re.compile(CONFIG_PATTERN)
> -               for line in blob.split('\n'):
> -                       line = line.strip()
> -                       if not line:
> -                               continue
> -
> -                       match = config_matcher.match(line)
> -                       if match:
> -                               entry = KconfigEntry(match.group(1), match.group(2))
> -                               self.add_entry(entry)
> -                               continue
> -
> -                       empty_match = is_not_set_matcher.match(line)
> -                       if empty_match:
> -                               entry = KconfigEntry(empty_match.group(1), 'n')
> -                               self.add_entry(entry)
> -                               continue
> -
> -                       if line[0] == '#':
> -                               continue
> -                       else:
> -                               raise KconfigParseError('Failed to parse: ' + line)
> -
> -       def read_from_file(self, path: str) -> None:
> -               with open(path, 'r') as f:
> -                       self.parse_from_string(f.read())
> +def parse_file(path: str) -> Kconfig:
> +       with open(path, 'r') as f:
> +               return parse_from_string(f.read())
> +
> +def parse_from_string(blob: str) -> Kconfig:
> +       """Parses a string containing Kconfig entries."""
> +       kconfig = Kconfig()
> +       is_not_set_matcher = re.compile(CONFIG_IS_NOT_SET_PATTERN)
> +       config_matcher = re.compile(CONFIG_PATTERN)
> +       for line in blob.split('\n'):
> +               line = line.strip()
> +               if not line:
> +                       continue
> +
> +               match = config_matcher.match(line)
> +               if match:
> +                       entry = KconfigEntry(match.group(1), match.group(2))
> +                       kconfig.add_entry(entry)
> +                       continue
> +
> +               empty_match = is_not_set_matcher.match(line)
> +               if empty_match:
> +                       entry = KconfigEntry(empty_match.group(1), 'n')
> +                       kconfig.add_entry(entry)
> +                       continue
> +
> +               if line[0] == '#':
> +                       continue
> +               else:
> +                       raise KconfigParseError('Failed to parse: ' + line)
> +       return kconfig
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 66095568bf32..51ee6e5dae91 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -116,8 +116,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
>                 self._extra_qemu_params = qemu_arch_params.extra_qemu_params
>
>         def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
> -               kconfig = kunit_config.Kconfig()
> -               kconfig.parse_from_string(self._kconfig)
> +               kconfig = kunit_config.parse_from_string(self._kconfig)
>                 base_kunitconfig.merge_in_entries(kconfig)
>
>         def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> @@ -249,8 +248,7 @@ class LinuxSourceTree(object):
>                         if not os.path.exists(kunitconfig_path):
>                                 shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path)
>
> -               self._kconfig = kunit_config.Kconfig()
> -               self._kconfig.read_from_file(kunitconfig_path)
> +               self._kconfig = kunit_config.parse_file(kunitconfig_path)
>
>         def clean(self) -> bool:
>                 try:
> @@ -262,8 +260,7 @@ class LinuxSourceTree(object):
>
>         def validate_config(self, build_dir) -> bool:
>                 kconfig_path = get_kconfig_path(build_dir)
> -               validated_kconfig = kunit_config.Kconfig()
> -               validated_kconfig.read_from_file(kconfig_path)
> +               validated_kconfig = kunit_config.parse_file(kconfig_path)
>                 if not self._kconfig.is_subset_of(validated_kconfig):
>                         invalid = self._kconfig.entries() - validated_kconfig.entries()
>                         message = 'Provided Kconfig is not contained in validated .config. Following fields found in kunitconfig, ' \
> @@ -291,8 +288,7 @@ class LinuxSourceTree(object):
>                 """Creates a new .config if it is not a subset of the .kunitconfig."""
>                 kconfig_path = get_kconfig_path(build_dir)
>                 if os.path.exists(kconfig_path):
> -                       existing_kconfig = kunit_config.Kconfig()
> -                       existing_kconfig.read_from_file(kconfig_path)
> +                       existing_kconfig = kunit_config.parse_file(kconfig_path)
>                         self._ops.make_arch_qemuconfig(self._kconfig)
>                         if not self._kconfig.is_subset_of(existing_kconfig):
>                                 print('Regenerating .config ...')
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 9c4126731457..4ec70e41ec5a 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -50,10 +50,9 @@ class KconfigTest(unittest.TestCase):
>                 self.assertFalse(kconfig1.is_subset_of(kconfig0))
>
>         def test_read_from_file(self):
> -               kconfig = kunit_config.Kconfig()
>                 kconfig_path = test_data_path('test_read_from_file.kconfig')
>
> -               kconfig.read_from_file(kconfig_path)
> +               kconfig = kunit_config.parse_file(kconfig_path)
>
>                 expected_kconfig = kunit_config.Kconfig()
>                 expected_kconfig.add_entry(
> @@ -86,8 +85,7 @@ class KconfigTest(unittest.TestCase):
>
>                 expected_kconfig.write_to_file(kconfig_path)
>
> -               actual_kconfig = kunit_config.Kconfig()
> -               actual_kconfig.read_from_file(kconfig_path)
> +               actual_kconfig = kunit_config.parse_file(kconfig_path)
>
>                 self.assertEqual(actual_kconfig.entries(),
>                                  expected_kconfig.entries())
>
> base-commit: 52a5d80a2225e2d0b2a8f4656b76aead2a443b2a
> --
> 2.34.0.rc0.344.g81b53c2807-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20211106013058.2621799-1-dlatypov%40google.com.