Re: [PATCH 02/14] kconfig: do not write choice values when their dependency becomes n

[Masahiro Yamada]

2018-02-08 7:55 GMT+09:00 Ulf Magnusson <ulfalizer@xxxxxxxxx>:
> On Tue, Feb 06, 2018 at 09:34:42AM +0900, Masahiro Yamada wrote:
>> "# CONFIG_... is not set" for choice values are wrongly written into
>> the .config file if they are once visible, then become invisible later.
>>
>>   Test case
>>   ---------
>>
>> ---------------------------(Kconfig)----------------------------
>> config A
>>       bool "A"
>>
>> choice
>>       prompt "Choice ?"
>>       depends on A
>>
>> config CHOICE_B
>>       bool "Choice B"
>>
>> config CHOICE_C
>>       bool "Choice C"
>>
>> endchoice
>> ----------------------------------------------------------------
>>
>> ---------------------------(.config)----------------------------
>> CONFIG_A=y
>> ----------------------------------------------------------------
>>
>> With the Kconfig and .config above,
>>
>>   $ make config
>>   scripts/kconfig/conf  --oldaskconfig Kconfig
>>   *
>>   * Linux Kernel Configuration
>>   *
>>   A (A) [Y/n] n
>>   #
>>   # configuration written to .config
>>   #
>>   $ cat .config
>>   #
>>   # Automatically generated file; DO NOT EDIT.
>>   # Linux Kernel Configuration
>>   #
>>   # CONFIG_A is not set
>>   # CONFIG_CHOICE_B is not set
>>   # CONFIG_CHOICE_C is not set
>>
>> Here,
>>
>>   # CONFIG_CHOICE_B is not set
>>   # CONFIG_CHOICE_C is not set
>>
>> should not be written into the .config file because their dependency
>> "depends on A" is unmet.
>>
>> Currently, there is no code that clears SYMBOL_WRITE of choice values.
>>
>> Clear SYMBOL_WRITE for all symbols in sym_calc_value(), then set it
>> again after calculating visibility.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> ---
>
>>  scripts/kconfig/symbol.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index c9123ed..5d6f6b1 100644
>> --- a/scripts/kconfig/symbol.c
>> +++ b/scripts/kconfig/symbol.c
>> @@ -371,8 +371,7 @@ void sym_calc_value(struct symbol *sym)
>>               sym->curr.tri = no;
>>               return;
>>       }
>> -     if (!sym_is_choice_value(sym))
>> -             sym->flags &= ~SYMBOL_WRITE;
>> +     sym->flags &= ~SYMBOL_WRITE;
>>
>>       sym_calc_visibility(sym);
>>
>> @@ -385,6 +384,7 @@ void sym_calc_value(struct symbol *sym)
>>               if (sym_is_choice_value(sym) && sym->visible == yes) {
>>                       prop = sym_get_choice_prop(sym);
>>                       newval.tri = (prop_get_symbol(prop)->curr.val == sym) ? yes : no;
>> +                     sym->flags |= SYMBOL_WRITE;
>>               } else {
>>                       if (sym->visible != no) {
>>                               /* if the symbol is visible use the user value
>> --
>> 2.7.4
>>
>
> Reviewed-by: Ulf Magnusson <ulfalizer@xxxxxxxxx>
>
> There's a possible simplification here:
>
> All defined symbols, regardless of type, and regardless of whether
> they're choice value symbols or not, always get written out if they have
> non-n visibility. Therefore, the sym->visible != no check for
> SYMBOL_WRITE can be moved to before the symbol type check, which gets
> rid of two SYMBOL_WRITE assignments and makes it clear that the logic is
> the same for all paths.
>
> This is safe for symbols defined without a type (S_UNKNOWN) too, because
> conf_write() skips those (plus they already generate a warning).
>
> This matches how I do it in the tri_value() function in Kconfiglib:
> https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2574.
> SYMBOL_WRITE corresponds to _write_to_conf.
>
> I've included a patch below. I tested it with the Kconfiglib test suite,
> which verifies that the C implementation still generates the same
> .config file for all defconfig files as well as for
> all{no,yes,def}config, for all ARCHes.
>
> (The Kconfiglib test suite runs scripts/kconfig/conf and compares its
> output against it, which means it doubles as a regression test for the C
> tools.)

Thank you for this.  This is simpler, and please let me take it.

I confirmed the same results were produced.




-- 
Best Regards
Masahiro Yamada