Re: [PATCHv3 next] kconfig: add dependency warning print about invalid values in verbose mode

[Masahiro Yamada]

"in verbose mode" is stale.

Please rephrase the subject.




On Tue, Jan 2, 2024 at 11:11 AM <sunying@xxxxxxxxxxxxxxxx> wrote:
>
> From: Ying Sun <sunying@xxxxxxxxxxxxxxxx>
>
> Warning in verbose mode about incorrect causes and
>  mismatch dependency of invalid values to help users correct them.

Same here, "verbose mode" does not exist in this patch.




>
> Detailed warning messages are printed only when the environment variable
>  is set like "KCONFIG_WARN_CHANGED_INPUT=1".
> By default, the current behavior is not changed.
>
> Signed-off-by: Siyuan Guo <zy21df106@xxxxxxxxxxx>
> Signed-off-by: Ying Sun <sunying@xxxxxxxxxxxxxxxx>
> ---
> v2 -> v3:
> * Fixed warning message that mess up the ncurses display.
> * Fixed memory leak where str_new() was called but str_free() was not used.
> * Integrated the code to avoid excessive dispersion.
> * Use the environment variable KCONFIG_WARN_CHANGED_INPUT as the switch




This checker prints wrong reports.


I just attached one test case.



[test Kconfig]

config MODULES
       def_bool y
       modules

config FOO
       tristate "foo"
       depends on BAR

config BAR
       tristate "bar"

config BAZ
       tristate "baz"
       select FOO


[test .config]

CONFIG_FOO=m
# CONFIG_BAR is not set
CONFIG_BAZ=y



[test command]


$ KCONFIG_WARN_CHANGED_INPUT=1 make  olddefconfig



[result]


Kconfig:8:warning: 'MODULES' set to y due to option constraints


WARNING: unmet direct dependencies detected for FOO
  Depends on [n]: BAR [=n]
  Selected by [y]:
  - BAZ [=y]
Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies
 Depends on [n]: BAR [=n]



$ cat .config
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86 6.8.0-rc3 Kernel Configuration
#
CONFIG_MODULES=y
CONFIG_FOO=y
# CONFIG_BAR is not set
CONFIG_BAZ=y







The first report

  Kconfig:8:warning: 'MODULES' set to y due to option constraints

should not be printed.

CONFIG options without prompt can be omitted.




The second report

  Kconfig:12:warning: 'FOO' set to n for it unmet direct dependencies

is completely wrong.



CONFIG_FOO was changed to 'y' due to the select.





One more thing,

Add
export KCONFIG_WARN_CHANGED_INPUT=1

to scripts/kconfig/Makefile




> v1 -> v2:
> * Reduced the number of code lines by refactoring and simplifying the logic.
> * Changed the print "ERROR" to "WARNING".
> * Focused on handling dependency errors: dir_dep and rev_dep, and range error.
>   - A downgrade from 'y' to 'm' has be warned.
>   - A new CONFIG option should not be warned.
>   - Overwriting caused by default value is not an error and is no longer printed.
> * Fixed style issues.
> ---
> ---
>  scripts/kconfig/confdata.c | 76 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index f1197e672431..352774928558 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -195,6 +195,52 @@ static void conf_message(const char *fmt, ...)
>         va_end(ap);
>  }
>
> +static void conf_warn_unmet_rev_dep(struct symbol *sym)
> +{
> +       struct gstr gs = str_new();
> +       char value = 'n';
> +
> +       switch (sym->curr.tri) {
> +       case no:
> +               value = 'n';
> +               break;
> +       case mod:
> +               sym_calc_value(modules_sym);
> +               value = (modules_sym->curr.tri == no) ? 'n' : 'm';
> +               break;
> +       case yes:
> +               value = 'y';
> +       }
> +
> +       str_printf(&gs,
> +               "'%s' set to %c for it is selected\n",
> +               sym->name, value);
> +       expr_gstr_print_revdep(sym->rev_dep.expr, &gs, yes,
> +                               " Selected by [y]:\n");
> +       expr_gstr_print_revdep(sym->rev_dep.expr, &gs, mod,
> +                               " Selected by [m]:\n");
> +
> +       conf_warning("%s", str_get(&gs));
> +       str_free(&gs);
> +}
> +
> +static void conf_warn_dep_error(struct symbol *sym)
> +{
> +       struct gstr gs = str_new();
> +
> +       str_printf(&gs,
> +               "'%s' set to n for it unmet direct dependencies\n",
> +               sym->name);
> +
> +       str_printf(&gs,
> +               " Depends on [%c]: ",
> +               sym->dir_dep.tri == mod ? 'm' : 'n');
> +       expr_gstr_print(sym->dir_dep.expr, &gs);
> +
> +       conf_warning("%s\n", str_get(&gs));
> +       str_free(&gs);
> +}
> +
>  const char *conf_get_configname(void)
>  {
>         char *name = getenv("KCONFIG_CONFIG");
> @@ -568,6 +614,36 @@ int conf_read(const char *name)
>                         continue;
>                 conf_unsaved++;
>                 /* maybe print value in verbose mode... */
> +               if (getenv("KCONFIG_WARN_CHANGED_INPUT") && (sym->prop)) {
> +                       conf_filename = sym->prop->file->name;
> +                       conf_lineno = sym->prop->menu->lineno;
> +
> +                       switch (sym->type) {
> +                       case S_BOOLEAN:
> +                       case S_TRISTATE:
> +                               if (sym->def[S_DEF_USER].tri != sym_get_tristate_value(sym)) {
> +                                       if (sym->dir_dep.tri < sym->def[S_DEF_USER].tri)
> +                                               conf_warn_dep_error(sym);
> +                                       else if (sym->rev_dep.tri > sym->def[S_DEF_USER].tri)
> +                                               conf_warn_unmet_rev_dep(sym);
> +                                       else
> +                                               conf_warning("'%s' set to %s due to option constraints\n",
> +                                                       sym->name, sym_get_string_value(sym));
> +                               }
> +                               break;
> +                       case S_INT:
> +                       case S_HEX:
> +                       case S_STRING:
> +                               if (sym->dir_dep.tri == no && sym_has_value(sym))
> +                                       conf_warn_dep_error(sym);
> +                               else
> +                                       conf_warning("'%s' set to %s due to option constraints\n",
> +                                                       sym->name, sym_get_string_value(sym));
> +                               break;
> +                       default:
> +                               break;
> +                       }
> +               }
>         }
>
>         for_all_symbols(i, sym) {
> --
> 2.43.0
>
>


--
Best Regards


Masahiro Yamada