Re: [PATCH] kconfig: make 'imply' obey the direct dependency

[Masahiro Yamada]

On Wed, Feb 19, 2020 at 5:42 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Yamada-san,
>
> On Wed, Feb 19, 2020 at 8:51 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> > The 'imply' statement may create unmet direct dependency when the
> > implied symbol depends on m.
> >
> > [Test Code]
> >
> >   config FOO
> >           tristate "foo"
> >           imply BAZ
> >
> >   config BAZ
> >           tristate "baz"
> >           depends on BAR
> >
> >   config BAR
> >           def_tristate m
> >
> >   config MODULES
> >           def_bool y
> >           option modules
> >
> > If you set FOO=y, BAZ is also promoted to y, which results in the
> > following .config file:
> >
> >   CONFIG_FOO=y
> >   CONFIG_BAZ=y
> >   CONFIG_BAR=m
> >   CONFIG_MODULES=y
> >
> > This ignores the dependency "BAZ depends on BAR".
> >
> > Unlike 'select', what is worse, Kconfig never shows the
> > "WARNING: unmet direct dependencies detected for ..." for this case.
> >
> > Because 'imply' should be weaker than 'depends on', Kconfig should
> > take the direct dependency into account.
> >
> > Describe this case in Documentation/kbuild/kconfig-language.rst for
> > clarification.
> >
> > Commit 237e3ad0f195 ("Kconfig: Introduce the "imply" keyword") says that
> > a symbol implied by y is restricted to y or n, excluding m.
> >
> > As for the combination of FOO=y and BAR=m, the case of BAZ=m is excluded
> > by the 'imply', and BAZ=y is also excluded by 'depends on'. So, only the
> > possible value is BAZ=n.
> >
> > Having said that, this case was probably "We don't care" at that time
> > because Kconfig did not handle 'depends on m' correctly until
> > commit f622f8279581 ("kconfig: warn unmet direct dependency of tristate
> > symbols selected by y") fixed it.
> >
> > Backporting this to 4.19+ will probably be fine. If you care this
> > problem on 4.14.x, you need to backport f622f8279581 as well.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
>
> Thanks a lot! This fixes the build issues in
> https://lore.kernel.org/alsa-devel/CAMuHMdW8SvDgQJyenTtEm4Xn2Ma6PK9pfwKR2_gn60t2AqNWXg@xxxxxxxxxxxxxx/
>
> Tested-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>

Even if this patch fixes the build issues,
the result may not be what you expect.

The very subtle part of 'imply' is that
symbols implied by 'y' cannot be 'm'.

When CONFIG_SND_SOC_ALL_CODECS=y,
the select'ed drivers were previously able to be 'm'.

After ea00d95200d02, drivers that were previously
'm' are now 'n'.

Probably, it shrunk the test-coverage.

I'd recommend to compare the .config file.


-- 
Best Regards
Masahiro Yamada