Inconsistency in dependency calculations

Commit Message

Arnaud Lacombe May 22, 2011, 4:23 a.m. UTC

Hi,

On Sat, May 21, 2011 at 12:55 PM, Yann E. MORIN
<[e-mail]> wrote:
> Hello All!
>
> Here I am again, with an inconstistency in the way forward dependencies are
> computed. ;-)
>
> Take this simple test-case for example:
>
>    ---8<--- Config.in ---8<--
>    config F
>        bool "F"
>
>    choice AB
>        bool "AB"
>    config A
>        bool "A"
>    config B
>        bool "B"
>    config C
>        bool "C"
>    endchoice
>
>    if A
>    config D
>        bool
>        default y if F
>        select E
>    config E
>        bool "E"
>    endif # A
>
>    if B
>    config D
>        bool
>        default y if F
>        select E
>    config E
>        bool "E"
>    endif # B
>    ---8<--- END ---8<---
>
> Then run:
>  rm -f .config .config.old
>  mconf Config.in
>
>    [ ] F (NEW)
>        AB (A)  --->
>    [ ] E (NEW)
>
> Then select F:
>
>    [*] F      D) selects E which has unmet direct dependencies (B)
>        AB (A)  --->
>    -*- E
>
> Notice the text following F. It is *not* a copy-paste error, I do really see
> this text.
>
expected.

> Now exit and save. Run mconf again, and exit staright away. Notice this
> error mesage on the console:
>    warning: (D) selects E which has unmet direct dependencies (B)
>
> Now, run mconf again, select B in the choice (keep F selected). You get the
> same warning on the console (expected, as it is printed before bringing up
> the dialog menu, and we get to see it only when exiting, not when loading).
>
> Now re-run mconf yet again and exit straight away. There is no warning this
> time.
>
> What I would expect for consistency: the warning is printed both when A or
> B is selected, warning about direct dep B when A is selected, and conversely
> when A is selected:
> A selected:
>    warning: (D) selects E which has unmet direct dependencies (B)
> B selected:
>    warning: (D) selects E which has unmet direct dependencies (A)
>
> What I would expect in an ideal world: no warning is printed at all.
>
as a rule, I would expect the backend never to output anything unless
it has been instructed to do so by the frontend, it has obviously not
been enforced in the past, and is likely not to trivial to fix...

I guess that mconf() can be told to redraw the complete screen, but
that's hackish...

> Behaves the same with kconfig in 2.6.39 and linux-next 20110520.
>
> Of course, this example is only here to pin-point the mis-behavior. Fact is,
> the real life example happens with a generated Config.in, so it is not possible
> to turn the conditional into "if A || B".
>
yeah.. the real problem is that you are using kconfig in a way which
has not been thought about. The problem you got is because the symbol
direct dependency is inherited by its last parent, in that case, B.
But you should not have the warning at the first place, in the B
branch, all E's dependency are met. This should fix your problem:

                if (sym && sym_is_choice(sym) &&

[same warning, gmail and long line do not play well together...]

 - Arnaud

> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | [phone] | Software  Designer | \ / CAMPAIGN     |  ___               |
> | [phone] `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yann E. MORIN May 22, 2011, 12:04 p.m. UTC | #1

Arnaud, All,

On Sunday 22 May 2011 06:23:32 Arnaud Lacombe wrote:
[--SNIP--]
> > Now exit and save. Run mconf again, and exit staright away. Notice this
> > error mesage on the console:
> >    warning: (D) selects E which has unmet direct dependencies (B)
[--SNIP--]
> > What I would expect in an ideal world: no warning is printed at all.
[--SNIP--]
> yeah.. the real problem is that you are using kconfig in a way which
> has not been thought about.

Yes, I know. But is this config snippet valid wrt kconfig syntax and
grammar? If so, there is a bug in the dependency tracking; if not,
then the doc should be updated to reflect the limitation.

But kconfig is now more and more used out of the kernel land, and I
believe it is very good. This means also that non-kernel users will
use it in ways not envisioned when kconfig was first written, such as
I do, and will uncover corner cases that exhibit the current limitations.

> The problem you got is because the symbol 
> direct dependency is inherited by its last parent, in that case, B.
> But you should not have the warning at the first place, in the B
> branch, all E's dependency are met. This should fix your problem:
> 
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 5fdf10d..77f913c 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -350,7 +350,7 @@ void menu_finalize(struct menu *parent)
>                         last_menu->next = NULL;
>                 }
> 
> -               sym->dir_dep.expr = parent->dep;
> +               sym->dir_dep.expr = expr_alloc_or(sym->dir_dep.expr,
> parent->dep);;
>         }
>         for (menu = parent->list; menu; menu = menu->next) {
>                 if (sym && sym_is_choice(sym) &&

Indeed! Thank you very much!

Do you plan to push this and your fix on choice visibility before the
end of the merge window?

Thank you again for this quick and good explanations! :-)

Regards,
Yann E. MORIN.

Patch

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 5fdf10d..77f913c 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -350,7 +350,7 @@  void menu_finalize(struct menu *parent)
                        last_menu->next = NULL;
                }

-               sym->dir_dep.expr = parent->dep;
+               sym->dir_dep.expr = expr_alloc_or(sym->dir_dep.expr,
parent->dep);;
        }
        for (menu = parent->list; menu; menu = menu->next) {