[v2] kconfig: check for select dependency errors on config load
diff mbox

Message ID alpine.DEB.2.00.0902211535440.18221@gandalf.stny.rr.com
State New, archived
Headers show

Commit Message

Steven Rostedt Feb. 21, 2009, 8:46 p.m. UTC
On Sat, 21 Feb 2009, Steven Rostedt wrote:
> On Fri, 20 Feb 2009, Randy Dunlap wrote:
> > EMBEDDED is misnamed.  It means "those who think that they know enough
> > to use all of the power of kconfig."
> > Some people spell that EXPERT etc.
> > Or it means "let me shoot myself in the foot."

> 
> Anyway, to avoid having these spit out all the time, I'll see if I can 
> make it not warn if the dependency was on EMBEDDED/EXPERT. I'll try to 
> work on it when I have time.

That was easier than I expected. Here's an updated version of the patch.

It now finds the symbol EMBEDDED, saves its state. Updates its state to 
'yes', runs the tests, resets its state back to what it was.

Now all I get from that previous config:

$ make menuconfig
scripts/kconfig/mconf arch/x86/Kconfig
.config:2561:warning: SCSI_SAS_LIBSAS selects SCSI_SAS_ATTRS which fails its dependencies!
.config:2561:warning: DRM selects I2C_ALGOBIT which fails its dependencies!

Hmm, that I2C_ALGOBIT depends on I2C_HELPER_AUTO, which looks similar to 
the EMBEDDED option. That is, Don't show this option unless this is not 
selected. I may just add a 'WHITELIST' of things that will be enabled 
during thet test.

But the SCSI_SAS_ATTRS depends on BLK_DEV_BSG which is not selected.


I also added to run the test on write as well.

But having it find the culprits will require a bit more that 5 minutes of
work. That will stay on my TODO list for now.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>


--
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

Comments

Steven Rostedt Feb. 21, 2009, 8:48 p.m. UTC | #1
On Sat, 21 Feb 2009, Steven Rostedt wrote:
> 
> Hmm, that I2C_ALGOBIT depends on I2C_HELPER_AUTO, which looks similar to 
> the EMBEDDED option. That is, Don't show this option unless this is not 
> selected. I may just add a 'WHITELIST' of things that will be enabled 
> during thet test.

It is actually the opposite of EMBEDDED. It is, show this item if 
I2C_HELPER_AUTO is _not_ selected.

-- Steve

--
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
Steven Rostedt Feb. 21, 2009, 9:53 p.m. UTC | #2
On Sat, 21 Feb 2009, Sam Ravnborg wrote:
> 
> We do not want to have such hardcoded information
> about the Kconfig structure in the backend.
> Rather we should fix the configuration once and for all so EMBEDDED
> get split into the sensible options is actually is.
> 
> Options that come to my mind is:
> OPTIMIZE_FOR_TEXT_SIZE
> OPTIMIZE_FOR_DATA_SIZE
> KERNEL_EXPERT
> 
> And we should them make their use of select conform
> like the rest of the kernel configuration.
> 
> So the first version of your patch is IMO better - but
> it could use some comments so others better follows what is
> going on. The current level of comments in the kconfig
> source base is not an example to follw

Sounds good. Yeah, the first patch was an RFC mainly because I want 
someone to look at the code and tell me "yeah that works" or "No! you 
can't do that". Because I spent several hours staring at the kconfig code 
trying to figure out WTF it was doing. Luckily, I did have a compiler 
writing course and the flex and bison code was fine for me.

I will definitely update the code with comments when I'm more comfortable 
with the code.

-- Steve

--
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

Patch
diff mbox

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 830d9ea..7717926 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -310,6 +310,127 @@  load:
 	return 0;
 }
 
+static int test_deps(struct symbol *sym)
+{
+	struct property *prop;
+	struct expr *expr;
+
+	if (sym->rev_dep.tri == no)
+		return 1;
+
+	for (prop = sym->prop; prop; prop = prop->next) {
+		if (prop->type != P_PROMPT)
+			continue;
+
+		expr = prop->visible.expr;
+		if (!expr)
+			continue;
+
+		if (expr_calc_value(expr) == no)
+			return 0;
+	}
+
+	return 1;
+}
+
+void test_select(struct symbol *sym)
+{
+	struct property *prop;
+	struct expr *expr;
+
+	for (prop = sym->prop; prop; prop = prop->next) {
+		if (prop->type != P_SELECT)
+			continue;
+
+		expr = prop->expr;
+
+		if (!expr || expr->type != E_SYMBOL ||
+		    !expr->left.sym)
+			continue;
+			
+		if (test_deps(expr->left.sym))
+			continue;
+
+		conf_warning("%s selects %s which fails its dependencies!",
+			     sym->name, expr->left.sym->name);
+	}
+}
+
+void test_conf(void)
+{
+	struct symbol *sym, *embedded;
+	tristate embed_tri;
+	struct menu *menu;
+	int type;
+
+	/*
+	 * EMBEDDED is more like an "EXPERT" option. It is OK
+	 * to select it even when it does not have the proper
+	 * dependencies.
+	 *
+	 * Find  the EMBEDDED symbol.
+	 * Save its state.
+	 * Set it to 'yes'.
+	 * Run tests.
+	 * Reset the EMBEDDED symbol.
+	 */
+	embedded = sym_lookup("EMBEDDED", 0);
+	embed_tri = embedded->curr.tri;
+	embedded->curr.tri = yes;
+	
+	menu = rootmenu.list;
+	while (menu) {
+		sym = menu->sym;
+		if (!sym)
+			goto next;
+
+		if (!(sym->flags & SYMBOL_CHOICE)) {
+			sym_calc_value(sym);
+			if (!(sym->flags & SYMBOL_WRITE))
+				goto next;
+			type = sym->type;
+			if (type == S_TRISTATE) {
+				sym_calc_value(modules_sym);
+				if (modules_sym->curr.tri == no)
+					type = S_BOOLEAN;
+			}
+			switch (type) {
+			case S_BOOLEAN:
+			case S_TRISTATE:
+				switch (sym_get_tristate_value(sym)) {
+				case no:
+					break;
+				case mod:
+				case yes:
+					test_select(sym);
+					break;
+				}
+				break;
+			case S_STRING:
+			case S_HEX:
+			case S_INT:
+				break;
+			}
+		}
+
+	next:
+		if (menu->list) {
+			menu = menu->list;
+			continue;
+		}
+		if (menu->next)
+			menu = menu->next;
+		else while ((menu = menu->parent)) {
+			if (menu->next) {
+				menu = menu->next;
+				break;
+			}
+		}
+	}
+
+	embedded->curr.tri = embed_tri;
+}
+
 int conf_read(const char *name)
 {
 	struct symbol *sym, *choice_sym;
@@ -386,6 +507,7 @@  int conf_read(const char *name)
 
 	sym_add_change_count(conf_warnings || conf_unsaved);
 
+	test_conf();
 	return 0;
 }
 
@@ -550,6 +672,8 @@  int conf_write(const char *name)
 
 	sym_set_change_count(0);
 
+	test_conf();
+
 	return 0;
 }