Re: [PATCH v2] kunit: tool: refactor internal kconfig handling, allow overriding

[David Gow <davidgow@google.com>]

On Sat, Jun 25, 2022 at 4:24 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> 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.
>

I like adding a verb, personally.

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

Ah, yep: my mistake. I was mixing up the (!b && value=n) check in
is_subset_of with the (b && value != b) in differing options. Whoops.

But, thinking about it, the special handling of "=n" in set A not
needing an equivalent in set B should fall out from the set difference
anyway. So no problem here at all.

> There's probably a way I can name these functions better to make the
> difference more clear.

How about "conflicting_options" versus "missing_options"?

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

Fair enough point:

>
> Daniel