[PATCH] Kbuild: kconfig: Verbose version of --listnewconfig

[PATCH] Kbuild: kconfig: Verbose version of --listnewconfig

[Ben Hutchings]

If the KBUILD_VERBOSE environment variable is set to non-zero, show
the default values of new symbols and not just their names.

Based on work by Bastian Blank <waldi@debian.org> and
maximilian attems <max@stro.at>.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 scripts/kconfig/conf.c     |   90 +++++++++++++++++++++++++++++++++++++++-----
 scripts/kconfig/confdata.c |    1 +
 scripts/kconfig/expr.h     |    2 +
 3 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 5459a38..6d37d5c 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -363,7 +363,6 @@ static void conf(struct menu *menu)
 		switch (prop->type) {
 		case P_MENU:
 			if ((input_mode == silentoldconfig ||
-			     input_mode == listnewconfig ||
 			     input_mode == oldnoconfig) &&
 			    rootEntry != menu) {
 				check_conf(menu);
@@ -423,11 +422,7 @@ static void check_conf(struct menu *menu)
 	if (sym && !sym_has_value(sym)) {
 		if (sym_is_changable(sym) ||
 		    (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)) {
-			if (input_mode == listnewconfig) {
-				if (sym->name && !sym_is_choice_value(sym)) {
-					printf("%s%s\n", CONFIG_, sym->name);
-				}
-			} else if (input_mode != oldnoconfig) {
+			if (input_mode != oldnoconfig) {
 				if (!conf_cnt++)
 					printf(_("*\n* Restart config...\n*\n"));
 				rootEntry = menu_get_parent_menu(menu);
@@ -440,6 +435,78 @@ static void check_conf(struct menu *menu)
 		check_conf(child);
 }
 
+static void report_conf(struct menu *menu, bool verbose)
+{
+	struct symbol *sym;
+	struct menu *child;
+	int l;
+	const char *str;
+
+	if (!menu_is_visible(menu))
+		return;
+
+	if (verbose && menu == &rootmenu) {
+		printf("\n#\n"
+		       "# Changes:\n"
+		       "#\n");
+	}
+
+	sym = menu->sym;
+	if (sym && (sym->flags & SYMBOL_NEW) &&
+	    sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
+		if (verbose) {
+			switch (sym->type) {
+			case S_BOOLEAN:
+			case S_TRISTATE:
+				switch (sym_get_tristate_value(sym)) {
+				case no:
+					printf("# CONFIG_%s is not set\n", sym->name);
+					break;
+				case mod:
+					printf("CONFIG_%s=m\n", sym->name);
+					break;
+				case yes:
+					printf("CONFIG_%s=y\n", sym->name);
+					break;
+				}
+				break;
+			case S_STRING:
+				str = sym_get_string_value(sym);
+				printf("CONFIG_%s=\"", sym->name);
+				while (1) {
+					l = strcspn(str, "\"\\");
+					if (l) {
+						fwrite(str, l, 1, stdout);
+						str += l;
+					}
+					if (!*str)
+						break;
+					printf("\\%c", *str++);
+				}
+				fputs("\"\n", stdout);
+				break;
+			case S_HEX:
+				str = sym_get_string_value(sym);
+				if (str[0] != '0' || (str[1] != 'x' && str[1] != 'X')) {
+					printf("CONFIG_%s=%s\n", sym->name, str);
+					break;
+				}
+			case S_INT:
+				str = sym_get_string_value(sym);
+				printf("CONFIG_%s=%s\n", sym->name, str);
+				break;
+			default:
+				break;
+			}
+		} else {
+			printf("CONFIG_%s\n", sym->name);
+		}
+	}
+
+	for (child = menu->list; child; child = child->next)
+		report_conf(child, verbose);
+}
+
 static struct option long_opts[] = {
 	{"oldaskconfig",    no_argument,       NULL, oldaskconfig},
 	{"oldconfig",       no_argument,       NULL, oldconfig},
@@ -460,6 +527,7 @@ int main(int ac, char **av)
 {
 	int opt;
 	const char *name;
+	const char *value;
 	struct stat tmpstat;
 
 	setlocale(LC_ALL, "");
@@ -604,16 +672,18 @@ int main(int ac, char **av)
 		input_mode = silentoldconfig;
 		/* fall through */
 	case oldconfig:
-	case listnewconfig:
 	case oldnoconfig:
 	case silentoldconfig:
 		/* Update until a loop caused no more changes */
 		do {
 			conf_cnt = 0;
 			check_conf(&rootmenu);
-		} while (conf_cnt &&
-			 (input_mode != listnewconfig &&
-			  input_mode != oldnoconfig));
+		} while (conf_cnt && input_mode != oldnoconfig);
+		break;
+	case listnewconfig:
+		conf_set_all_new_symbols(def_default);
+		value = getenv("KBUILD_VERBOSE");
+		report_conf(&rootmenu, value && atoi(value));
 		break;
 	}
 
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c06f150..3682ef5 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -1009,6 +1009,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
 	for_all_symbols(i, sym) {
 		if (sym_has_value(sym))
 			continue;
+		sym->flags |= SYMBOL_NEW;
 		switch (sym_get_type(sym)) {
 		case S_BOOLEAN:
 		case S_TRISTATE:
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 184eb6a..b267933 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -108,6 +108,8 @@ struct symbol {
 #define SYMBOL_DEF3       0x40000  /* symbol.def[S_DEF_3] is valid */
 #define SYMBOL_DEF4       0x80000  /* symbol.def[S_DEF_4] is valid */
 
+#define SYMBOL_NEW        0x100000 /* symbol is new (loaded config did not provide a value) */
+
 #define SYMBOL_MAXLENGTH	256
 #define SYMBOL_HASHSIZE		9973
 
-- 
1.7.2.3

Re: [PATCH] Kbuild: kconfig: Verbose version of --listnewconfig

[Michal Marek]

On Tue, Nov 23, 2010 at 04:59:57AM +0000, Ben Hutchings wrote:
> If the KBUILD_VERBOSE environment variable is set to non-zero, show
> the default values of new symbols and not just their names.
> 
> Based on work by Bastian Blank <waldi@debian.org> and
> maximilian attems <max@stro.at>.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  scripts/kconfig/conf.c     |   90 +++++++++++++++++++++++++++++++++++++++-----
>  scripts/kconfig/confdata.c |    1 +
>  scripts/kconfig/expr.h     |    2 +
>  3 files changed, 83 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 5459a38..6d37d5c 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -363,7 +363,6 @@ static void conf(struct menu *menu)
>  		switch (prop->type) {
>  		case P_MENU:
>  			if ((input_mode == silentoldconfig ||
> -			     input_mode == listnewconfig ||
>  			     input_mode == oldnoconfig) &&
>  			    rootEntry != menu) {
>  				check_conf(menu);
> @@ -423,11 +422,7 @@ static void check_conf(struct menu *menu)
>  	if (sym && !sym_has_value(sym)) {
>  		if (sym_is_changable(sym) ||
>  		    (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)) {
> -			if (input_mode == listnewconfig) {
> -				if (sym->name && !sym_is_choice_value(sym)) {
> -					printf("%s%s\n", CONFIG_, sym->name);
> -				}
> -			} else if (input_mode != oldnoconfig) {
> +			if (input_mode != oldnoconfig) {
>  				if (!conf_cnt++)
>  					printf(_("*\n* Restart config...\n*\n"));
>  				rootEntry = menu_get_parent_menu(menu);
> @@ -440,6 +435,78 @@ static void check_conf(struct menu *menu)
>  		check_conf(child);
>  }
>  
> +static void report_conf(struct menu *menu, bool verbose)
> +{
> +	struct symbol *sym;
> +	struct menu *child;
> +	int l;
> +	const char *str;
> +
> +	if (!menu_is_visible(menu))
> +		return;
> +
> +	if (verbose && menu == &rootmenu) {
> +		printf("\n#\n"
> +		       "# Changes:\n"
> +		       "#\n");
> +	}
> +
> +	sym = menu->sym;
> +	if (sym && (sym->flags & SYMBOL_NEW) &&
> +	    sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
> +		if (verbose) {
> +			switch (sym->type) {
> +			case S_BOOLEAN:
> +			case S_TRISTATE:
> +				switch (sym_get_tristate_value(sym)) {
> +				case no:
> +					printf("# CONFIG_%s is not set\n", sym->name);
> +					break;
> [...]

Hi,

this is almost a 1:1 copy of the conf_write_symbol() function, so what
about reusing it like this? Otherwise the patch looks OK.

Michal


Subject: [PATCH] kconfig: Use conf_write_symbol() in listnewconfig

Signed-off-by: Michal Marek <mmarek@suse.cz>

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 6d37d5c..41731c7 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -439,8 +439,6 @@ static void report_conf(struct menu *menu, bool verbose)
 {
 	struct symbol *sym;
 	struct menu *child;
-	int l;
-	const char *str;
 
 	if (!menu_is_visible(menu))
 		return;
@@ -455,49 +453,7 @@ static void report_conf(struct menu *menu, bool verbose)
 	if (sym && (sym->flags & SYMBOL_NEW) &&
 	    sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
 		if (verbose) {
-			switch (sym->type) {
-			case S_BOOLEAN:
-			case S_TRISTATE:
-				switch (sym_get_tristate_value(sym)) {
-				case no:
-					printf("# CONFIG_%s is not set\n", sym->name);
-					break;
-				case mod:
-					printf("CONFIG_%s=m\n", sym->name);
-					break;
-				case yes:
-					printf("CONFIG_%s=y\n", sym->name);
-					break;
-				}
-				break;
-			case S_STRING:
-				str = sym_get_string_value(sym);
-				printf("CONFIG_%s=\"", sym->name);
-				while (1) {
-					l = strcspn(str, "\"\\");
-					if (l) {
-						fwrite(str, l, 1, stdout);
-						str += l;
-					}
-					if (!*str)
-						break;
-					printf("\\%c", *str++);
-				}
-				fputs("\"\n", stdout);
-				break;
-			case S_HEX:
-				str = sym_get_string_value(sym);
-				if (str[0] != '0' || (str[1] != 'x' && str[1] != 'X')) {
-					printf("CONFIG_%s=%s\n", sym->name, str);
-					break;
-				}
-			case S_INT:
-				str = sym_get_string_value(sym);
-				printf("CONFIG_%s=%s\n", sym->name, str);
-				break;
-			default:
-				break;
-			}
+			conf_write_symbol(sym, sym->type, stdout, true);
 		} else {
 			printf("CONFIG_%s\n", sym->name);
 		}
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 44c9d62..1955b48 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -440,7 +440,7 @@ static void conf_write_string(bool headerfile, const char *name,
 	fputs("\"\n", out);
 }
 
-static void conf_write_symbol(struct symbol *sym, enum symbol_type type,
+void conf_write_symbol(struct symbol *sym, enum symbol_type type,
                               FILE *out, bool write_no)
 {
 	const char *str;
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index 17342fe..6da571b 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -7,6 +7,7 @@ P(conf_read_simple,int,(const char *name, int));
 P(conf_write_defconfig,int,(const char *name));
 P(conf_write,int,(const char *name));
 P(conf_write_autoconf,int,(void));
+P(conf_write_symbol, void,(struct symbol*, enum symbol_type, FILE*, bool));
 P(conf_get_changed,bool,(void));
 P(conf_set_changed_callback, void,(void (*fn)(void)));
 P(conf_set_message_callback, void,(void (*fn)(const char *fmt, va_list ap)));

[PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Ben Hutchings]

If the KBUILD_VERBOSE environment variable is set to non-zero, show
the default values of new symbols and not just their names.

Based on work by Bastian Blank <waldi@debian.org> and
maximilian attems <max@stro.at>.  Simplified by Michal Marek
<mmarek@suse.cz>.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
On Fri, 2010-12-03 at 13:23 +0100, Michal Marek wrote:
[...]
> this is almost a 1:1 copy of the conf_write_symbol() function, so what
> about reusing it like this? Otherwise the patch looks OK.
[...]

Well spotted - I assume that function didn't exist when this code was
first added in Debian, but it does seem to be exactly equivalent.  I've
incorporated your change into this version.

Ben.

 scripts/kconfig/conf.c      |   45 +++++++++++++++++++++++++++++++++---------
 scripts/kconfig/confdata.c  |    5 ++-
 scripts/kconfig/expr.h      |    2 +
 scripts/kconfig/lkc_proto.h |    1 +
 4 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 5459a38..f4752b4 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -363,7 +363,6 @@ static void conf(struct menu *menu)
 		switch (prop->type) {
 		case P_MENU:
 			if ((input_mode == silentoldconfig ||
-			     input_mode == listnewconfig ||
 			     input_mode == oldnoconfig) &&
 			    rootEntry != menu) {
 				check_conf(menu);
@@ -423,11 +422,7 @@ static void check_conf(struct menu *menu)
 	if (sym && !sym_has_value(sym)) {
 		if (sym_is_changable(sym) ||
 		    (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)) {
-			if (input_mode == listnewconfig) {
-				if (sym->name && !sym_is_choice_value(sym)) {
-					printf("%s%s\n", CONFIG_, sym->name);
-				}
-			} else if (input_mode != oldnoconfig) {
+			if (input_mode != oldnoconfig) {
 				if (!conf_cnt++)
 					printf(_("*\n* Restart config...\n*\n"));
 				rootEntry = menu_get_parent_menu(menu);
@@ -440,6 +435,33 @@ static void check_conf(struct menu *menu)
 		check_conf(child);
 }
 
+static void report_conf(struct menu *menu, bool verbose)
+{
+	struct symbol *sym;
+	struct menu *child;
+
+	if (!menu_is_visible(menu))
+		return;
+
+	if (verbose && menu == &rootmenu) {
+		printf("\n#\n"
+		       "# Changes:\n"
+		       "#\n");
+	}
+
+	sym = menu->sym;
+	if (sym && (sym->flags & SYMBOL_NEW) &&
+	    sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
+		if (verbose)
+			conf_write_symbol(sym, sym->type, stdout, true);
+		else
+			printf("CONFIG_%s\n", sym->name);
+	}
+
+	for (child = menu->list; child; child = child->next)
+		report_conf(child, verbose);
+}
+
 static struct option long_opts[] = {
 	{"oldaskconfig",    no_argument,       NULL, oldaskconfig},
 	{"oldconfig",       no_argument,       NULL, oldconfig},
@@ -460,6 +482,7 @@ int main(int ac, char **av)
 {
 	int opt;
 	const char *name;
+	const char *value;
 	struct stat tmpstat;
 
 	setlocale(LC_ALL, "");
@@ -604,16 +627,18 @@ int main(int ac, char **av)
 		input_mode = silentoldconfig;
 		/* fall through */
 	case oldconfig:
-	case listnewconfig:
 	case oldnoconfig:
 	case silentoldconfig:
 		/* Update until a loop caused no more changes */
 		do {
 			conf_cnt = 0;
 			check_conf(&rootmenu);
-		} while (conf_cnt &&
-			 (input_mode != listnewconfig &&
-			  input_mode != oldnoconfig));
+		} while (conf_cnt && input_mode != oldnoconfig);
+		break;
+	case listnewconfig:
+		conf_set_all_new_symbols(def_default);
+		value = getenv("KBUILD_VERBOSE");
+		report_conf(&rootmenu, value && atoi(value));
 		break;
 	}
 
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c06f150..fbbacac 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -440,8 +440,8 @@ static void conf_write_string(bool headerfile, const char *name,
 	fputs("\"\n", out);
 }
 
-static void conf_write_symbol(struct symbol *sym, enum symbol_type type,
-                              FILE *out, bool write_no)
+void conf_write_symbol(struct symbol *sym, enum symbol_type type,
+		       FILE *out, bool write_no)
 {
 	const char *str;
 
@@ -1009,6 +1009,7 @@ void conf_set_all_new_symbols(enum conf_def_mode mode)
 	for_all_symbols(i, sym) {
 		if (sym_has_value(sym))
 			continue;
+		sym->flags |= SYMBOL_NEW;
 		switch (sym_get_type(sym)) {
 		case S_BOOLEAN:
 		case S_TRISTATE:
diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 184eb6a..b267933 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -108,6 +108,8 @@ struct symbol {
 #define SYMBOL_DEF3       0x40000  /* symbol.def[S_DEF_3] is valid */
 #define SYMBOL_DEF4       0x80000  /* symbol.def[S_DEF_4] is valid */
 
+#define SYMBOL_NEW        0x100000 /* symbol is new (loaded config did not provide a value) */
+
 #define SYMBOL_MAXLENGTH	256
 #define SYMBOL_HASHSIZE		9973
 
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index 17342fe..6da571b 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -7,6 +7,7 @@ P(conf_read_simple,int,(const char *name, int));
 P(conf_write_defconfig,int,(const char *name));
 P(conf_write,int,(const char *name));
 P(conf_write_autoconf,int,(void));
+P(conf_write_symbol, void,(struct symbol*, enum symbol_type, FILE*, bool));
 P(conf_get_changed,bool,(void));
 P(conf_set_changed_callback, void,(void (*fn)(void)));
 P(conf_set_message_callback, void,(void (*fn)(const char *fmt, va_list ap)));
-- 
1.7.2.3



-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 12:10 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> If the KBUILD_VERBOSE environment variable is set to non-zero, show
> the default values of new symbols and not just their names.
>
> Based on work by Bastian Blank <waldi@debian.org> and
> maximilian attems <max@stro.at>.  Simplified by Michal Marek
> <mmarek@suse.cz>.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> On Fri, 2010-12-03 at 13:23 +0100, Michal Marek wrote:
> [...]
>> this is almost a 1:1 copy of the conf_write_symbol() function, so what
>> about reusing it like this? Otherwise the patch looks OK.
> [...]
>
> Well spotted - I assume that function didn't exist when this code was
> first added in Debian, but it does seem to be exactly equivalent.  I've
> incorporated your change into this version.
>
> Ben.
>
>  scripts/kconfig/conf.c      |   45 +++++++++++++++++++++++++++++++++---------
>  scripts/kconfig/confdata.c  |    5 ++-
>  scripts/kconfig/expr.h      |    2 +
>  scripts/kconfig/lkc_proto.h |    1 +
>  4 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 5459a38..f4752b4 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -363,7 +363,6 @@ static void conf(struct menu *menu)
>                switch (prop->type) {
>                case P_MENU:
>                        if ((input_mode == silentoldconfig ||
> -                            input_mode == listnewconfig ||
>                             input_mode == oldnoconfig) &&
>                            rootEntry != menu) {
>                                check_conf(menu);
> @@ -423,11 +422,7 @@ static void check_conf(struct menu *menu)
>        if (sym && !sym_has_value(sym)) {
>                if (sym_is_changable(sym) ||
>                    (sym_is_choice(sym) && sym_get_tristate_value(sym) == yes)) {
> -                       if (input_mode == listnewconfig) {
> -                               if (sym->name && !sym_is_choice_value(sym)) {
> -                                       printf("%s%s\n", CONFIG_, sym->name);
> -                               }
> -                       } else if (input_mode != oldnoconfig) {
> +                       if (input_mode != oldnoconfig) {
>                                if (!conf_cnt++)
>                                        printf(_("*\n* Restart config...\n*\n"));
>                                rootEntry = menu_get_parent_menu(menu);
> @@ -440,6 +435,33 @@ static void check_conf(struct menu *menu)
>                check_conf(child);
>  }
>
> +static void report_conf(struct menu *menu, bool verbose)
> +{
> +       struct symbol *sym;
> +       struct menu *child;
> +
> +       if (!menu_is_visible(menu))
> +               return;
> +
> +       if (verbose && menu == &rootmenu) {
> +               printf("\n#\n"
> +                      "# Changes:\n"
> +                      "#\n");
> +       }
> +
I would not expect to see any header if there is no new symbol(s).
However, that might complicate the code too much. Btw, I find
"Changes" to be misleading, is that header necessary ?

> +       sym = menu->sym;
> +       if (sym && (sym->flags & SYMBOL_NEW) &&
> +           sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
> +               if (verbose)
> +                       conf_write_symbol(sym, sym->type, stdout, true);
> +               else
> +                       printf("CONFIG_%s\n", sym->name);
Please don't hardcode the prefix string. This should be:

                       printf("%s%s\n", CONFIG_, sym->name);

 - Arnaud

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Sat, 2010-12-04 at 13:11 -0500, Arnaud Lacombe wrote:
[...]
> > +static void report_conf(struct menu *menu, bool verbose)
> > +{
> > +       struct symbol *sym;
> > +       struct menu *child;
> > +
> > +       if (!menu_is_visible(menu))
> > +               return;
> > +
> > +       if (verbose && menu == &rootmenu) {
> > +               printf("\n#\n"
> > +                      "# Changes:\n"
> > +                      "#\n");
> > +       }
> > +
> I would not expect to see any header if there is no new symbol(s).
> However, that might complicate the code too much. Btw, I find
> "Changes" to be misleading, is that header necessary ?

We use this feature (or an earlier version of it) in automated kernel
builds in Debian, so we expect the output to appear in build logs and
the header makes it easier to pick out.

> > +       sym = menu->sym;
> > +       if (sym && (sym->flags & SYMBOL_NEW) &&
> > +           sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
> > +               if (verbose)
> > +                       conf_write_symbol(sym, sym->type, stdout, true);
> > +               else
> > +                       printf("CONFIG_%s\n", sym->name);
> Please don't hardcode the prefix string. This should be:
> 
>                        printf("%s%s\n", CONFIG_, sym->name);

OK, but I don't understand why it would change.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 1:30 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> [...]
>> > +       sym = menu->sym;
>> > +       if (sym && (sym->flags & SYMBOL_NEW) &&
>> > +           sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
>> > +               if (verbose)
>> > +                       conf_write_symbol(sym, sym->type, stdout, true);
>> > +               else
>> > +                       printf("CONFIG_%s\n", sym->name);
>> Please don't hardcode the prefix string. This should be:
>>
>>                        printf("%s%s\n", CONFIG_, sym->name);
>
> OK, but I don't understand why it would change.
>
the Linux kernel is not the only user of kconfig.

 - Arnaud

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 1:30 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sat, 2010-12-04 at 13:11 -0500, Arnaud Lacombe wrote:
> [...]
>> > +static void report_conf(struct menu *menu, bool verbose)
>> > +{
>> > +       struct symbol *sym;
>> > +       struct menu *child;
>> > +
>> > +       if (!menu_is_visible(menu))
>> > +               return;
>> > +
>> > +       if (verbose && menu == &rootmenu) {
>> > +               printf("\n#\n"
>> > +                      "# Changes:\n"
>> > +                      "#\n");
>> > +       }
>> > +
FWIW, some more nits about this header:
 - I'd rather either always or never see it, not depending on whether
or not KBUILD_VERBOSE is set to non-zero.
 - The "menu == &rootmenu" test is pretty useless, just move the
display (if any) outside of report_conf(), where there is no
ambiguity.
 - And why do you need a leading newline in front of the header (ie.
the "\n#\n") ? The only places where this construct is used in conf is
when there is a need to highlight the message, which should not be
needed here.

>> I would not expect to see any header if there is no new symbol(s).
>> However, that might complicate the code too much. Btw, I find
>> "Changes" to be misleading, is that header necessary ?
>
> We use this feature (or an earlier version of it) in automated kernel
> builds in Debian, so we expect the output to appear in build logs and
> the header makes it easier to pick out.
>
That's easily doable outside kconfig.

 - Arnaud

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Sam Ravnborg]

On Sat, Dec 04, 2010 at 06:30:21PM +0000, Ben Hutchings wrote:
> On Sat, 2010-12-04 at 13:11 -0500, Arnaud Lacombe wrote:
> [...]
> > > +static void report_conf(struct menu *menu, bool verbose)
> > > +{
> > > +       struct symbol *sym;
> > > +       struct menu *child;
> > > +
> > > +       if (!menu_is_visible(menu))
> > > +               return;
> > > +
> > > +       if (verbose && menu == &rootmenu) {
> > > +               printf("\n#\n"
> > > +                      "# Changes:\n"
> > > +                      "#\n");
> > > +       }
> > > +
> > I would not expect to see any header if there is no new symbol(s).
> > However, that might complicate the code too much. Btw, I find
> > "Changes" to be misleading, is that header necessary ?
> 
> We use this feature (or an earlier version of it) in automated kernel
> builds in Debian, so we expect the output to appear in build logs and
> the header makes it easier to pick out.
> 
> > > +       sym = menu->sym;
> > > +       if (sym && (sym->flags & SYMBOL_NEW) &&
> > > +           sym_is_changable(sym) && sym->name && !sym_is_choice_value(sym)) {
> > > +               if (verbose)
> > > +                       conf_write_symbol(sym, sym->type, stdout, true);
> > > +               else
> > > +                       printf("CONFIG_%s\n", sym->name);
> > Please don't hardcode the prefix string. This should be:
> > 
> >                        printf("%s%s\n", CONFIG_, sym->name);
> 
> OK, but I don't understand why it would change.

Other projects uses kconfig and they use other prefix.

	Sam

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

On Sat, 2010-12-04 at 14:53 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Sat, Dec 4, 2010 at 1:30 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Sat, 2010-12-04 at 13:11 -0500, Arnaud Lacombe wrote:
> > [...]
> >> > +static void report_conf(struct menu *menu, bool verbose)
> >> > +{
> >> > +       struct symbol *sym;
> >> > +       struct menu *child;
> >> > +
> >> > +       if (!menu_is_visible(menu))
> >> > +               return;
> >> > +
> >> > +       if (verbose && menu == &rootmenu) {
> >> > +               printf("\n#\n"
> >> > +                      "# Changes:\n"
> >> > +                      "#\n");
> >> > +       }
> >> > +
> FWIW, some more nits about this header:
>  - I'd rather either always or never see it, not depending on whether
> or not KBUILD_VERBOSE is set to non-zero.

The other users of listnewconfig obviously didn't want that.

>  - The "menu == &rootmenu" test is pretty useless, just move the
> display (if any) outside of report_conf(), where there is no
> ambiguity.
>  - And why do you need a leading newline in front of the header (ie.
> the "\n#\n") ? The only places where this construct is used in conf is
> when there is a need to highlight the message, which should not be
> needed here.
> 
> >> I would not expect to see any header if there is no new symbol(s).
> >> However, that might complicate the code too much. Btw, I find
> >> "Changes" to be misleading, is that header necessary ?
> >
> > We use this feature (or an earlier version of it) in automated kernel
> > builds in Debian, so we expect the output to appear in build logs and
> > the header makes it easier to pick out.
> >
> That's easily doable outside kconfig.

No it isn't, as the Kconfig code must be built as part of the
listnewconfig target.  (Building just the code first will provoke
warnings about the invalid config.)

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 12:10 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> If the KBUILD_VERBOSE environment variable is set to non-zero, show
> the default values of new symbols and not just their names.
>
One more thing. This patch would be the first to introduce a KBUILD_
environment variable into kconfig. All the other environment variable
used are prefixed with KCONFIG_. Maybe would it be better to introduce
a KCONFIG_VERBOSE variable and link it to KBUILD_VERBOSE in kconfig's
Makefile. That would also remove the need to atoi() its value.

 - Arnaud

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 4:07 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
[...]
>> >> I would not expect to see any header if there is no new symbol(s).
>> >> However, that might complicate the code too much. Btw, I find
>> >> "Changes" to be misleading, is that header necessary ?
>> >
>> > We use this feature (or an earlier version of it) in automated kernel
>> > builds in Debian, so we expect the output to appear in build logs and
>> > the header makes it easier to pick out.
>> >
>> That's easily doable outside kconfig.
>
> No it isn't, as the Kconfig code must be built as part of the
> listnewconfig target.  (Building just the code first will provoke
> warnings about the invalid config.)
>
echo "#"
echo "# Changes:"
echo "#"
make [...] listnewconfig

does the job, without introducing extra header others do not seem to
want to see.

 - Arnaud

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

[This message is a bit out-of-scope of this patch]

On Sat, Dec 4, 2010 at 12:10 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index 184eb6a..b267933 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -108,6 +108,8 @@ struct symbol {
>  #define SYMBOL_DEF3       0x40000  /* symbol.def[S_DEF_3] is valid */
>  #define SYMBOL_DEF4       0x80000  /* symbol.def[S_DEF_4] is valid */
>
> +#define SYMBOL_NEW        0x100000 /* symbol is new (loaded config did not provide a value) */
> +
>  #define SYMBOL_MAXLENGTH       256
>  #define SYMBOL_HASHSIZE                9973
>
Just for the record, as long as I like this explicit flag, it does
only make sense for the S_DEF_USER case.

Kconfig seems to have been thought originally to support multiple
state of the tree (through the S_DEF_* flags). However, there has been
years after years more and more code written solely for the S_DEF_USER
case. Nowadays, making useful use of these S_DEF_* flags has became
pretty much impossible.

If the goal of this patch was just to provide a verbose
--listnewconfig, that should have been doable within check_conf().

Just a side note, using conf_write symbol() has the disadvantage to print:

# CONFIG_FOO is not set

instead of

CONFIG_FOO=n

or

CONFIG_FOO is not set

I've never been a big fan of having the format included in a generic
library. I guess the caller should specify a printer callback. That
would avoid also the `headerfile' fuss of conf_write_string.

I'd suggest the printer callback to be of the form:

struct conf_printer_callback {
    void (*print_set)(struct symbol *);
    void (*print_unset)(struct symbol *);
    void (*print_comment)(struct symbol *);
}

that way, conf_write() and conf_write_autoconf() would also share more code...

 - Arnaud, who had other plan for the day :/

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

On Sat, 2010-12-04 at 16:14 -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Sat, Dec 4, 2010 at 4:07 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> [...]
> >> >> I would not expect to see any header if there is no new symbol(s).
> >> >> However, that might complicate the code too much. Btw, I find
> >> >> "Changes" to be misleading, is that header necessary ?
> >> >
> >> > We use this feature (or an earlier version of it) in automated kernel
> >> > builds in Debian, so we expect the output to appear in build logs and
> >> > the header makes it easier to pick out.
> >> >
> >> That's easily doable outside kconfig.
> >
> > No it isn't, as the Kconfig code must be built as part of the
> > listnewconfig target.  (Building just the code first will provoke
> > warnings about the invalid config.)
> >
> echo "#"
> echo "# Changes:"
> echo "#"
> make [...] listnewconfig
> 
> does the job, without introducing extra header others do not seem to
> want to see.

Now try that with a clean tree and an outdated config file.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCHv2] Kbuild: kconfig: Verbose version of --listnewconfig

[Arnaud Lacombe]

Hi,

On Sat, Dec 4, 2010 at 4:53 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sat, 2010-12-04 at 16:14 -0500, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Sat, Dec 4, 2010 at 4:07 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> [...]
>> >> >> I would not expect to see any header if there is no new symbol(s).
>> >> >> However, that might complicate the code too much. Btw, I find
>> >> >> "Changes" to be misleading, is that header necessary ?
>> >> >
>> >> > We use this feature (or an earlier version of it) in automated kernel
>> >> > builds in Debian, so we expect the output to appear in build logs and
>> >> > the header makes it easier to pick out.
>> >> >
>> >> That's easily doable outside kconfig.
>> >
>> > No it isn't, as the Kconfig code must be built as part of the
>> > listnewconfig target.  (Building just the code first will provoke
>> > warnings about the invalid config.)
>> >
>> echo "#"
>> echo "# Changes:"
>> echo "#"
>> make [...] listnewconfig
>>
>> does the job, without introducing extra header others do not seem to
>> want to see.
>
> Now try that with a clean tree and an outdated config file.
>
make -s listnewconfig

is perfectly silent[0]. Ok, I admit, I had to cheat, ie. I built
kconfig with HOST_EXTRACFLAGS="-Wno-char-subscripts". You may also
want to use `silentoldconfig' beforehand to tell you if the config
needs adjustment or not, in which case, kconfig no longer has to be
built.

Now, if by "warnings about the invalid config" you mean the illegal
use of the "select" keyword, I'd say that these warnings should not
happen in a release kernel, and time has better to be spent fixing
them than putting some marker to skip them. Note that this task is up
to the subsystem maintainer, in most case.

 - Arnaud

[0]: on top of Linus' tree as of Nov. 5.