| Message ID | 1312067670.22074.61.camel@i7.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > > > -- 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
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
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 -- 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
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
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. > > -- 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
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
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 :) -- 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
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 -- 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
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.
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 *** -- 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
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.
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 *** -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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.
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 > > -- 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
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.
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 -- 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
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 #
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 -- 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
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 *** -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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.
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 > > -- 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
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.
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 > > -- 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
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)
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(-)