| Message ID | 20231122034753.1446513-1-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kconfig: WERROR unmet symbol dependency | expand |
On (23/11/22 12:47), Sergey Senozhatsky wrote: > When KCONFIG_WERROR env variable is set treat unmet direct > symbol dependency as a terminal condition (error). > > Suggested-by: Stefan Reinauer <reinauer@google.com> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> Gentle ping.
On Tue, Nov 28, 2023 at 2:34 PM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (23/11/22 12:47), Sergey Senozhatsky wrote: > > When KCONFIG_WERROR env variable is set treat unmet direct > > symbol dependency as a terminal condition (error). > > > > Suggested-by: Stefan Reinauer <reinauer@google.com> > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > Gentle ping. I believe you know this patch is too cheesy to be accepted. KCONFIG_WERROR is meant to turn all warnings to errors. I do not see getenv("KCONFIG_WERROR") sprinkled everywhere in Kconfig. One more thing, you cannot directly exit(1) from sym_calc_value(). Curses programs (menuconfig / nconfig), must call endwin() before exiting. Otherwise, the terminal will not come back to the canonical mode. You do not need to fix it, though. -- Best Regards Masahiro Yamada
On (23/11/28 23:19), Masahiro Yamada wrote: [..] > KCONFIG_WERROR is meant to turn all warnings > to errors. > I do not see getenv("KCONFIG_WERROR") > sprinkled everywhere in Kconfig. > One more thing, you cannot directly exit(1) > from sym_calc_value(). We do exit(1) for KCONFIG_WARN_UNKNOWN_SYMBOLS in conf_read(). I can introduce two new helpers that will tell if confdata.c and symbol.c triggered any warnings and if KCONFIG_WERROR is set. And then different code paths can call them and handle exit gracefully, depending on the context (ncurses, menu, etc.). Something like this --- scripts/kconfig/conf.c | 6 ++++++ scripts/kconfig/confdata.c | 13 ++++++++----- scripts/kconfig/lkc_proto.h | 2 ++ scripts/kconfig/symbol.c | 9 +++++++++ 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 33d19e419908..662a5e7c37c2 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -827,6 +827,9 @@ int main(int ac, char **av) break; } + if (conf_errors()) + exit(1); + if (sync_kconfig) { name = getenv("KCONFIG_NOSILENTUPDATE"); if (name && *name) { @@ -890,6 +893,9 @@ int main(int ac, char **av) break; } + if (sym_dep_errors()) + exit(1); + if (input_mode == savedefconfig) { if (conf_write_defconfig(defconfig_file)) { fprintf(stderr, "n*** Error while saving defconfig to: %s\n\n", diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index bd14aae1db58..9c8bee10c952 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -155,6 +155,13 @@ static void conf_message(const char *fmt, ...) static const char *conf_filename; static int conf_lineno, conf_warnings; +bool conf_errors(void) +{ + if (conf_warnings) + return getenv("KCONFIG_WERROR"); + return false; +} + static void conf_warning(const char *fmt, ...) { va_list ap; @@ -365,10 +372,9 @@ int conf_read_simple(const char *name, int def) char *p, *val; struct symbol *sym; int i, def_flags; - const char *warn_unknown, *werror, *sym_name; + const char *warn_unknown, *sym_name; warn_unknown = getenv("KCONFIG_WARN_UNKNOWN_SYMBOLS"); - werror = getenv("KCONFIG_WERROR"); if (name) { in = zconf_fopen(name); } else { @@ -526,9 +532,6 @@ int conf_read_simple(const char *name, int def) free(line); fclose(in); - if (conf_warnings && werror) - exit(1); - return 0; } diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h index edd1e617b25c..e4931bde7ca7 100644 --- a/scripts/kconfig/lkc_proto.h +++ b/scripts/kconfig/lkc_proto.h @@ -12,6 +12,7 @@ void conf_set_changed(bool val); bool conf_get_changed(void); void conf_set_changed_callback(void (*fn)(void)); void conf_set_message_callback(void (*fn)(const char *s)); +bool conf_errors(void); /* symbol.c */ extern struct symbol * symbol_hash[SYMBOL_HASHSIZE]; @@ -22,6 +23,7 @@ void print_symbol_for_listconfig(struct symbol *sym); struct symbol ** sym_re_search(const char *pattern); const char * sym_type_name(enum symbol_type type); void sym_calc_value(struct symbol *sym); +bool sym_dep_errors(void); enum symbol_type sym_get_type(struct symbol *sym); bool sym_tristate_within_range(struct symbol *sym,tristate tri); bool sym_set_tristate_value(struct symbol *sym,tristate tri); diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index a76925b46ce6..4eb352bba710 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -37,6 +37,7 @@ static struct symbol symbol_empty = { struct symbol *modules_sym; static tristate modules_val; +static int sym_warnings; enum symbol_type sym_get_type(struct symbol *sym) { @@ -317,6 +318,14 @@ static void sym_warn_unmet_dep(struct symbol *sym) " Selected by [m]:\n"); fputs(str_get(&gs), stderr); + sym_warnings++; +} + +bool sym_dep_errors(void) +{ + if (sym_warnings) + return getenv("KCONFIG_WERROR"); + return false; } void sym_calc_value(struct symbol *sym)
On Wed, Nov 29, 2023 at 1:13 PM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (23/11/28 23:19), Masahiro Yamada wrote: > > [..] > > > KCONFIG_WERROR is meant to turn all warnings > > to errors. > > I do not see getenv("KCONFIG_WERROR") > > sprinkled everywhere in Kconfig. > > One more thing, you cannot directly exit(1) > > from sym_calc_value(). > > We do exit(1) for KCONFIG_WARN_UNKNOWN_SYMBOLS in conf_read(). > > I can introduce two new helpers that will tell if confdata.c and symbol.c > triggered any warnings and if KCONFIG_WERROR is set. And then different > code paths can call them and handle exit gracefully, depending on the > context (ncurses, menu, etc.). > > Something like this I do not want to patch warnings one by one. I will take some time to think about it.
On (23/12/01 00:42), Masahiro Yamada wrote: > On Wed, Nov 29, 2023 at 1:13 PM Sergey Senozhatsky > <senozhatsky@chromium.org> wrote: > > > > On (23/11/28 23:19), Masahiro Yamada wrote: > > > > [..] > > > > > KCONFIG_WERROR is meant to turn all warnings > > > to errors. > > > I do not see getenv("KCONFIG_WERROR") > > > sprinkled everywhere in Kconfig. > > > One more thing, you cannot directly exit(1) > > > from sym_calc_value(). > > > > We do exit(1) for KCONFIG_WARN_UNKNOWN_SYMBOLS in conf_read(). > > > > I can introduce two new helpers that will tell if confdata.c and symbol.c > > triggered any warnings and if KCONFIG_WERROR is set. And then different > > code paths can call them and handle exit gracefully, depending on the > > context (ncurses, menu, etc.). > > > > Something like this > > > I do not want to patch warnings one by one. > > > I will take some time to think about it. Gentle ping on this. We are not concerned with every possible warning at the moment, however, we do want the critical ones from CI and (semi)automated continuous uprev PoV to be covered by WERROR. We do experience real life problems with "missing direct dependency" not being a terminal condition under WERROR.
On Fri, Dec 22, 2023 at 11:57 AM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (23/12/01 00:42), Masahiro Yamada wrote: > > On Wed, Nov 29, 2023 at 1:13 PM Sergey Senozhatsky > > <senozhatsky@chromium.org> wrote: > > > > > > On (23/11/28 23:19), Masahiro Yamada wrote: > > > > > > [..] > > > > > > > KCONFIG_WERROR is meant to turn all warnings > > > > to errors. > > > > I do not see getenv("KCONFIG_WERROR") > > > > sprinkled everywhere in Kconfig. > > > > One more thing, you cannot directly exit(1) > > > > from sym_calc_value(). > > > > > > We do exit(1) for KCONFIG_WARN_UNKNOWN_SYMBOLS in conf_read(). > > > > > > I can introduce two new helpers that will tell if confdata.c and symbol.c > > > triggered any warnings and if KCONFIG_WERROR is set. And then different > > > code paths can call them and handle exit gracefully, depending on the > > > context (ncurses, menu, etc.). > > > > > > Something like this > > > > > > I do not want to patch warnings one by one. > > > > > > I will take some time to think about it. > > Gentle ping on this. > > We are not concerned with every possible warning at the moment, however, > we do want the critical ones from CI and (semi)automated continuous uprev > PoV to be covered by WERROR. We do experience real life problems with > "missing direct dependency" not being a terminal condition under WERROR. Applied to linux-kbuild. Thanks.
On (23/12/29 22:41), Masahiro Yamada wrote: > > > > We do exit(1) for KCONFIG_WARN_UNKNOWN_SYMBOLS in conf_read(). > > > > > > > > I can introduce two new helpers that will tell if confdata.c and symbol.c > > > > triggered any warnings and if KCONFIG_WERROR is set. And then different > > > > code paths can call them and handle exit gracefully, depending on the > > > > context (ncurses, menu, etc.). > > > > > > > > Something like this > > > > > > > > > I do not want to patch warnings one by one. > > > > > > > > > I will take some time to think about it. > > > > Gentle ping on this. > > > > We are not concerned with every possible warning at the moment, however, > > we do want the critical ones from CI and (semi)automated continuous uprev > > PoV to be covered by WERROR. We do experience real life problems with > > "missing direct dependency" not being a terminal condition under WERROR. > > > Applied to linux-kbuild. Thanks!
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index a76925b46ce6..34fc66e075b7 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -37,6 +37,7 @@ static struct symbol symbol_empty = { struct symbol *modules_sym; static tristate modules_val; +static int sym_warnings; enum symbol_type sym_get_type(struct symbol *sym) { @@ -317,12 +318,14 @@ static void sym_warn_unmet_dep(struct symbol *sym) " Selected by [m]:\n"); fputs(str_get(&gs), stderr); + sym_warnings++; } void sym_calc_value(struct symbol *sym) { struct symbol_value newval, oldval; struct property *prop; + const char *werror; struct expr *e; if (!sym) @@ -338,8 +341,9 @@ void sym_calc_value(struct symbol *sym) sym_calc_value(prop_get_symbol(prop)); } + werror = getenv("KCONFIG_WERROR"); + sym_warnings = 0; sym->flags |= SYMBOL_VALID; - oldval = sym->curr; switch (sym->type) { @@ -430,6 +434,9 @@ void sym_calc_value(struct symbol *sym) ; } + if (sym_warnings && werror) + exit(1); + sym->curr = newval; if (sym_is_choice(sym) && newval.tri == yes) sym->curr.val = sym_calc_choice(sym);
When KCONFIG_WERROR env variable is set treat unmet direct symbol dependency as a terminal condition (error). Suggested-by: Stefan Reinauer <reinauer@google.com> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- scripts/kconfig/symbol.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)