[15/15] kconfig: fix randomising choice entries in presence of KCONFIG_ALLCONFIG
diff mbox

Message ID 8357b48549e17b3e4e402c7f977b65708922e60f.1372097219.git.yann.morin.1998@free.fr
State New, archived
Headers show

Commit Message

Yann E. MORIN June 24, 2013, 6:11 p.m. UTC
From: "Yann E. MORIN" <yann.morin.1998@free.fr>

Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
is specified.

For example, given those two files (Thomas' test-case):

    ---8<--- Config.test.in
    config OPTIONA
        bool "Option A"

    choice
        prompt "This is a choice"

    config CHOICE_OPTIONA
        bool "Choice Option A"

    config CHOICE_OPTIONB
        bool "Choice Option B"

    endchoice

    config OPTIONB
        bool "Option B"
    ---8<--- Config.test.in

    ---8<--- config.defaults
    CONFIG_OPTIONA=y
    ---8<--- config.defaults

And running:
    ./scripts/kconfig/conf --randconfig Config.test.in

does properly randomise the two choice symbols (and the two booleans).

However, running:
    KCONFIG_ALLCONFIG=config.defaults \
    ./scripts/kconfig/conf --randconfig Config.test.in

does *not* reandomise the two choice entries, and only CHOICE_OPTIONA
will ever be selected. (OPTIONA will always be set (expected), and
OPTIONB will be be properly randomised (expected).)

This patch defers setting that a choice has a value until a symbol for
that choice is indeed set, so that choices are properly randomised when
KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>

---
Changes v3 -> v4
  - fix previous issue where some choices would not be set, which would
    cause silentoldconfig to ask for them and was then breaking this
    workflow (as reported by Arnd and Sedat):
        KCONFIG_ALLCONFIG=foo.defconfig make randconfig
        make silentoldconfig </dev/nullo
    which I have tested (3h28min!) with:
        touch defconfig
        for(( i=0; i<10000; i++ )); do
            KCONFIG_ALLCONFIG=$(pwd)/defconfig make randconfig >/dev/null 2>&1
            make silentoldconfig </dev/null >/dev/null 2>&1 || break
        done
    which did not break at all.
  - change done in v3 (below) is already fixed by a previous patch

Changes v2 -> v3
  - ensure only one symbol is set in a choice

Changes v1 -> v2:
  - further postpone setting that a choice has a value until
    one is indeed set
  - do not print symbols that are part of an invisible choice
---
 scripts/kconfig/confdata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Yann E. MORIN June 25, 2013, 8:01 a.m. UTC | #1
Michal, All,

On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

I've just been informed by Fengguang that this patch breaks yet another
use-case of his, but this time KCONFIG_ALLCONFIG is not involved.

I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
Sorry for the breakage (again...). :-(

We really need a known, shared test-suite for kconfig... :-/

Regards,
Yann E. MORIN.
Sedat Dilek June 25, 2013, 8:10 a.m. UTC | #2
On Tue, Jun 25, 2013 at 10:01 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Michal, All,
>
> On Monday 24 June 2013 20:11:59 Yann E. MORIN wrote:
>> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
>> is specified.
> [--SNIP--]
>> This patch defers setting that a choice has a value until a symbol for
>> that choice is indeed set, so that choices are properly randomised when
>> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.
>
> I've just been informed by Fengguang that this patch breaks yet another
> use-case of his, but this time KCONFIG_ALLCONFIG is not involved.
>
> I'll investigate this issue when I'm back home tonight (ie. ~16:00 UTC).
> Sorry for the breakage (again...). :-(
>
> We really need a known, shared test-suite for kconfig... :-/
>

[ CC Alexander ]

A-propos, "testing Kconfig" I recommended to take into account the
script of Alexander.
In PM we discussed on where to place such scripts and that Alexander's
patch needs some refreshing in your eyes.
Maybe, Alexander can/will help as he is the original author.
As said, this script helped in the Freetz router project to eliminate
dozens of (logically) wrong Kconfig settings.
IMHO there exist a lot of more, as there are "double/triple" checks in
the Freetz-buildsystem, but hey noone is perfect :-).

- Sedat -

[1] http://freetz.org/browser/trunk/tools/developer/create-kconfig-warnings

> Regards,
> Yann E. MORIN.
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +0/33 662376056 | Software  Designer | \ / CAMPAIGN     |   ^                |
> | --==< O_o >==-- '------------.-------:  X  AGAINST      |  /e\  There is no  |
> | http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL    |  """  conspiracy.  |
> '------------------------------'-------'------------------'--------------------'
--
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
Yann E. MORIN June 25, 2013, 8:58 p.m. UTC | #3
Michal, All,

On 2013-06-24 20:11 +0200, Yann E. MORIN spake thusly:
> Currently, randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG
> is specified.
[--SNIP--]
> This patch defers setting that a choice has a value until a symbol for
> that choice is indeed set, so that choices are properly randomised when
> KCONFIG_ALLCONFIG is set, but not if a symbol for that choice is set.

OK, so this second attempt also broke standard workflows. Fixing it is
more involved than expected. Let's get rid of it before too many people
complain, and before it hits mainline.

Michal, can you strip this from your tree (since it's the HEAD of your
kconfig branch), or do you want me to submit a revert patch?

Regards,
Yann E. MORIN.

Patch
diff mbox

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c55c227..3e39208 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -288,8 +288,6 @@  load:
 	for_all_symbols(i, sym) {
 		sym->flags |= SYMBOL_CHANGED;
 		sym->flags &= ~(def_flags|SYMBOL_VALID);
-		if (sym_is_choice(sym))
-			sym->flags |= def_flags;
 		switch (sym->type) {
 		case S_INT:
 		case S_HEX:
@@ -379,13 +377,13 @@  setsym:
 			case mod:
 				if (cs->def[def].tri == yes) {
 					conf_warning("%s creates inconsistent choice state", sym->name);
-					cs->flags &= ~def_flags;
 				}
 				break;
 			case yes:
 				if (cs->def[def].tri != no)
 					conf_warning("override: %s changes choice state", sym->name);
 				cs->def[def].val = sym;
+				cs->flags |= def_flags;
 				break;
 			}
 			cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri);
@@ -791,6 +789,8 @@  int conf_write(const char *name)
 			sym_calc_value(sym);
 			if (!(sym->flags & SYMBOL_WRITE))
 				goto next;
+			if (sym_is_choice_value(sym) && !menu_is_visible(menu->parent))
+				goto next;
 			sym->flags &= ~SYMBOL_WRITE;
 
 			conf_write_symbol(out, sym, &kconfig_printer_cb, NULL);