Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220520224200.3764027-1-dlatypov@google.com> <CABVgOSkNpaY1HsygqkWC8LVO+XTqGwNxFNgne-ajoTiPqCnrkg@mail.gmail.com>
In-Reply-To: <CABVgOSkNpaY1HsygqkWC8LVO+XTqGwNxFNgne-ajoTiPqCnrkg@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Fri, 24 Jun 2022 13:24:21 -0700
Message-ID: <CAGS_qxqQiWDRwOJcU+1X31SgdKGFMLT8tA7EJXN9JUmKOhYfxw@mail.gmail.com>
Subject: Re: [PATCH v2] kunit: tool: refactor internal kconfig handling, allow overriding
To:     David Gow <davidgow@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 Fri, Jun 24, 2022 at 12:55 AM David Gow <davidgow@google.com> wrote:
> >
> > +       def set_diff(self, other: 'Kconfig') -> Set[KconfigEntry]:
> > +               return set(self._as_entries()) - set(other._as_entries())
> > +
>
> It took me a couple of goes to realise that this was looking at the
> difference between sets, not trying to set the difference. Maybe
> different_entries() or something like that'd be clearer, but I can't
> say it bothers me enough to be worth a new version.

Wdyt about `set_difference()`?
Or maybe adding a verb: `get_set_diff()`, `compute_set_diff()`?

But we do want to make it clear it has set (asymmetric) difference
semantics, see below.

>
> Then again (as noted below), the direct set difference isn't exactly
> what we want, it's more the equivalent of is_subset_of(). The
> follow-up repeated-kunitconfig patch adds differing_options(), which
> is closer to what we'd want, I think:
> https://lore.kernel.org/linux-kselftest/20220624001247.3255978-1-dlatypov@google.com/

differing_options() does not have the right semantics.
For this, we do explicitly want a set difference.

Context: This is the func used to print out these warnings:
$ .../kunit.py run --kconfig_add=CONFIG_PCI=y
...
Missing: CONFIG_PCI=y

That comes from
  invalid = self._kconfig.set_diff(validated_kconfig)
Using differing_options() to get our version of the configs:
  invalid = (want for want, got in
self._kconfig.differing_options(validated_kconfig))
we instead get an empty list.

The problem is that differing_options() only shows config options that
are explicitly to different values.

There's probably a way I can name these functions better to make the
difference more clear.
Or perhaps we should move set_diff() out of this class and have
kunit_kernel.py itself due the computation.

E.g.
  // make as_entries() "public", s/invalid/missing
  missing = set(self._kconfig.as_entries()) - set(validated_config.as_entries())

Thoughts?

Daniel