| Message ID | ghvc0cy00x.fsf@lena.gouders.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Dirk Gouders | 2013-11-01 00:39:46 [+0100]: >This patch sets choice_values' visibility that depend on symbols set >to 'm' to 'n' if the corresponding choice is set to 'y'. This makes >them disappear from the choice list and will also cause the >choice_values' value set to 'n' in sym_calc_value() and as a result >they are written as "not set" to the resulting .config file. > >Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Still solves the issue for me :) >Signed-off-by: Dirk Gouders <dirk@gouders.net> Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sebastian, All, On 2013-11-04 18:27 +0100, Sebastian Andrzej Siewior spake thusly: > * Dirk Gouders | 2013-11-01 00:39:46 [+0100]: > > >This patch sets choice_values' visibility that depend on symbols set > >to 'm' to 'n' if the corresponding choice is set to 'y'. This makes > >them disappear from the choice list and will also cause the > >choice_values' value set to 'n' in sym_calc_value() and as a result > >they are written as "not set" to the resulting .config file. > > > >Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Still solves the issue for me :) Should I consider this as: Tested-by: <you> ? Regards, Yann E. MORIN.
On 11/04/2013 09:46 PM, Yann E. MORIN wrote: > Sebastian, All, Hi Yann, >> Still solves the issue for me :) > > Should I consider this as: > Tested-by: <you> > ? Yes, please. Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Regards, > Yann E. MORIN. > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dirk, All, On 2013-11-01 00:39 +0100, Dirk Gouders spake thusly: > If choices consist of choice_values that depend on symbols set to 'm', > those choice_values are not set to 'n' if the choice is changed from > 'm' to 'y' (in which case only one active choice_value is allowed). > Those values are also written to the config file causing modules to be > built when they should not. > > The following config can be used to reproduce and examine the problem: > > config modules > boolean modules > default y > option modules > > config dependency > tristate "Dependency" > default m > > choice > prompt "Tristate Choice" > default choice0 > > config choice0 > tristate "Choice 0" > > config choice1 > tristate "Choice 1" > depends on dependency > > endchoice > > This patch sets choice_values' visibility that depend on symbols set > to 'm' to 'n' if the corresponding choice is set to 'y'. This makes > them disappear from the choice list and will also cause the > choice_values' value set to 'n' in sym_calc_value() and as a result > they are written as "not set" to the resulting .config file. It seems I'm missing something here. I just copy-pasted your example (test.in. below) and used it with current master (cset #be408cd) without your patch, and then ran: $ git clean -dX # clean the tree $ make menuconfig # Generate the frontend --> exit without saving $ ./scripts/kconfig/mconf test.in --> change the choice to 'y' --> do not change anything else --> exit and save $ cat .config CONFIG_modules=y CONFIG_dependency=m CONFIG_c0=y # CONFIG_c1 is not set (.config header elided on purpose) This looks like the expected output to me. So I did further tests (still without your patch): $ git clean -dX # clean the tree $ make menuconfig # Generate the frontend --> exit without saving $ ./scripts/kconfig/mconf test.in --> change nothing --> exit and save $ cat .config CONFIG_modules=y CONFIG_dependency=m # CONFIG_c0 is not set # CONFIG_c1 is not set $ ./scripts/kconfig/mconf test.in --> change the choice to 'y' --> do not change anything else --> exit and save $ cat .config CONFIG_modules=y CONFIG_dependency=m CONFIG_c0=y # CONFIG_c1 is not set Still the expected output, as far as I can see. I can observe the exact same result with your patch applied. Ditto with kbuild/for-next from Michal's tree, with or without your patch. So while I understand and can reproduce the original issue, and this patch indeed solves this original issue, the test-case above does not seem to completely illustrate the issue. Are you sure this test-case exhibits the problem for you? Anyway, I'm taking that in my tree locally, but that won't be part of for-next, since I'd like that we: - either find a real reduced test-case, - or just repeat the description from the original bug report Needless to say that I'd prefer the former over the latter. Then I'll queue it as a post-rc1 fix. Regards, Yann E. MORIN. > Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Dirk Gouders <dirk@gouders.net> > --- > scripts/kconfig/symbol.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > index 22a3c40..32bbaa3 100644 > --- a/scripts/kconfig/symbol.c > +++ b/scripts/kconfig/symbol.c > @@ -188,12 +188,23 @@ static void sym_validate_range(struct symbol *sym) > static void sym_calc_visibility(struct symbol *sym) > { > struct property *prop; > + struct symbol *choice_sym = NULL; > tristate tri; > > /* any prompt visible? */ > tri = no; > + > + if (sym_is_choice_value(sym)) > + choice_sym = prop_get_symbol(sym_get_choice_prop(sym)); > + > for_all_prompts(sym, prop) { > prop->visible.tri = expr_calc_value(prop->visible.expr); > + /* > + * choice_values with visibility 'mod' are not visible if the > + * corresponding choice's value is 'yes'. > + */ > + if (prop->visible.tri == mod && (choice_sym && choice_sym->curr.tri == yes)) > + prop->visible.tri = no; > tri = EXPR_OR(tri, prop->visible.tri); > } > if (tri == mod && (sym->type != S_TRISTATE || modules_val == no)) > -- > 1.8.4 >
"Yann E. MORIN" <yann.morin.1998@free.fr> writes: > Dirk, All, > > On 2013-11-01 00:39 +0100, Dirk Gouders spake thusly: >> If choices consist of choice_values that depend on symbols set to 'm', >> those choice_values are not set to 'n' if the choice is changed from >> 'm' to 'y' (in which case only one active choice_value is allowed). >> Those values are also written to the config file causing modules to be >> built when they should not. >> >> The following config can be used to reproduce and examine the problem: >> >> config modules >> boolean modules >> default y >> option modules >> >> config dependency >> tristate "Dependency" >> default m >> >> choice >> prompt "Tristate Choice" >> default choice0 >> >> config choice0 >> tristate "Choice 0" >> >> config choice1 >> tristate "Choice 1" >> depends on dependency >> >> endchoice >> >> This patch sets choice_values' visibility that depend on symbols set >> to 'm' to 'n' if the corresponding choice is set to 'y'. This makes >> them disappear from the choice list and will also cause the >> choice_values' value set to 'n' in sym_calc_value() and as a result >> they are written as "not set" to the resulting .config file. > > It seems I'm missing something here. > > I just copy-pasted your example (test.in. below) and used it with > current master (cset #be408cd) without your patch, and then ran: > $ git clean -dX # clean the tree > $ make menuconfig # Generate the frontend > --> exit without saving > $ ./scripts/kconfig/mconf test.in > --> change the choice to 'y' > --> do not change anything else > --> exit and save > > $ cat .config > CONFIG_modules=y > CONFIG_dependency=m > CONFIG_c0=y > # CONFIG_c1 is not set > > (.config header elided on purpose) > This looks like the expected output to me. > > So I did further tests (still without your patch): > $ git clean -dX # clean the tree > $ make menuconfig # Generate the frontend > --> exit without saving > $ ./scripts/kconfig/mconf test.in > --> change nothing > --> exit and save > > $ cat .config > CONFIG_modules=y > CONFIG_dependency=m > # CONFIG_c0 is not set > # CONFIG_c1 is not set This, by the way, is the other problem I mentioned earlier. There is a default value defined for "Tristate Choice" and the way I understand the kconfig language, CONFIG_c0 should be 'm' here. But that is another issue it is just a nice example for what I tried to describe earlier. > $ ./scripts/kconfig/mconf test.in > --> change the choice to 'y' > --> do not change anything else > --> exit and save > > $ cat .config > CONFIG_modules=y > CONFIG_dependency=m > CONFIG_c0=y > # CONFIG_c1 is not set > > Still the expected output, as far as I can see. > > I can observe the exact same result with your patch applied. Ditto with > kbuild/for-next from Michal's tree, with or without your patch. > > So while I understand and can reproduce the original issue, and this > patch indeed solves this original issue, the test-case above does not > seem to completely illustrate the issue. > > Are you sure this test-case exhibits the problem for you? Yes, but obviously, I did not describe it very clearly. The steps to reproduce the problem are: $ ./scripts/kconfig/mconf test.in --> change c0 and c1 to 'm' # This is the missing part! --> change the choice to 'y' --> do not change anything else --> exit and save I spontaneously planned to answer with a modified config file with default values 'm' specified for 'c0' and 'c1' (complete file below) but I noticed that my latest patch does not help in that case. The first patch that modifies sym_calc_value() would handle it nicely but the latter one that modifies sym_calc_visibility() does not. The combination also does not work, because sym_calc_visibility() influences sym_calc_value(). So, I have to say that I am no longer really satisfied with the patch. It fixes the reported problem but I think it should fix related obvious problems as well (see config below). I'd prefer I take some more time and try to find a more sensible fix. Thanks for your review and testing, Yann. Dirk PS: Sebastian, I also want to say thank you to you for the testing so far! - Sample Kconfig ------------------------------------------------------- config modules boolean modules default y option modules config dependency tristate "Dependency" default m choice tristate "Tristate Choice" default choice0 config choice0 tristate "Choice 0" default m config choice1 tristate "Choice 1" depends on dependency default m endchoice ------------------------------------------------------------------------ > Anyway, I'm taking that in my tree locally, but that won't be part of > for-next, since I'd like that we: > - either find a real reduced test-case, > - or just repeat the description from the original bug report > > Needless to say that I'd prefer the former over the latter. Then I'll > queue it as a post-rc1 fix. > > Regards, > Yann E. MORIN. > >> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> Signed-off-by: Dirk Gouders <dirk@gouders.net> >> --- >> scripts/kconfig/symbol.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c >> index 22a3c40..32bbaa3 100644 >> --- a/scripts/kconfig/symbol.c >> +++ b/scripts/kconfig/symbol.c >> @@ -188,12 +188,23 @@ static void sym_validate_range(struct symbol *sym) >> static void sym_calc_visibility(struct symbol *sym) >> { >> struct property *prop; >> + struct symbol *choice_sym = NULL; >> tristate tri; >> >> /* any prompt visible? */ >> tri = no; >> + >> + if (sym_is_choice_value(sym)) >> + choice_sym = prop_get_symbol(sym_get_choice_prop(sym)); >> + >> for_all_prompts(sym, prop) { >> prop->visible.tri = expr_calc_value(prop->visible.expr); >> + /* >> + * choice_values with visibility 'mod' are not visible if the >> + * corresponding choice's value is 'yes'. >> + */ >> + if (prop->visible.tri == mod && (choice_sym && choice_sym->curr.tri == yes)) >> + prop->visible.tri = no; >> tri = EXPR_OR(tri, prop->visible.tri); >> } >> if (tri == mod && (sym->type != S_TRISTATE || modules_val == no)) >> -- >> 1.8.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dirk, All, On 2013-11-06 15:43 +0100, Dirk Gouders spake thusly: > "Yann E. MORIN" <yann.morin.1998@free.fr> writes: [--SNIP--] > > It seems I'm missing something here. [--SNIP--] > Yes, but obviously, I did not describe it very clearly. The steps to > reproduce the problem are: > > $ ./scripts/kconfig/mconf test.in > --> change c0 and c1 to 'm' # This is the missing part! Aha! Gotcha. Thanks. > I spontaneously planned to answer with a modified config file with > default values 'm' specified for 'c0' and 'c1' (complete file below) but > I noticed that my latest patch does not help in that case. The first > patch that modifies sym_calc_value() would handle it nicely but the > latter one that modifies sym_calc_visibility() does not. The > combination also does not work, because sym_calc_visibility() influences > sym_calc_value(). > > So, I have to say that I am no longer really satisfied with the patch. > It fixes the reported problem but I think it should fix related > obvious problems as well (see config below). I'd prefer I take some > more time and try to find a more sensible fix. Please, one patch to fix one bug. It does not matter if you need to touch the same part of the code, but please keep fixes for different bugs, separate (unless of course, the bugs are just different manifestations of the same deficiency in the code). Regards, Yann E. MORIN.
"Yann E. MORIN" <yann.morin.1998@free.fr> writes: > Dirk, All, > > On 2013-11-06 15:43 +0100, Dirk Gouders spake thusly: >> "Yann E. MORIN" <yann.morin.1998@free.fr> writes: > [--SNIP--] >> > It seems I'm missing something here. > [--SNIP--] >> Yes, but obviously, I did not describe it very clearly. The steps to >> reproduce the problem are: >> >> $ ./scripts/kconfig/mconf test.in >> --> change c0 and c1 to 'm' # This is the missing part! > > Aha! Gotcha. Thanks. > >> I spontaneously planned to answer with a modified config file with >> default values 'm' specified for 'c0' and 'c1' (complete file below) but >> I noticed that my latest patch does not help in that case. The first >> patch that modifies sym_calc_value() would handle it nicely but the >> latter one that modifies sym_calc_visibility() does not. The >> combination also does not work, because sym_calc_visibility() influences >> sym_calc_value(). >> >> So, I have to say that I am no longer really satisfied with the patch. >> It fixes the reported problem but I think it should fix related >> obvious problems as well (see config below). I'd prefer I take some >> more time and try to find a more sensible fix. > > Please, one patch to fix one bug. > > It does not matter if you need to touch the same part of the code, but > please keep fixes for different bugs, separate (unless of course, the > bugs are just different manifestations of the same deficiency in the > code). I understand. I will send a v4 with a clearer description of the steps needed to trigger the problem and also the added Tested-by: line, in case you see a need for it. The other two problems I mentioned are concerning default values of tristate choices and I am quite confident that those fixes will touch other parts of the code. Dirk -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dirk Gouders <dirk@gouders.net> writes: [SNIP] >> Are you sure this test-case exhibits the problem for you? > > Yes, but obviously, I did not describe it very clearly. The steps to > reproduce the problem are: > > $ ./scripts/kconfig/mconf test.in > --> change c0 and c1 to 'm' # This is the missing part! > --> change the choice to 'y' > --> do not change anything else > --> exit and save > > I spontaneously planned to answer with a modified config file with > default values 'm' specified for 'c0' and 'c1' (complete file below) but > I noticed that my latest patch does not help in that case. The first > patch that modifies sym_calc_value() would handle it nicely but the > latter one that modifies sym_calc_visibility() does not. The > combination also does not work, because sym_calc_visibility() influences > sym_calc_value(). [SNIP] Hi Yann, all, seems that I was a bit misleaded, here. While looking at how to possibly fix what I described, I realized that default values for choice values are not supported and therfore there is no issue: choices_kconfig:17:warning: defaults for choice values not supported choices_kconfig:22:warning: defaults for choice values not supported I noticed these warnings only accidently, when I was using an assert() that caused an abort and prevented the output to stderr being hidden by the ncurses output. Perhaps I should redirect stderr to a file and inspect it, in the future... So, my concerns with my own patch were unsubstantiated. Dirk > - Sample Kconfig ------------------------------------------------------- > > config modules > boolean modules > default y > option modules > > config dependency > tristate "Dependency" > default m > > choice > tristate "Tristate Choice" > default choice0 > > config choice0 > tristate "Choice 0" > default m > > config choice1 > tristate "Choice 1" > depends on dependency > default m > > endchoice > > ------------------------------------------------------------------------ -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index 22a3c40..32bbaa3 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -188,12 +188,23 @@ static void sym_validate_range(struct symbol *sym) static void sym_calc_visibility(struct symbol *sym) { struct property *prop; + struct symbol *choice_sym = NULL; tristate tri; /* any prompt visible? */ tri = no; + + if (sym_is_choice_value(sym)) + choice_sym = prop_get_symbol(sym_get_choice_prop(sym)); + for_all_prompts(sym, prop) { prop->visible.tri = expr_calc_value(prop->visible.expr); + /* + * choice_values with visibility 'mod' are not visible if the + * corresponding choice's value is 'yes'. + */ + if (prop->visible.tri == mod && (choice_sym && choice_sym->curr.tri == yes)) + prop->visible.tri = no; tri = EXPR_OR(tri, prop->visible.tri); } if (tri == mod && (sym->type != S_TRISTATE || modules_val == no))
If choices consist of choice_values that depend on symbols set to 'm', those choice_values are not set to 'n' if the choice is changed from 'm' to 'y' (in which case only one active choice_value is allowed). Those values are also written to the config file causing modules to be built when they should not. The following config can be used to reproduce and examine the problem: config modules boolean modules default y option modules config dependency tristate "Dependency" default m choice prompt "Tristate Choice" default choice0 config choice0 tristate "Choice 0" config choice1 tristate "Choice 1" depends on dependency endchoice This patch sets choice_values' visibility that depend on symbols set to 'm' to 'n' if the corresponding choice is set to 'y'. This makes them disappear from the choice list and will also cause the choice_values' value set to 'n' in sym_calc_value() and as a result they are written as "not set" to the resulting .config file. Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Dirk Gouders <dirk@gouders.net> --- scripts/kconfig/symbol.c | 11 +++++++++++ 1 file changed, 11 insertions(+)