* [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() [not found] <1312067499.22074.59.camel@i7.infradead.org> @ 2011-07-30 23:13 ` David Woodhouse 2011-07-31 2:17 ` Arnaud Lacombe 2011-07-30 23:14 ` [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' David Woodhouse 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-07-30 23:13 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild We're going to want to do this from elsewhere, shortly... Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- scripts/kconfig/confdata.c | 42 +++++++++++++++++++++++------------------- 1 files changed, 23 insertions(+), 19 deletions(-) diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 2bafd9a..a518ab3 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -179,6 +179,27 @@ static int conf_set_sym_val(struct symbol *sym, int def, int def_flags, char *p) return 0; } +static void conf_validate_choice_val(struct symbol *sym, int def, int def_flags) +{ + struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym)); + switch (sym->def[def].tri) { + case no: + break; + 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; + break; + } + cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri); +} + int conf_read_simple(const char *name, int def) { FILE *in = NULL; @@ -311,25 +332,8 @@ load: continue; } setsym: - if (sym && sym_is_choice_value(sym)) { - struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym)); - switch (sym->def[def].tri) { - case no: - break; - 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; - break; - } - cs->def[def].tri = EXPR_OR(cs->def[def].tri, sym->def[def].tri); - } + if (sym && sym_is_choice_value(sym)) + conf_validate_choice_val(sym, def, def_flags); } fclose(in); -- 1.7.6 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() 2011-07-30 23:13 ` [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() David Woodhouse @ 2011-07-31 2:17 ` Arnaud Lacombe 2011-07-31 22:21 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-31 2:17 UTC (permalink / raw) To: David Woodhouse Cc: H. Peter Anvin, linux-kernel, linux-kbuild, Arnaud Lacombe Hi, On Sat, Jul 30, 2011 at 7:13 PM, David Woodhouse <dwmw2@infradead.org> wrote: > We're going to want to do this from elsewhere, shortly... > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > I would go a little further and apply the attached patch too, The two jump site to `setsym' do not meet the conditionnal and can be avoided. - Arnaud diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 0341254..74dcfb1 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -284,7 +284,7 @@ load: sym = sym_find(line + 2 + strlen(CONFIG_)); if (!sym) { sym_add_change_count(1); - goto setsym; + continue; } } else { sym = sym_lookup(line + 2 + strlen(CONFIG_), 0); @@ -318,7 +318,7 @@ load: sym = sym_find(line + strlen(CONFIG_)); if (!sym) { sym_add_change_count(1); - goto setsym; + continue; } } else { sym = sym_lookup(line + strlen(CONFIG_), 0); @@ -335,7 +335,7 @@ load: conf_warning("unexpected data"); continue; } -setsym: if (sym && sym_is_choice_value(sym)) conf_validate_choice_val(sym, def, def_flags); } ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() 2011-07-31 2:17 ` Arnaud Lacombe @ 2011-07-31 22:21 ` David Woodhouse 0 siblings, 0 replies; 50+ messages in thread From: David Woodhouse @ 2011-07-31 22:21 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild On Sat, 2011-07-30 at 22:17 -0400, Arnaud Lacombe wrote: > > I would go a little further and apply the attached patch too, The two jump > site to `setsym' do not meet the conditionnal and can be avoided. Makes sense. It also makes sense to do that as a separate patch; the one moving code around should *just* move code around. Want to let me have a Signed-off-by: for it, and I'll add it to git://git.infradead.org/users/dwmw2/x86merge.git Thanks. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' [not found] <1312067499.22074.59.camel@i7.infradead.org> 2011-07-30 23:13 ` [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() David Woodhouse @ 2011-07-30 23:14 ` David Woodhouse 2011-07-30 23:44 ` Arnaud Lacombe 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-07-30 23:14 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild This allows you to set (and clear) config options on the make command line, for all config targets. For example: make CONFIG_64BIT=n randconfig make CONFIG_64BIT=n allmodconfig make CONFIG_64BIT=y CONFIG_SATA_MV=y oldconfig Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- scripts/kconfig/conf.c | 7 ++++++- scripts/kconfig/confdata.c | 26 ++++++++++++++++++++++++++ scripts/kconfig/lkc.h | 1 + 3 files changed, 33 insertions(+), 1 deletions(-) diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 006ad81..2b91e3b 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -456,7 +456,7 @@ static struct option long_opts[] = { {NULL, 0, NULL, 0} }; -int main(int ac, char **av) +int main(int ac, char **av, char **ep) { int opt; const char *name; @@ -563,6 +563,11 @@ int main(int ac, char **av) break; } + for ( ; *ep; ep++) { + if (!strncmp(*ep, CONFIG_, strlen(CONFIG_))) + conf_set_symbol_from_env(*ep); + } + if (sync_kconfig) { if (conf_get_changed()) { name = getenv("KCONFIG_NOSILENTUPDATE"); diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index a518ab3..c64eb33 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -342,6 +342,32 @@ setsym: return 0; } +void conf_set_symbol_from_env(char *str) +{ + char *p = strchr(str, '='); + struct symbol *sym; + int def = S_DEF_USER; + int def_flags = SYMBOL_DEF << def; + + if (!p) + return; + + *p = 0; + sym = sym_find(str + strlen(CONFIG_)); + *p++ = '='; + + if (!sym) + return; + + sym_calc_value(sym); + if (!sym_set_string_value(sym, p)) + return; + + conf_message(CONFIG_ "%s set to %s from environment", sym->name, p); + if (sym_is_choice_value(sym)) + conf_validate_choice_val(sym, def, def_flags); +} + int conf_read(const char *name) { struct symbol *sym, *choice_sym; diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index f34a0a9..fc2f3ad 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -89,6 +89,7 @@ char *conf_get_default_confname(void); void sym_set_change_count(int count); void sym_add_change_count(int count); void conf_set_all_new_symbols(enum conf_def_mode mode); +void conf_set_symbol_from_env(char *); /* confdata.c and expr.c */ static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out) -- 1.7.6 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-30 23:14 ` [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' David Woodhouse @ 2011-07-30 23:44 ` Arnaud Lacombe 2011-07-30 23:53 ` H. Peter Anvin 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-30 23:44 UTC (permalink / raw) To: David Woodhouse; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Sat, Jul 30, 2011 at 7:14 PM, David Woodhouse <dwmw2@infradead.org> wrote: > This allows you to set (and clear) config options on the make command > line, for all config targets. For example: > > make CONFIG_64BIT=n randconfig > make CONFIG_64BIT=n allmodconfig > make CONFIG_64BIT=y CONFIG_SATA_MV=y oldconfig > technically, no, this will not work as you intend. The following: make CONFIG_SATA_MV=y allnoconfig will fail to set CONFIG_SATA_MV=y. This is not a flaw in your patch, but a known issue with kconfig as even if you are setting CONFIG_SATA_MV=y, it's direct dependency are still unmet and thus the symbol in never considered to be output. The same flaw is present with the "allno.config" logic. Beside that, the one thing I dislike with your patch is that it is unconditional and global to all symbols, and you have no way to forbid the environment to override a value. What about: - a "select"-like approach, which would guarantee symbol selection and warn you when unmet-dependency are present - an opt-in approach to the symbol override from the environment - Arnaud > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > --- > scripts/kconfig/conf.c | 7 ++++++- > scripts/kconfig/confdata.c | 26 ++++++++++++++++++++++++++ > scripts/kconfig/lkc.h | 1 + > 3 files changed, 33 insertions(+), 1 deletions(-) > > diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c > index 006ad81..2b91e3b 100644 > --- a/scripts/kconfig/conf.c > +++ b/scripts/kconfig/conf.c > @@ -456,7 +456,7 @@ static struct option long_opts[] = { > {NULL, 0, NULL, 0} > }; > > -int main(int ac, char **av) > +int main(int ac, char **av, char **ep) > { > int opt; > const char *name; > @@ -563,6 +563,11 @@ int main(int ac, char **av) > break; > } > > + for ( ; *ep; ep++) { > + if (!strncmp(*ep, CONFIG_, strlen(CONFIG_))) > + conf_set_symbol_from_env(*ep); > + } > + > if (sync_kconfig) { > if (conf_get_changed()) { > name = getenv("KCONFIG_NOSILENTUPDATE"); > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index a518ab3..c64eb33 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -342,6 +342,32 @@ setsym: > return 0; > } > > +void conf_set_symbol_from_env(char *str) > +{ > + char *p = strchr(str, '='); > + struct symbol *sym; > + int def = S_DEF_USER; > + int def_flags = SYMBOL_DEF << def; > + > + if (!p) > + return; > + > + *p = 0; > + sym = sym_find(str + strlen(CONFIG_)); > + *p++ = '='; > + > + if (!sym) > + return; > + > + sym_calc_value(sym); > + if (!sym_set_string_value(sym, p)) > + return; > + > + conf_message(CONFIG_ "%s set to %s from environment", sym->name, p); > + if (sym_is_choice_value(sym)) > + conf_validate_choice_val(sym, def, def_flags); > +} > + > int conf_read(const char *name) > { > struct symbol *sym, *choice_sym; > diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h > index f34a0a9..fc2f3ad 100644 > --- a/scripts/kconfig/lkc.h > +++ b/scripts/kconfig/lkc.h > @@ -89,6 +89,7 @@ char *conf_get_default_confname(void); > void sym_set_change_count(int count); > void sym_add_change_count(int count); > void conf_set_all_new_symbols(enum conf_def_mode mode); > +void conf_set_symbol_from_env(char *); > > /* confdata.c and expr.c */ > static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out) > -- > 1.7.6 > > > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-30 23:44 ` Arnaud Lacombe @ 2011-07-30 23:53 ` H. Peter Anvin 2011-07-31 0:05 ` Arnaud Lacombe 0 siblings, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2011-07-30 23:53 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: David Woodhouse, linux-kernel, linux-kbuild On 07/30/2011 04:44 PM, Arnaud Lacombe wrote: > > Beside that, the one thing I dislike with your patch is that it is > unconditional and global to all symbols, and you have no way to forbid > the environment to override a value. > Why???? -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-30 23:53 ` H. Peter Anvin @ 2011-07-31 0:05 ` Arnaud Lacombe 2011-07-31 0:29 ` H. Peter Anvin 2011-08-09 14:14 ` Michal Marek 0 siblings, 2 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-31 0:05 UTC (permalink / raw) To: H. Peter Anvin; +Cc: David Woodhouse, linux-kernel, linux-kbuild Hi, On Sat, Jul 30, 2011 at 7:53 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 07/30/2011 04:44 PM, Arnaud Lacombe wrote: >> >> Beside that, the one thing I dislike with your patch is that it is >> unconditional and global to all symbols, and you have no way to forbid >> the environment to override a value. >> > > Why???? > Because kconfig might not be ran exclusively from a fully controlled and restricted environment ? Not to mention that it is used by other people than the linux kernel folks. - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 0:05 ` Arnaud Lacombe @ 2011-07-31 0:29 ` H. Peter Anvin 2011-07-31 1:06 ` Arnaud Lacombe 2011-08-09 14:14 ` Michal Marek 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2011-07-31 0:29 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: David Woodhouse, linux-kernel, linux-kbuild On 07/30/2011 05:05 PM, Arnaud Lacombe wrote: >> >> Why???? >> > Because kconfig might not be ran exclusively from a fully controlled > and restricted environment ? Not to mention that it is used by other > people than the linux kernel folks. > I'm sorry, but whitelisting specific options is an absolutely idiotic way to deal with that. The options use a specific namespace (CONFIG_*), and only allowing some options to be set on the command line, but not others, is a serious violation of the principle of least surprise. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 0:29 ` H. Peter Anvin @ 2011-07-31 1:06 ` Arnaud Lacombe 2011-07-31 1:28 ` H. Peter Anvin 2011-07-31 7:33 ` David Woodhouse 0 siblings, 2 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-31 1:06 UTC (permalink / raw) To: H. Peter Anvin; +Cc: David Woodhouse, linux-kernel, linux-kbuild, Roman Zippel Hi, [Added Roman Zippel to the Cc: list.] On Sat, Jul 30, 2011 at 8:29 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 07/30/2011 05:05 PM, Arnaud Lacombe wrote: >>> >>> Why???? >>> >> Because kconfig might not be ran exclusively from a fully controlled >> and restricted environment ? Not to mention that it is used by other >> people than the linux kernel folks. >> > > I'm sorry, but whitelisting specific options is an absolutely idiotic > way to deal with that. I'm sure the author of `option env=""' will appreciate that. I'd be interested to know if there was a reason to do it that way rather than allow the environment to override all symbols. > The options use a specific namespace (CONFIG_*), CONFIG_ is sure very specific namespace... > and only allowing some options to be set on the command line, but not > others, is a serious violation of the principle of least surprise. > The principle of least surprise is broken anyway as the proposed patch has absolutely no dependency checking and verification. You can `make CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. - Arnaud > -hpa > > -- > H. Peter Anvin, Intel Open Source Technology Center > I work for Intel. I don't speak on their behalf. > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 1:06 ` Arnaud Lacombe @ 2011-07-31 1:28 ` H. Peter Anvin 2011-07-31 2:09 ` Arnaud Lacombe 2011-07-31 7:33 ` David Woodhouse 1 sibling, 1 reply; 50+ messages in thread From: H. Peter Anvin @ 2011-07-31 1:28 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: David Woodhouse, linux-kernel, linux-kbuild, Roman Zippel On 07/30/2011 06:06 PM, Arnaud Lacombe wrote: >> > The principle of least surprise is broken anyway as the proposed patch > has absolutely no dependency checking and verification. You can `make > CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. > This gives you exactly the same way as a .config file or a ALLCONFIG file with the same options, which is what one realistically should expect. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 1:28 ` H. Peter Anvin @ 2011-07-31 2:09 ` Arnaud Lacombe 2011-07-31 5:21 ` Arnaud Lacombe 2011-07-31 22:18 ` David Woodhouse 0 siblings, 2 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-31 2:09 UTC (permalink / raw) To: H. Peter Anvin; +Cc: David Woodhouse, linux-kernel, linux-kbuild, Roman Zippel Hi, On Sat, Jul 30, 2011 at 9:28 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 07/30/2011 06:06 PM, Arnaud Lacombe wrote: >>> >> The principle of least surprise is broken anyway as the proposed patch >> has absolutely no dependency checking and verification. You can `make >> CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. >> > This gives you exactly the same way as a .config file or a ALLCONFIG > file with the same options, which is what one realistically should expect. > no, I would expect it to have the same effect as a `select CONFIG_SATA_MV', have it forcibly set to the given value, and be warned if there was any problem. Btw, `make CONFIG_GENERIC_BUG=n oldconfig' or `make CONFIG_64BIT=n oldconfig'[0] does not even work, and I'm getting no error. So either you make it work for all possible uses, or you warn the user he tried something illegal, but you don't just fail silently. From my point of view, this patch, as-is, open a delicate Pandora's box. - Arnaud [0]: which is still funny to do with CONFIG_X86_64=y :) ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 2:09 ` Arnaud Lacombe @ 2011-07-31 5:21 ` Arnaud Lacombe 2011-07-31 22:18 ` David Woodhouse 1 sibling, 0 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-07-31 5:21 UTC (permalink / raw) To: H. Peter Anvin; +Cc: David Woodhouse, linux-kernel, linux-kbuild, Roman Zippel Hi, On Sat, Jul 30, 2011 at 10:09 PM, Arnaud Lacombe <lacombar@gmail.com> wrote: > Hi, > > On Sat, Jul 30, 2011 at 9:28 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> On 07/30/2011 06:06 PM, Arnaud Lacombe wrote: >>>> >>> The principle of least surprise is broken anyway as the proposed patch >>> has absolutely no dependency checking and verification. You can `make >>> CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. >>> >> This gives you exactly the same way as a .config file or a ALLCONFIG >> file with the same options, which is what one realistically should expect. >> > no, I would expect it to have the same effect as a `select > CONFIG_SATA_MV', have it forcibly set to the given value, and be > warned if there was any problem. > > Btw, `make CONFIG_GENERIC_BUG=n oldconfig' or `make CONFIG_64BIT=n > oldconfig'[0] does not even work, and I'm getting no error. So either > you make it work for all possible uses, or you warn the user he tried > something illegal, but you don't just fail silently. > ok, the issue is that you will only be allowed to change visible symbols. CONFIG_64BIT is conditionally visible (when ARCH=x86), so right now, you can not do on x86-64: % make ARCH=i386 defconfig % make CONFIG_64BIT=n oldconfig # [0] and expect it to work. Instead of allowing explicitly a symbol to be overridden, the behavior is to implicitly, silently and un-deterministicaly[1] deny an override. Strange ... - Arnaud [0]: this match David's use-case described in <1311986969.20983.52.camel@i7.infradead.org> [1]: ie. it depends on the state of the tree when ran ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 2:09 ` Arnaud Lacombe 2011-07-31 5:21 ` Arnaud Lacombe @ 2011-07-31 22:18 ` David Woodhouse 2011-08-09 15:22 ` Arnaud Lacombe 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-07-31 22:18 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sat, 2011-07-30 at 22:09 -0400, Arnaud Lacombe wrote: > Btw, `make CONFIG_GENERIC_BUG=n oldconfig' or `make CONFIG_64BIT=n > oldconfig'[0] does not even work, and I'm getting no error. So either > you make it work for all possible uses, or you warn the user he tried > something illegal, but you don't just fail silently. You're absolutely right that we should report it. I'm dubious about trying to report a *reason*... it would be nice if we could do it *reliably*, but we don't actually pass the reasons back up so the code has to guess. I'm torn between ripping it out because it's guessing, and trying to improve it. What do you think? I'd *love* to be able to say 'You need to enable CONFIG_SATA in order to enable CONFIG_SATA_MV'. But if I were to do *that* then I'd probably end up ripping out the whole of this 'select' atrocity (which would now have no excuse for its existence) and I'd be even further from the simple task I started off with :) [dwmw2@i7 linux-2.6]$ make CONFIG_GENERIC_BUG=n oldconfig scripts/kconfig/conf --oldconfig Kconfig # # Could not set CONFIG_GENERIC_BUG=n; perhaps it is selected by another option? # # # configuration written to .config # [dwmw2@i7 linux-2.6]$ make CONFIG_SATA_MV=y oldconfig scripts/kconfig/conf --oldconfig Kconfig # # Could not set CONFIG_SATA_MV=y; perhaps it has unmet dependencies? # # # configuration written to .config # Incremental patch: diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 0341254..ff25669 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -364,8 +364,22 @@ void conf_set_symbol_from_env(char *str) return; sym_calc_value(sym); - if (!sym_set_string_value(sym, p)) + if (!sym_set_string_value(sym, p)) { + char *reason; + + if ((sym->type == S_BOOLEAN || sym->type == S_TRISTATE) && + !p[1] && toupper(p[0]) == 'N') + /* We were turning it *off* and failed... selected? */ + reason = "perhaps it is selected by another option?"; + else if (!sym->visible) + reason = "perhaps it has unmet dependencies?"; + else + reason = "perhaps your setting was invalid?"; + + conf_message("Could not set " CONFIG_ "%s=%s; %s", + sym->name, p, reason); return; + } conf_message(CONFIG_ "%s set to %s from environment", sym->name, p); if (sym_is_choice_value(sym)) -- dwmw2 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 22:18 ` David Woodhouse @ 2011-08-09 15:22 ` Arnaud Lacombe 0 siblings, 0 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-09 15:22 UTC (permalink / raw) To: David Woodhouse Cc: H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel, Michal Marek, Sam Ravnborg Hi, On Sun, Jul 31, 2011 at 6:18 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Sat, 2011-07-30 at 22:09 -0400, Arnaud Lacombe wrote: >> Btw, `make CONFIG_GENERIC_BUG=n oldconfig' or `make CONFIG_64BIT=n >> oldconfig'[0] does not even work, and I'm getting no error. So either >> you make it work for all possible uses, or you warn the user he tried >> something illegal, but you don't just fail silently. > > You're absolutely right that we should report it. I'm dubious about > trying to report a *reason*... it would be nice if we could do it > *reliably*, but we don't actually pass the reasons back up so the code > has to guess. I'm torn between ripping it out because it's guessing, and > trying to improve it. What do you think? > > I'd *love* to be able to say 'You need to enable CONFIG_SATA in order to > enable CONFIG_SATA_MV'. But if I were to do *that* then I'd probably end > up ripping out the whole of this 'select' atrocity (which would now have > no excuse for its existence) and I'd be even further from the simple > task I started off with :) > > [dwmw2@i7 linux-2.6]$ make CONFIG_GENERIC_BUG=n oldconfig > scripts/kconfig/conf --oldconfig Kconfig > # > # Could not set CONFIG_GENERIC_BUG=n; perhaps it is selected by another option? > # > # > # configuration written to .config > # > [dwmw2@i7 linux-2.6]$ make CONFIG_SATA_MV=y oldconfig > scripts/kconfig/conf --oldconfig Kconfig > # > # Could not set CONFIG_SATA_MV=y; perhaps it has unmet dependencies? > # > # > # configuration written to .config > # > > > Incremental patch: > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index 0341254..ff25669 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -364,8 +364,22 @@ void conf_set_symbol_from_env(char *str) > return; > > sym_calc_value(sym); > - if (!sym_set_string_value(sym, p)) > + if (!sym_set_string_value(sym, p)) { > + char *reason; > + > + if ((sym->type == S_BOOLEAN || sym->type == S_TRISTATE) && > + !p[1] && toupper(p[0]) == 'N') > + /* We were turning it *off* and failed... selected? */ > + reason = "perhaps it is selected by another option?"; > + else if (!sym->visible) > + reason = "perhaps it has unmet dependencies?"; > + else > + reason = "perhaps your setting was invalid?"; > + > + conf_message("Could not set " CONFIG_ "%s=%s; %s", > + sym->name, p, reason); > return; > + } > > conf_message(CONFIG_ "%s set to %s from environment", sym->name, p); > if (sym_is_choice_value(sym)) > You are still breaking dependencies. Currently every environment variable creates a dependency in auto.conf.cmd in order to force `auto.conf' regeneration if the environment change. Your patch lacks such a safety. cf commit 93449082e905ce73d0346d617dd67c4b668b58af Author: Roman Zippel <zippel@linux-m68k.org> Date: Mon Jan 14 04:50:54 2008 +0100 kconfig: environment symbol support for implementation details about how it is dealt with currently. Btw, Michal (or Sam), is it voluntary not to make `auto.conf' just depends on `.config' ? Thanks, - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 1:06 ` Arnaud Lacombe 2011-07-31 1:28 ` H. Peter Anvin @ 2011-07-31 7:33 ` David Woodhouse 2011-07-31 16:37 ` Randy Dunlap 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-07-31 7:33 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sat, 2011-07-30 at 21:06 -0400, Arnaud Lacombe wrote: > The principle of least surprise is broken anyway as the proposed patch > has absolutely no dependency checking and verification. You can `make > CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. That's always true in kconfig *anyway*. We've *never* really had an option for "do whatever you need to enable this option". We've even hard-coded this failure in our language, by introducing this horrible 'select' thing to work around it. I'd no more expect that, than I would for it to write the code for me if I type 'make CONFIG_BTRFSv2=y oldconfig'. So no, it doesn't violate the principle of least surprise. > ok, the issue is that you will only be allowed to change visible > symbols. CONFIG_64BIT is conditionally visible (when ARCH=x86), so > right now, you can not do on x86-64: > > % make ARCH=i386 defconfig > % make CONFIG_64BIT=n oldconfig # [0] That works fine here. What was ARCH set to in your second test? If it's ARCH=x86_64 then that's expected. That's the whole point of my *other* patch to make 'ARCH=x86' be the default, so that the value of CONFIG_64BIT in your .config is *not* forcibly overridden to match the build host. That's a *separate* bug, which I also have a patch for. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 7:33 ` David Woodhouse @ 2011-07-31 16:37 ` Randy Dunlap 2011-07-31 16:57 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Randy Dunlap @ 2011-07-31 16:37 UTC (permalink / raw) To: David Woodhouse Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sun, 31 Jul 2011 08:33:39 +0100 David Woodhouse wrote: > On Sat, 2011-07-30 at 21:06 -0400, Arnaud Lacombe wrote: > > The principle of least surprise is broken anyway as the proposed patch > > has absolutely no dependency checking and verification. You can `make > > CONFIG_SATA_MV=y allnoconfig', you will _not_ get it set. > > That's always true in kconfig *anyway*. We've *never* really had an > option for "do whatever you need to enable this option". We've even > hard-coded this failure in our language, by introducing this horrible > 'select' thing to work around it. > > I'd no more expect that, than I would for it to write the code for me if > I type 'make CONFIG_BTRFSv2=y oldconfig'. > > So no, it doesn't violate the principle of least surprise. > > > ok, the issue is that you will only be allowed to change visible > > symbols. CONFIG_64BIT is conditionally visible (when ARCH=x86), so > > right now, you can not do on x86-64: > > > > % make ARCH=i386 defconfig > > % make CONFIG_64BIT=n oldconfig # [0] > > That works fine here. What was ARCH set to in your second test? If it's > ARCH=x86_64 then that's expected. That's the whole point of my *other* > patch to make 'ARCH=x86' be the default, so that the value of > CONFIG_64BIT in your .config is *not* forcibly overridden to match the > build host. That's a *separate* bug, which I also have a patch for. Simple question: what does "ARCH=x86" mean? It doesn't mean anything to me without SUBARCH or nnBIT specified. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 16:37 ` Randy Dunlap @ 2011-07-31 16:57 ` David Woodhouse 2011-07-31 17:08 ` Randy Dunlap 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-07-31 16:57 UTC (permalink / raw) To: Randy Dunlap Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sun, 2011-07-31 at 09:37 -0700, Randy Dunlap wrote: > Simple question: what does "ARCH=x86" mean? > > It doesn't mean anything to me without SUBARCH or nnBIT specified. SUBARCH is meaningless for a native build; it's only for ARCH=um. So I don't know why that would make anything more meaningful to you. And why would CONFIG_64BIT make a difference either? Or conversely: why do CONFIG_PAE, CONFIG_LITTLE_ENDIAN, etc. *not* make a difference to your understanding? ARCH=x86 means exactly what it says: "build a kernel for the x86 architecture". Just like ARCH=mips means "build a kernel for MIPS" and ARCH=sparc means "build a kernel for SPARC", and ARCH=parisc means "build a kernel for PARISC", and ARCH=powerpc means "build a kernel for PowerPC". and ARCH=s390 means "build a kernel for S390". In *all* of those cases, CONFIG_64BIT is just one more configuration option; one of *many* that define what actual hardware the kernel supports. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 16:57 ` David Woodhouse @ 2011-07-31 17:08 ` Randy Dunlap 2011-07-31 17:40 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Randy Dunlap @ 2011-07-31 17:08 UTC (permalink / raw) To: David Woodhouse Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sun, 31 Jul 2011 17:57:46 +0100 David Woodhouse wrote: > On Sun, 2011-07-31 at 09:37 -0700, Randy Dunlap wrote: > > Simple question: what does "ARCH=x86" mean? > > > > It doesn't mean anything to me without SUBARCH or nnBIT specified. > > SUBARCH is meaningless for a native build; it's only for ARCH=um. So I > don't know why that would make anything more meaningful to you. > > And why would CONFIG_64BIT make a difference either? Or conversely: why > do CONFIG_PAE, CONFIG_LITTLE_ENDIAN, etc. *not* make a difference to > your understanding? > > ARCH=x86 means exactly what it says: "build a kernel for the x86 > architecture". > > Just like ARCH=mips means "build a kernel for MIPS" and ARCH=sparc means > "build a kernel for SPARC", and ARCH=parisc means "build a kernel for > PARISC", and ARCH=powerpc means "build a kernel for PowerPC". and > ARCH=s390 means "build a kernel for S390". > > In *all* of those cases, CONFIG_64BIT is just one more configuration > option; one of *many* that define what actual hardware the kernel > supports. OK, it seems that we agree that ARCH=x86 is an incomplete specification. Thanks. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 17:08 ` Randy Dunlap @ 2011-07-31 17:40 ` David Woodhouse 0 siblings, 0 replies; 50+ messages in thread From: David Woodhouse @ 2011-07-31 17:40 UTC (permalink / raw) To: Randy Dunlap Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild, Roman Zippel On Sun, 2011-07-31 at 10:08 -0700, Randy Dunlap wrote: > > OK, it seems that we agree that ARCH=x86 is an incomplete > specification. > Thanks. Of course it's incomplete, just as the legacy ARCH=i386 was incomplete. Even an allnoconfig still had over a hundred lines more configuration before it was a complete description of what gets built. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-07-31 0:05 ` Arnaud Lacombe 2011-07-31 0:29 ` H. Peter Anvin @ 2011-08-09 14:14 ` Michal Marek 2011-08-09 15:26 ` Arnaud Lacombe 1 sibling, 1 reply; 50+ messages in thread From: Michal Marek @ 2011-08-09 14:14 UTC (permalink / raw) To: Arnaud Lacombe Cc: H. Peter Anvin, David Woodhouse, linux-kernel, linux-kbuild On 31.7.2011 02:05, Arnaud Lacombe wrote: > Hi, > > On Sat, Jul 30, 2011 at 7:53 PM, H. Peter Anvin<hpa@zytor.com> wrote: >> On 07/30/2011 04:44 PM, Arnaud Lacombe wrote: >>> >>> Beside that, the one thing I dislike with your patch is that it is >>> unconditional and global to all symbols, and you have no way to forbid >>> the environment to override a value. >>> >> >> Why???? >> > Because kconfig might not be ran exclusively from a fully controlled > and restricted environment ? Not to mention that it is used by other > people than the linux kernel folks. Well, it has always been possible to trick kbuild (not kconfig) into accepting CONFIG_* options from environment, because unset kconfig options in auto.conf are not seen by make. Of course this is completely fragile, because there is no dependency checking and such variables are only seen by make and do not appear in autoconf.h. So a patch that teaches kconfig to read options from the environment would actually make some (albeit currently "illegal") use cases work correctly :). Michal ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-09 14:14 ` Michal Marek @ 2011-08-09 15:26 ` Arnaud Lacombe 2011-08-10 12:59 ` Michal Marek 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-09 15:26 UTC (permalink / raw) To: Michal Marek; +Cc: H. Peter Anvin, David Woodhouse, linux-kernel, linux-kbuild Hi, On Tue, Aug 9, 2011 at 10:14 AM, Michal Marek <mmarek@suse.cz> wrote: > On 31.7.2011 02:05, Arnaud Lacombe wrote: >> >> Hi, >> >> On Sat, Jul 30, 2011 at 7:53 PM, H. Peter Anvin<hpa@zytor.com> wrote: >>> >>> On 07/30/2011 04:44 PM, Arnaud Lacombe wrote: >>>> >>>> Beside that, the one thing I dislike with your patch is that it is >>>> unconditional and global to all symbols, and you have no way to forbid >>>> the environment to override a value. >>>> >>> >>> Why???? >>> >> Because kconfig might not be ran exclusively from a fully controlled >> and restricted environment ? Not to mention that it is used by other >> people than the linux kernel folks. > > Well, it has always been possible to trick kbuild (not kconfig) into > accepting CONFIG_* options from environment, because unset kconfig options > in auto.conf are not seen by make. Of course this is completely fragile, > because there is no dependency checking and such variables are only seen by > make and do not appear in autoconf.h. So a patch that teaches kconfig to > read options from the environment would actually make some (albeit currently > "illegal") use cases work correctly :). > kconfig can already set symbol value from the environment. The only limitation I can see is that it is not optional and require an explicit environment variable name. - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-09 15:26 ` Arnaud Lacombe @ 2011-08-10 12:59 ` Michal Marek 2011-08-10 13:07 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Michal Marek @ 2011-08-10 12:59 UTC (permalink / raw) To: Arnaud Lacombe Cc: H. Peter Anvin, David Woodhouse, linux-kernel, linux-kbuild On 9.8.2011 17:26, Arnaud Lacombe wrote: > On Tue, Aug 9, 2011 at 10:14 AM, Michal Marek <mmarek@suse.cz> wrote: >> On 31.7.2011 02:05, Arnaud Lacombe wrote: >>> Because kconfig might not be ran exclusively from a fully controlled >>> and restricted environment ? Not to mention that it is used by other >>> people than the linux kernel folks. >> >> Well, it has always been possible to trick kbuild (not kconfig) into >> accepting CONFIG_* options from environment, because unset kconfig options >> in auto.conf are not seen by make. Of course this is completely fragile, >> because there is no dependency checking and such variables are only seen by >> make and do not appear in autoconf.h. So a patch that teaches kconfig to >> read options from the environment would actually make some (albeit currently >> "illegal") use cases work correctly :). >> > kconfig can already set symbol value from the environment. The only > limitation I can see is that it is not optional and require an > explicit environment variable name. I wasn't talking about the env= syntax, but about make CONFIG_EXT2_FS=m all which makes kbuild visit fs/ext2 even if CONFIG_EXT2_FS is disabled in .config. With no update of the configuration or checking the dependencies. Hm, actually this would be a problem even if kconfig does read the CONFIG_* variables from the environment, because it could result in a mismatch if kconfig determines that the variable cannot be set, but make still sees it in the environment. So we would have to use 'undefine CONFIG_FOO' instead of '# CONFIG_FOO is not set' in include/config/auto.conf, to be able to properly support make CONFIG_FOO=y. Michal ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 12:59 ` Michal Marek @ 2011-08-10 13:07 ` David Woodhouse 2011-08-10 14:15 ` Arnaud Lacombe 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-10 13:07 UTC (permalink / raw) To: Michal Marek; +Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 14:59 +0200, Michal Marek wrote: > I wasn't talking about the env= syntax, but about > > make CONFIG_EXT2_FS=m all > which makes kbuild visit fs/ext2 even if CONFIG_EXT2_FS is disabled in > .config. With no update of the configuration or checking the dependencies. > > Hm, actually this would be a problem even if kconfig does read the > CONFIG_* variables from the environment, because it could result in a > mismatch if kconfig determines that the variable cannot be set, but make > still sees it in the environment. So we would have to use 'undefine > CONFIG_FOO' instead of '# CONFIG_FOO is not set' in > include/config/auto.conf, to be able to properly support make CONFIG_FOO=y. That's always been true; it's *always* been broken like that. But now we're actually *encouraging* people to start setting CONFIG_FOO=y in the environment or the make command line, so we should certainly make it more robust. It should be simple enough to force a reconfig if CONFIG_* is specified in the environment. Actually, while we're at it let's stop just picking it up from the environment and pick it up *only* if it's overridden in the make flags. Something like this will list the variables which are overridden on the command line: CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) Then we can make auto.conf.cmd trigger a reconfig if it's non-empty, and make the kconfig tool allow overrides *only* for those variables specified in $CONFIG_OVERRIDES... which avoids any worries about "stray" environment variables too. I'll see if I can clean the above expression up and do that. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 13:07 ` David Woodhouse @ 2011-08-10 14:15 ` Arnaud Lacombe 2011-08-10 14:17 ` David Woodhouse 2011-08-10 17:01 ` Randy Dunlap 0 siblings, 2 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-10 14:15 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Wed, Aug 10, 2011 at 9:07 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 14:59 +0200, Michal Marek wrote: >> I wasn't talking about the env= syntax, but about >> >> make CONFIG_EXT2_FS=m all >> which makes kbuild visit fs/ext2 even if CONFIG_EXT2_FS is disabled in >> .config. With no update of the configuration or checking the dependencies. >> >> Hm, actually this would be a problem even if kconfig does read the >> CONFIG_* variables from the environment, because it could result in a >> mismatch if kconfig determines that the variable cannot be set, but make >> still sees it in the environment. So we would have to use 'undefine >> CONFIG_FOO' instead of '# CONFIG_FOO is not set' in >> include/config/auto.conf, to be able to properly support make CONFIG_FOO=y. > > That's always been true; it's *always* been broken like that. > > But now we're actually *encouraging* people to start setting > CONFIG_FOO=y in the environment or the make command line, so we should > certainly make it more robust. > who is "we" ? > It should be simple enough to force a reconfig if CONFIG_* is specified > in the environment. > which is broken, the complete kernel Kconfig tree and all inter-dependency cannot be fully understood, especially by non-developer. So what you ask for is either not doing what the user wants, but respecting dependency, or doing what the user want, but not respecting dependency, and by the same, creating illegal configuration. > Actually, while we're at it let's stop just picking it up from the > environment and pick it up *only* if it's overridden in the make flags. > again, who's "we" ? > Something like this will list the variables which are overridden on the > command line: > > CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) > > Then we can make auto.conf.cmd trigger a reconfig if it's non-empty, and > make the kconfig tool allow overrides *only* for those variables > specified in $CONFIG_OVERRIDES... which avoids any worries about "stray" > environment variables too. > > I'll see if I can clean the above expression up and do that. > hum, let's take a real life example: user foo wants his config to enable CONFIG_WIRELESS_EXT, so with what you propose, he would do: # make CONFIG_WIRELESS_EXT=y allmodconfig currently, this would _never_ work, unless one of the symbol selected by `allmodconfig' selects it, as WIRELESS_EXT is defined the following: config WIRELESS_EXT bool I suspect there also an implicit dependency to WIRELESS. Now, you cannot just force WIRELESS_EXT to 'y', as there would be room for illegal configuration, or it might just never be built. Moreover, even if you did that, you would not just have to toggle the value, but to act as the 'select' do, ie. create a reverse dependency. If that was working, I would expect that you could do the opposite, that is: # make CONFIG_WIRELESS_EXT=n allyesconfig but again, behind implementation details, that might create an illegal configuration. There is a reason wireless chip select that symbol, that reason is only known by the wireless folks, so I do not see why the user should be allowed to go against. That said, if you want to hack around that, just edit net/wireless/Kconfig, that will be faster than trying to understand the whole thing. Btw, warning about potential missing dependencies are useless[0], user, even kernel hacker do _not_ read them. We spent a few day tracking down a build failure on powerpc triggered because SND_ISA was selected without ISA_DMA_API, a warning was present, but nobody cared about it. - Arnaud > -- > David Woodhouse Open Source Technology Centre > David.Woodhouse@intel.com Intel Corporation > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 14:15 ` Arnaud Lacombe @ 2011-08-10 14:17 ` David Woodhouse 2011-08-10 14:34 ` Arnaud Lacombe 2011-08-10 17:01 ` Randy Dunlap 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-10 14:17 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 10:15 -0400, Arnaud Lacombe wrote: > > hum, let's take a real life example: user foo wants his config to > enable CONFIG_WIRELESS_EXT, so with what you propose, he would do: > > # make CONFIG_WIRELESS_EXT=y allmodconfig > > currently, this would _never_ work, unless one of the symbol selected > by `allmodconfig' selects it, as WIRELESS_EXT is defined the > following: > > config WIRELESS_EXT > bool > > I suspect there also an implicit dependency to WIRELESS. This is a complete red herring. It's *always* been like that, and works *just* like this for the 'all.config' file etc. If you have nothing relevant to say, just don't say anything. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 14:17 ` David Woodhouse @ 2011-08-10 14:34 ` Arnaud Lacombe 2011-08-10 16:33 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-10 14:34 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, Aug 10, 2011 at 10:17 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 10:15 -0400, Arnaud Lacombe wrote: >> >> hum, let's take a real life example: user foo wants his config to >> enable CONFIG_WIRELESS_EXT, so with what you propose, he would do: >> >> # make CONFIG_WIRELESS_EXT=y allmodconfig >> >> currently, this would _never_ work, unless one of the symbol selected >> by `allmodconfig' selects it, as WIRELESS_EXT is defined the >> following: >> >> config WIRELESS_EXT >> bool >> >> I suspect there also an implicit dependency to WIRELESS. > > This is a complete red herring. It's *always* been like that, and works > *just* like this for the 'all.config' file etc. > your point being ? I might as well tell you that I find the current behavior of 'all*.config' just as broken wrt. to dependency management. > If you have nothing relevant to say, just don't say anything. > maybe you can come with a detailed description of your proposal's behavior, including how to manage case like this, instead of just throwing patch around ? If I do: # make CONFIG_WIRELESS_EXT=y allnoconfig I expect either a success or an error, not a silent discard. And *yes*, the problem already exists with "all*.config". Thanks, - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 14:34 ` Arnaud Lacombe @ 2011-08-10 16:33 ` David Woodhouse 2011-08-10 17:00 ` Emmanuel Deloget 2011-08-10 17:44 ` Arnaud Lacombe 0 siblings, 2 replies; 50+ messages in thread From: David Woodhouse @ 2011-08-10 16:33 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 10:34 -0400, Arnaud Lacombe wrote: > your point being ? I might as well tell you that I find the current > behavior of 'all*.config' just as broken wrt. to dependency > management. You might indeed. And I would find that commentary just as irrelevant and unhelpful. > > If you have nothing relevant to say, just don't say anything. > > > maybe you can come with a detailed description of your proposal's > behavior, including how to manage case like this, instead of just > throwing patch around ? How's this for a definition: "The behaviour for unsettable (or unclearable) options shall be exactly like it already is if you put them in all*.config, or if you manually edit the .config file and run 'make oldconfig', as people have been doing for years. There is nothing new to see here." > If I do: > > # make CONFIG_WIRELESS_EXT=y allnoconfig > > I expect either a success or an error, not a silent discard. And > *yes*, the problem already exists with "all*.config". [dwmw2@i7 linux-2.6]$ make CONFIG_WIRELESS_EXT=y allnoconfig scripts/kconfig/conf --allnoconfig Kconfig # # Could not set CONFIG_WIRELESS_EXT=y; perhaps it has unmet dependencies? # # # configuration written to .config # -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 16:33 ` David Woodhouse @ 2011-08-10 17:00 ` Emmanuel Deloget 2011-08-10 17:52 ` H. Peter Anvin 2011-08-10 17:44 ` Arnaud Lacombe 1 sibling, 1 reply; 50+ messages in thread From: Emmanuel Deloget @ 2011-08-10 17:00 UTC (permalink / raw) To: David Woodhouse Cc: Arnaud Lacombe, Michal Marek, H. Peter Anvin, linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org On 08/10/2011 06:33 PM, David Woodhouse wrote: > On Wed, 2011-08-10 at 10:34 -0400, Arnaud Lacombe wrote: >> your point being ? I might as well tell you that I find the current >> behavior of 'all*.config' just as broken wrt. to dependency >> management. > You might indeed. And I would find that commentary just as irrelevant > and unhelpful. > >>> If you have nothing relevant to say, just don't say anything. >>> >> maybe you can come with a detailed description of your proposal's >> behavior, including how to manage case like this, instead of just >> throwing patch around ? > How's this for a definition: > > "The behaviour for unsettable (or unclearable) options shall be exactly > like it already is if you put them in all*.config, or if you manually > edit the .config file and run 'make oldconfig', as people have been > doing for years. There is nothing new to see here." > >> If I do: >> >> # make CONFIG_WIRELESS_EXT=y allnoconfig >> >> I expect either a success or an error, not a silent discard. And >> *yes*, the problem already exists with "all*.config". > [dwmw2@i7 linux-2.6]$ make CONFIG_WIRELESS_EXT=y allnoconfig > scripts/kconfig/conf --allnoconfig Kconfig > # > # Could not set CONFIG_WIRELESS_EXT=y; perhaps it has unmet dependencies? > # > # > # configuration written to .config > # > I understand that my question is indeed neither wanted nor clever, but what's the point of trying to support "make CONFIG_FOO=y"? Will we be expected to type a 42 meters long command line to compile the kernel instead of doing a menuconfig in the foreseable future? (and between typos, unmet dependencies and the myriad of other possible errors, I'm not sure I'll get more free time). I don't get it. If the goal is to help the kernel hackers and if it really helps them it might be a thing to do - it might prove useful for very simple CONFIG_ options but I'm not sure this will stay true for the general case. Best regards, -- Emmanuel ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 17:00 ` Emmanuel Deloget @ 2011-08-10 17:52 ` H. Peter Anvin 0 siblings, 0 replies; 50+ messages in thread From: H. Peter Anvin @ 2011-08-10 17:52 UTC (permalink / raw) To: emmanuel.deloget Cc: David Woodhouse, Arnaud Lacombe, Michal Marek, linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org On 08/10/2011 12:00 PM, Emmanuel Deloget wrote: > > I understand that my question is indeed neither wanted nor clever, but > what's the point of trying to support "make CONFIG_FOO=y"? > > Will we be expected to type a 42 meters long command line to compile the > kernel instead of doing a menuconfig in the foreseable future? (and > between typos, unmet dependencies and the myriad of other possible > errors, I'm not sure I'll get more free time). > > I don't get it. If the goal is to help the kernel hackers and if it > really helps them it might be a thing to do - it might prove useful for > very simple CONFIG_ options but I'm not sure this will stay true for the > general case. > That's useful for some cases, though, instead of having to create a file; the driving cases here are architecture, platform or 32/64-bit selection. For longer ones you'll create a file. And no, menuconfig and .config files will not go away. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 16:33 ` David Woodhouse 2011-08-10 17:00 ` Emmanuel Deloget @ 2011-08-10 17:44 ` Arnaud Lacombe 2011-08-10 17:54 ` H. Peter Anvin 2011-08-10 17:59 ` David Woodhouse 1 sibling, 2 replies; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-10 17:44 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Wed, Aug 10, 2011 at 12:33 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 10:34 -0400, Arnaud Lacombe wrote: >> your point being ? I might as well tell you that I find the current >> behavior of 'all*.config' just as broken wrt. to dependency >> management. > > You might indeed. And I would find that commentary just as irrelevant > and unhelpful. > you are free to do so. >> > If you have nothing relevant to say, just don't say anything. >> > >> maybe you can come with a detailed description of your proposal's >> behavior, including how to manage case like this, instead of just >> throwing patch around ? > > How's this for a definition: > > "The behaviour for unsettable (or unclearable) options shall be exactly > like it already is if you put them in all*.config, or if you manually > edit the .config file and run 'make oldconfig', as people have been > doing for years. There is nothing new to see here." > Then I would say it is plain broken, especially if widespread. >> If I do: >> >> # make CONFIG_WIRELESS_EXT=y allnoconfig >> >> I expect either a success or an error, not a silent discard. And >> *yes*, the problem already exists with "all*.config". > > [dwmw2@i7 linux-2.6]$ make CONFIG_WIRELESS_EXT=y allnoconfig > scripts/kconfig/conf --allnoconfig Kconfig > # > # Could not set CONFIG_WIRELESS_EXT=y; perhaps it has unmet dependencies? > # > # > # configuration written to .config > # Exactly my point, you just successfully created an half-backed config which is different than what Aunt Tillie wanted you to generate. This should be an hard error, same for "all*.config", not to mention that the error message is far from being helpful. - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 17:44 ` Arnaud Lacombe @ 2011-08-10 17:54 ` H. Peter Anvin 2011-08-10 17:59 ` David Woodhouse 1 sibling, 0 replies; 50+ messages in thread From: H. Peter Anvin @ 2011-08-10 17:54 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: David Woodhouse, Michal Marek, linux-kernel, linux-kbuild On 08/10/2011 12:44 PM, Arnaud Lacombe wrote: > Exactly my point, you just successfully created an half-backed config > which is different than what Aunt Tillie wanted you to generate. This > should be an hard error, same for "all*.config", not to mention that > the error message is far from being helpful. Arnaud, This is arguably a bug in how config fragments are injected via any mechanism, but that is completely orthogonal to us wanting another injection mechanism. -hpa ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 17:44 ` Arnaud Lacombe 2011-08-10 17:54 ` H. Peter Anvin @ 2011-08-10 17:59 ` David Woodhouse 2011-08-10 18:40 ` Arnaud Lacombe 1 sibling, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-10 17:59 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 13:44 -0400, Arnaud Lacombe wrote: > Exactly my point, you just successfully created an half-backed config > which is different than what Aunt Tillie wanted you to generate. This > should be an hard error, same for "all*.config", not to mention that > the error message is far from being helpful. You are whining about something that has been true of the kernel config system for at least the last 16 years that I've been working on it, You're *right*, of course, but you're getting on my tits by whining about it only *now*, in this context. At least I have offered *an* error message reporting that the request was not honoured, which is a whole lot better that we've been used to. Please, if this offends you then by all means go and fix it. A sane way of handling dependencies would give a way to say "do what you need to do in order to enable CONFIG_SATA_MV", and should remove the abomination of 'select', which was introduced purely to work around that lack. But none of that is directly relevant in *this* thread. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 17:59 ` David Woodhouse @ 2011-08-10 18:40 ` Arnaud Lacombe 2011-08-10 18:52 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-10 18:40 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Wed, Aug 10, 2011 at 1:59 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 13:44 -0400, Arnaud Lacombe wrote: >> Exactly my point, you just successfully created an half-backed config >> which is different than what Aunt Tillie wanted you to generate. This >> should be an hard error, same for "all*.config", not to mention that >> the error message is far from being helpful. > > You are whining about something that has been true of the kernel config > system for at least the last 16 years that I've been working on it, > s/16/6/; the all.config logic has been added in: commit 90389160efc2864501ced6e662f9419eb7a3e6c8 Author: Roman Zippel <zippel@linux-m68k.org> Date: Tue Nov 8 21:34:49 2005 -0800 [PATCH] kconfig: preset config during all*config so, at best, this buggy behavior is ~ 6 years old. Before that, I'd assume that the internal namespace was not accessible by any other mean than the front-ends. > You're *right*, of course, but you're getting on my tits by whining > about it only *now*, in this context. well... sorry for your tits, 'hope you're enjoying it ;-) > At least I have offered *an* error > message reporting that the request was not honoured, which is a whole > lot better that we've been used to. > s/error/warning/, but indeed, that's a step forward. > Please, if this offends you then by all means go and fix it. A sane way > of handling dependencies would give a way to say "do what you need to do > in order to enable CONFIG_SATA_MV", and should remove the abomination of > 'select', which was introduced purely to work around that lack. > > But none of that is directly relevant in *this* thread. > to paraphrase you, I'd say, this might looks "cute but might give behavior that people will come to depend on in their scripts and then we take it away again", "that's why I'd kind of like to see it done *once*, *properly*". - Arnaud > -- > dwmw2 > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 18:40 ` Arnaud Lacombe @ 2011-08-10 18:52 ` David Woodhouse 2011-08-10 22:33 ` Arnaud Lacombe 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-10 18:52 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 14:40 -0400, Arnaud Lacombe wrote: > so, at best, this buggy behavior is ~ 6 years old. Before that, I'd > assume that the internal namespace was not accessible by any other > mean than the front-ends. I've been editing .config or doing 's/.*CONFIG_FOO[= ].*/CONFIG_FOO=y/' on it for a decade before that. With all the same limitations as all.config, and my new CONFIG_FOO=y command line support. How do *you* quickly, from the command line, enable or disable a single option in an existing config? > > Please, if this offends you then by all means go and fix it. A sane way > > of handling dependencies would give a way to say "do what you need to do > > in order to enable CONFIG_SATA_MV", and should remove the abomination of > > 'select', which was introduced purely to work around that lack. > > > > But none of that is directly relevant in *this* thread. > > > to paraphrase you, I'd say, this might looks "cute but might give > behavior that people will come to depend on in their scripts and then > we take it away again", "that's why I'd kind of like to see it done > *once*, *properly*". That's a reasonable concern, but I think it's misplaced in this case. We're not enabling anything that we're later going to break. I can't see many people *depending* on the fact that 'make CONFIG_SATA_MV=y oldconfig' actually does *nothing* in some cases. When we later, hopefully, get proper dependency resolution, that will take something that *wasn't* working and make it work. For all.config and for the command line overrides (and in other places) at the same time. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 18:52 ` David Woodhouse @ 2011-08-10 22:33 ` Arnaud Lacombe 2011-08-10 23:16 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-10 22:33 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Wed, Aug 10, 2011 at 2:52 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 14:40 -0400, Arnaud Lacombe wrote: >> so, at best, this buggy behavior is ~ 6 years old. Before that, I'd >> assume that the internal namespace was not accessible by any other >> mean than the front-ends. > > I've been editing .config or doing 's/.*CONFIG_FOO[= ].*/CONFIG_FOO=y/' > on it for a decade before that. With all the same limitations as > all.config, and my new CONFIG_FOO=y command line support. > > How do *you* quickly, from the command line, enable or disable a single > option in an existing config? > >> > Please, if this offends you then by all means go and fix it. A sane way >> > of handling dependencies would give a way to say "do what you need to do >> > in order to enable CONFIG_SATA_MV", and should remove the abomination of >> > 'select', which was introduced purely to work around that lack. >> > >> > But none of that is directly relevant in *this* thread. >> > >> to paraphrase you, I'd say, this might looks "cute but might give >> behavior that people will come to depend on in their scripts and then >> we take it away again", "that's why I'd kind of like to see it done >> *once*, *properly*". > > That's a reasonable concern, but I think it's misplaced in this case. > We're not enabling anything that we're later going to break. I can't see > many people *depending* on the fact that 'make CONFIG_SATA_MV=y > oldconfig' actually does *nothing* in some cases. > you are wrong, you ends up with half-baked compile-time dependency, which break the build: % make allnoconfig drivers/ata/ do nothing, but works. % make CONFIG_SATA_MV=y allnoconfig drivers/ata/ tries to build `drivers/ata/sata_mv.o' and miserably fails. [tested on top of your patches] - Arnaud > When we later, hopefully, get proper dependency resolution, that will > take something that *wasn't* working and make it work. For all.config > and for the command line overrides (and in other places) at the same > time. > > -- > dwmw2 > > ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 22:33 ` Arnaud Lacombe @ 2011-08-10 23:16 ` David Woodhouse 2011-08-11 3:29 ` Arnaud Lacombe 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-10 23:16 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 18:33 -0400, Arnaud Lacombe wrote: > > We're not enabling anything that we're later going to break. I can't see > > many people *depending* on the fact that 'make CONFIG_SATA_MV=y > > oldconfig' actually does *nothing* in some cases. > > > you are wrong, you ends up with half-baked compile-time dependency, > which break the build: s/*nothing*/nothing useful/, for crying out loud. The point remains that we're not enabling anything which we're later going to break. Nobody is going to come to *depend* on this behaviour. And anyway, this behaviour exists even *before* my patches, as Michal pointed out in a far more helpful and constructive fashion in <CACqU3MU4wJb3ij6skod-ZiM+Q0OMTXNdbJ+qWjJW8VZNEP+x1g@mail.gmail.com> earlier today. It's simple enough to fix, too: diff --git a/Makefile b/Makefile index 1fc5172..6cc7f7b 100644 --- a/Makefile +++ b/Makefile @@ -474,6 +474,9 @@ ifeq ($(KBUILD_EXTMOD),) endif endif +# This could probably be simpler? +CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) + ifeq ($(mixed-targets),1) # =========================================================================== # We're called with mixed targets (*config and build targets). @@ -507,6 +510,10 @@ else # Build targets only - this includes vmlinux, arch specific targets, clean # targets and others. In general all targets except *config targets. +ifneq ($(CONFIG_OVERRIDES),) +$(error Cannot perform build targets with CONFIG symbols overridden) +endif + ifeq ($(KBUILD_EXTMOD),) # Additional helpers built in scripts/ # Carefully list dependencies so we do not try to build scripts twice -- dwmw2 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 23:16 ` David Woodhouse @ 2011-08-11 3:29 ` Arnaud Lacombe 2011-08-11 8:42 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-11 3:29 UTC (permalink / raw) To: David Woodhouse; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Wed, Aug 10, 2011 at 7:16 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 18:33 -0400, Arnaud Lacombe wrote: >> > We're not enabling anything that we're later going to break. I can't see >> > many people *depending* on the fact that 'make CONFIG_SATA_MV=y >> > oldconfig' actually does *nothing* in some cases. >> > >> you are wrong, you ends up with half-baked compile-time dependency, >> which break the build: > > s/*nothing*/nothing useful/, for crying out loud. > > The point remains that we're not enabling anything which we're later > going to break. Nobody is going to come to *depend* on this behaviour. > > And anyway, this behaviour exists even *before* my patches, as Michal > pointed out in a far more helpful and constructive fashion in > <CACqU3MU4wJb3ij6skod-ZiM+Q0OMTXNdbJ+qWjJW8VZNEP+x1g@mail.gmail.com> > earlier today. > > It's simple enough to fix, too: > > diff --git a/Makefile b/Makefile > index 1fc5172..6cc7f7b 100644 > --- a/Makefile > +++ b/Makefile > @@ -474,6 +474,9 @@ ifeq ($(KBUILD_EXTMOD),) > endif > endif > > +# This could probably be simpler? > +CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) > + > ifeq ($(mixed-targets),1) > # =========================================================================== > # We're called with mixed targets (*config and build targets). > @@ -507,6 +510,10 @@ else > # Build targets only - this includes vmlinux, arch specific targets, clean > # targets and others. In general all targets except *config targets. > > +ifneq ($(CONFIG_OVERRIDES),) > +$(error Cannot perform build targets with CONFIG symbols overridden) > +endif > + > ifeq ($(KBUILD_EXTMOD),) > # Additional helpers built in scripts/ > # Carefully list dependencies so we do not try to build scripts twice > how is that supposed to work with your other patches ? % vi mail.txt [strip all lines until 'diff --git a/Makefile b/Makefile'] % git apply mail.txt % make CONFIG_NET=y allyesconfig drivers/ata/ [...] scripts/kconfig/conf --allyesconfig Kconfig # # CONFIG_NET set to y from environment # # # configuration written to .config # Makefile:504: *** Cannot perform build targets with CONFIG symbols overridden. Stop. make: *** [prepare] Error 2 - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 3:29 ` Arnaud Lacombe @ 2011-08-11 8:42 ` David Woodhouse 2011-08-11 8:58 ` Michal Marek 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-11 8:42 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 23:29 -0400, Arnaud Lacombe wrote: > how is that supposed to work with your other patches ? You cannot perform build targets with CONFIG symbols overridden. > % make CONFIG_NET=y allyesconfig drivers/ata/ ^^^^^^^ override ^^^^^^^^^^ build target > Makefile:504: *** Cannot perform build targets with CONFIG symbols > overridden. Stop. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error message which told you that, if you'd been paying the slightest bit of attention rather than just making pointless noise. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 8:42 ` David Woodhouse @ 2011-08-11 8:58 ` Michal Marek 2011-08-11 11:10 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Michal Marek @ 2011-08-11 8:58 UTC (permalink / raw) To: David Woodhouse Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On 11.8.2011 10:42, David Woodhouse wrote: > On Wed, 2011-08-10 at 23:29 -0400, Arnaud Lacombe wrote: >> how is that supposed to work with your other patches ? > > You cannot perform build targets with CONFIG symbols overridden. > >> % make CONFIG_NET=y allyesconfig drivers/ata/ > ^^^^^^^ override ^^^^^^^^^^ build target > >> Makefile:504: *** Cannot perform build targets with CONFIG symbols >> overridden. Stop. > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error message which > told you that, if you'd been paying the slightest bit of attention > rather than just making pointless noise. I would much rather see include/config/auto.conf reset all configuration variables to match what kconfig computed. I.e. use 'undefine CONFIG_FOO' instead of '# CONFIG_FOO is not set'. Additionally print this for any CONFIG_* variable found in the environment, even if the symbol is not visible in the current configuration. The include/config/auto.conf file is only used internally, so it should be safe to change the format. Michal ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 8:58 ` Michal Marek @ 2011-08-11 11:10 ` David Woodhouse 2011-08-11 11:15 ` Andreas Schwab 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-11 11:10 UTC (permalink / raw) To: Michal Marek; +Cc: Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On Thu, 2011-08-11 at 10:58 +0200, Michal Marek wrote: > I would much rather see include/config/auto.conf reset all configuration > variables to match what kconfig computed. I.e. use 'undefine CONFIG_FOO' > instead of '# CONFIG_FOO is not set'. That would be cute, but I'm not sure how to undefine something set on the command line: $ cat > Makefile <<EOF undefine BAR foo: echo $(BAR) EOF $ make foo BAR=hh echo hh hh Arguably, if someone *does* try something like Arnaud's "make CONFIG_FOO_BAR=y oldconfig bzImage" .. and it *wasn't* able to set CONFIG_FOO_BAR, then the nicest behaviour would be to fail, rather than to attempt to build it. So perhaps we should clean up only those settings inherited from the environment, and still (as in the patch I sent earlier) refuse to allow build targets in conjunction with CONFIG overrides on the command line? -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 11:10 ` David Woodhouse @ 2011-08-11 11:15 ` Andreas Schwab 2011-08-11 11:40 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Andreas Schwab @ 2011-08-11 11:15 UTC (permalink / raw) To: David Woodhouse Cc: Michal Marek, Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild David Woodhouse <dwmw2@infradead.org> writes: > That would be cute, but I'm not sure how to undefine something set on > the command line: > > $ cat > Makefile <<EOF > undefine BAR override undefine BAR Andreas. -- Andreas Schwab, schwab@redhat.com GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84 5EC7 45C6 250E 6F00 984E "And now for something completely different." ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 11:15 ` Andreas Schwab @ 2011-08-11 11:40 ` David Woodhouse 2011-08-11 11:56 ` Michal Marek 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-11 11:40 UTC (permalink / raw) To: Andreas Schwab Cc: Michal Marek, Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: > > That would be cute, but I'm not sure how to undefine something set on > > the command line: > > > > $ cat > Makefile <<EOF > > undefine BAR > > override undefine BAR Thanks. So we don't really need to change the auto.conf syntax; we could just do: $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) But still I suspect we might *not* want that for options set on the command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. I'll look at making it work only if what's on the command line matches the output of kconfig, but I'm also relatively content with saying "no config overrides on the command line for build targets" -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 11:40 ` David Woodhouse @ 2011-08-11 11:56 ` Michal Marek 2011-08-11 13:20 ` David Woodhouse 2011-08-11 14:57 ` Arnaud Lacombe 0 siblings, 2 replies; 50+ messages in thread From: Michal Marek @ 2011-08-11 11:56 UTC (permalink / raw) To: David Woodhouse Cc: Andreas Schwab, Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On 11.8.2011 13:40, David Woodhouse wrote: > On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: >>> That would be cute, but I'm not sure how to undefine something set on >>> the command line: >>> >>> $ cat > Makefile <<EOF >>> undefine BAR >> >> override undefine BAR > > Thanks. So we don't really need to change the auto.conf syntax; we could > just do: > > $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) > > But still I suspect we might *not* want that for options set on the > command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to > say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. If you do $ echo 'CONFIG_FOO=y' >all.config $ make allnoconfig bzImage then it will also build bzImage even if kconfig wasn't able to set CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is sufficient, as long as CONFIG_FOO is consistently unset. Michal ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 11:56 ` Michal Marek @ 2011-08-11 13:20 ` David Woodhouse 2011-08-11 14:57 ` Arnaud Lacombe 1 sibling, 0 replies; 50+ messages in thread From: David Woodhouse @ 2011-08-11 13:20 UTC (permalink / raw) To: Michal Marek Cc: Andreas Schwab, Arnaud Lacombe, H. Peter Anvin, linux-kernel, linux-kbuild On Thu, 2011-08-11 at 13:56 +0200, Michal Marek wrote: > > If you do > $ echo 'CONFIG_FOO=y' >all.config > $ make allnoconfig bzImage > then it will also build bzImage even if kconfig wasn't able to set > CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is > sufficient, as long as CONFIG_FOO is consistently unset. The thing is, such a warning is likely to be lost in the scrollback. There's definitely something to be said for: [dwmw2@i7 linux-2.6]$ make CONFIG_SATA_MV=y bzImage Makefile:572: *** CONFIG_SATA_MV could not be set to "y" as requested. Stop. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 11:56 ` Michal Marek 2011-08-11 13:20 ` David Woodhouse @ 2011-08-11 14:57 ` Arnaud Lacombe 2011-08-11 15:07 ` David Woodhouse 1 sibling, 1 reply; 50+ messages in thread From: Arnaud Lacombe @ 2011-08-11 14:57 UTC (permalink / raw) To: Michal Marek Cc: David Woodhouse, Andreas Schwab, H. Peter Anvin, linux-kernel, linux-kbuild Hi, On Thu, Aug 11, 2011 at 7:56 AM, Michal Marek <mmarek@suse.cz> wrote: > On 11.8.2011 13:40, David Woodhouse wrote: >> On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: >>>> That would be cute, but I'm not sure how to undefine something set on >>>> the command line: >>>> >>>> $ cat > Makefile <<EOF >>>> undefine BAR >>> >>> override undefine BAR >> >> Thanks. So we don't really need to change the auto.conf syntax; we could >> just do: >> >> $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) >> >> But still I suspect we might *not* want that for options set on the >> command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to >> say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. > > If you do > $ echo 'CONFIG_FOO=y' >all.config > $ make allnoconfig bzImage > then it will also build bzImage even if kconfig wasn't able to set > CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is > sufficient, as long as CONFIG_FOO is consistently unset. > FWIW, this is the broken behavior I have been pointing all along... If kconfig hard failed on such case, we would not need such Kbuild black-magic. From my point of view, if environment override there should be, it should behave the same as the all.config logic and hard fail when an override has not been met. Code wise, this would translate as backend code path being the same. - Arnaud ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 14:57 ` Arnaud Lacombe @ 2011-08-11 15:07 ` David Woodhouse 2011-08-11 15:24 ` Michal Marek 0 siblings, 1 reply; 50+ messages in thread From: David Woodhouse @ 2011-08-11 15:07 UTC (permalink / raw) To: Arnaud Lacombe Cc: Michal Marek, Andreas Schwab, H. Peter Anvin, linux-kernel, linux-kbuild On Thu, 2011-08-11 at 10:57 -0400, Arnaud Lacombe wrote: > > FWIW, this is the broken behavior I have been pointing all along... > > If kconfig hard failed on such case, we would not need such Kbuild > black-magic. > > From my point of view, if environment override there should be, it > should behave the same as the all.config logic and hard fail when an > override has not been met. > Code wise, this would translate as backend code path being the same. The patches I have so far *do* behave the same as the all.config logic, because the back end code *is* fairly much the same. I was looking at the all.config logic when I wrote the patch to kconfig. It *doesn't* currently hard fail. But I'm more than happy to make it do so. I think you're right; that makes most sense. I've just been looking at ways to allow real build targets to proceed *only* if any config options specified on the command line *did* get honoured by kconfig. But that gets a bit messy since you also want to automatically trigger an 'oldconfig' run if anything was specified on the command line. So you end up with one automatic oldconfig run in a sub-make, then the *second* time around it when the supposedly identical submake is invoked for the real build target, it would have to behave differently. I'm much happier with automatically triggering a reconfig if options are given on the command line, and a hard fail if they can't be honoured. That means that 'make CONFIG_FOO=y bzImage' will work 'properly', which IMO is either to do as it was asked, or fail. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 15:07 ` David Woodhouse @ 2011-08-11 15:24 ` Michal Marek 2011-08-11 15:50 ` David Woodhouse 0 siblings, 1 reply; 50+ messages in thread From: Michal Marek @ 2011-08-11 15:24 UTC (permalink / raw) To: David Woodhouse Cc: Arnaud Lacombe, Andreas Schwab, H. Peter Anvin, linux-kernel, linux-kbuild On 11.8.2011 17:07, David Woodhouse wrote: > On Thu, 2011-08-11 at 10:57 -0400, Arnaud Lacombe wrote: >> >> FWIW, this is the broken behavior I have been pointing all along... >> >> If kconfig hard failed on such case, we would not need such Kbuild >> black-magic. >> >> From my point of view, if environment override there should be, it >> should behave the same as the all.config logic and hard fail when an >> override has not been met. >> Code wise, this would translate as backend code path being the same. > > > The patches I have so far *do* behave the same as the all.config logic, > because the back end code *is* fairly much the same. I was looking at > the all.config logic when I wrote the patch to kconfig. > > It *doesn't* currently hard fail. But I'm more than happy to make it do > so. I think you're right; that makes most sense. One of my use cases for all.config is $ cp .../older-working-config all.config $ make KCONFIG_ALLCONFIG=1 allmodconfig i.e. reuse what is possible from an older config and enable all new options. Surely it sometimes results in suboptimal configuration with unwanted debug options enabled, but most of the time it works. I won't be happy if you make it hard fail because of renamed or removed config options. Michal ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-11 15:24 ` Michal Marek @ 2011-08-11 15:50 ` David Woodhouse 0 siblings, 0 replies; 50+ messages in thread From: David Woodhouse @ 2011-08-11 15:50 UTC (permalink / raw) To: Michal Marek Cc: Arnaud Lacombe, Andreas Schwab, H. Peter Anvin, linux-kernel, linux-kbuild On Thu, 2011-08-11 at 17:24 +0200, Michal Marek wrote: > > One of my use cases for all.config is > > $ cp .../older-working-config all.config > $ make KCONFIG_ALLCONFIG=1 allmodconfig > > i.e. reuse what is possible from an older config and enable all new > options. Surely it sometimes results in suboptimal configuration with > unwanted debug options enabled, but most of the time it works. I won't > be happy if you make it hard fail because of renamed or removed config > options. On the other hand, 'make CONFIG_FOO=y oldconfig' probably *should* fail. Perhaps it should set what it *can*, if you try to set multiple options at once — but if it can't set all of them, it should exit with failure status... which will mean that no subsequent build target will happen. -- dwmw2 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 14:15 ` Arnaud Lacombe 2011-08-10 14:17 ` David Woodhouse @ 2011-08-10 17:01 ` Randy Dunlap 2011-08-10 17:25 ` David Woodhouse 1 sibling, 1 reply; 50+ messages in thread From: Randy Dunlap @ 2011-08-10 17:01 UTC (permalink / raw) To: Arnaud Lacombe Cc: David Woodhouse, Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 10 Aug 2011 10:15:55 -0400 Arnaud Lacombe wrote: > Btw, warning about potential missing dependencies are useless[0], > user, even kernel hacker do _not_ read them. We spent a few day > tracking down a build failure on powerpc triggered because SND_ISA was > selected without ISA_DMA_API, a warning was present, but nobody cared > about it. That one was an anomaly. I usually see them and often send patches for them. But granted, most developers just pass over them. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' 2011-08-10 17:01 ` Randy Dunlap @ 2011-08-10 17:25 ` David Woodhouse 0 siblings, 0 replies; 50+ messages in thread From: David Woodhouse @ 2011-08-10 17:25 UTC (permalink / raw) To: Randy Dunlap Cc: Arnaud Lacombe, Michal Marek, H. Peter Anvin, linux-kernel, linux-kbuild On Wed, 2011-08-10 at 10:01 -0700, Randy Dunlap wrote: > On Wed, 10 Aug 2011 10:15:55 -0400 Arnaud Lacombe wrote: > > > Btw, warning about potential missing dependencies are useless[0], > > user, even kernel hacker do _not_ read them. We spent a few day > > tracking down a build failure on powerpc triggered because SND_ISA was > > selected without ISA_DMA_API, a warning was present, but nobody cared > > about it. > > That one was an anomaly. I usually see them and often send patches > for them. But granted, most developers just pass over them. I see that kind of thing as a natural consequence of the whole 'select' abomination. But that's even *further* off-topic from the original topic of this thread than Arnaud has already dragged us. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2011-08-11 15:51 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1312067499.22074.59.camel@i7.infradead.org>
2011-07-30 23:13 ` [PATCH 1/2] kconfig: Factor out conf_validate_choice_val() function from conf_read_simple() David Woodhouse
2011-07-31 2:17 ` Arnaud Lacombe
2011-07-31 22:21 ` David Woodhouse
2011-07-30 23:14 ` [PATCH 2/2] Enable 'make CONFIG_FOO=y oldconfig' David Woodhouse
2011-07-30 23:44 ` Arnaud Lacombe
2011-07-30 23:53 ` H. Peter Anvin
2011-07-31 0:05 ` Arnaud Lacombe
2011-07-31 0:29 ` H. Peter Anvin
2011-07-31 1:06 ` Arnaud Lacombe
2011-07-31 1:28 ` H. Peter Anvin
2011-07-31 2:09 ` Arnaud Lacombe
2011-07-31 5:21 ` Arnaud Lacombe
2011-07-31 22:18 ` David Woodhouse
2011-08-09 15:22 ` Arnaud Lacombe
2011-07-31 7:33 ` David Woodhouse
2011-07-31 16:37 ` Randy Dunlap
2011-07-31 16:57 ` David Woodhouse
2011-07-31 17:08 ` Randy Dunlap
2011-07-31 17:40 ` David Woodhouse
2011-08-09 14:14 ` Michal Marek
2011-08-09 15:26 ` Arnaud Lacombe
2011-08-10 12:59 ` Michal Marek
2011-08-10 13:07 ` David Woodhouse
2011-08-10 14:15 ` Arnaud Lacombe
2011-08-10 14:17 ` David Woodhouse
2011-08-10 14:34 ` Arnaud Lacombe
2011-08-10 16:33 ` David Woodhouse
2011-08-10 17:00 ` Emmanuel Deloget
2011-08-10 17:52 ` H. Peter Anvin
2011-08-10 17:44 ` Arnaud Lacombe
2011-08-10 17:54 ` H. Peter Anvin
2011-08-10 17:59 ` David Woodhouse
2011-08-10 18:40 ` Arnaud Lacombe
2011-08-10 18:52 ` David Woodhouse
2011-08-10 22:33 ` Arnaud Lacombe
2011-08-10 23:16 ` David Woodhouse
2011-08-11 3:29 ` Arnaud Lacombe
2011-08-11 8:42 ` David Woodhouse
2011-08-11 8:58 ` Michal Marek
2011-08-11 11:10 ` David Woodhouse
2011-08-11 11:15 ` Andreas Schwab
2011-08-11 11:40 ` David Woodhouse
2011-08-11 11:56 ` Michal Marek
2011-08-11 13:20 ` David Woodhouse
2011-08-11 14:57 ` Arnaud Lacombe
2011-08-11 15:07 ` David Woodhouse
2011-08-11 15:24 ` Michal Marek
2011-08-11 15:50 ` David Woodhouse
2011-08-10 17:01 ` Randy Dunlap
2011-08-10 17:25 ` David Woodhouse
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox