| Message ID | 20130717211649.GA5619@merkur.ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 17, 2013 at 11:16:49PM +0200, Sam Ravnborg wrote: > + > +static void exec_command(const char *command, struct symbol *sym) > +{ > + char buffer[2048]; > + FILE *stream; Just some indentation level saving: > + > + stream = popen(command, "r"); > + > + if (stream != NULL) { if (!stream) { menu_warn(current_entry, "command '%s' failed to execute", command); return; } and the rest starts one level less to the right: if (fgets(buffer, sizeof(buffer), stream) != NULL) { int i; buffer[sizeof(buffer) - 1] = '\0'; and so on... > + > + /* Drop any trialing newlines */ > + i = strlen(buffer); > + while (i > 0 && buffer[i - 1] == '\n') { > + buffer[i - 1] = '\0'; > + i--; > + } > + /* Validate the output of the command */ > + if (strlen(buffer) == 0) { > + menu_warn(current_entry, > + "command '%s' - invalid (empty?) return value: \"%s\"", > + command, buffer); > + return; > + } > + > + menu_warn(current_entry, "default: %s", buffer); > + sym_add_default(sym, buffer); > + } else { > + menu_warn(current_entry, "command '%s' - empty return value", command); > + } > + pclose(stream); > + } else { > + menu_warn(current_entry, "command '%s' failed to execute", command); > + } > +}
Sam, All, Well, while I was fighting on this on my side... ;-) On 2013-07-17 23:16 +0200, Sam Ravnborg spake thusly: > > We could extend the symbol option part to retreive values from a binary. > > Something like this: > > > > config FOOBAR > > bool > > option exec="true" > > > > FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n". > > And similar conversions for other types. > > > > This only extendt Kconfig slightly - using an already present method to import > > external values. > > > > The drawback I see with this approach is that we may execute a lot of small programs > > where the value is never used. > > Following is a quick patch implmenting this idea. > You need to run gperf manually to enable this. > > "gperf -C scripts/kconfig/zconf.gperf > scripts/kconfig/zconf.hash.c" > > I did not figure out how to use the built-in rules to generate this file :-( make REGENERATE_PARSERS=y menuconfig > I have tested this lightly - as we should discuss if this is a viable way forward. Instead of extending the Kconfig language, I was thinking (as initially suggested by Andrew) of generating a Kconfig file before all config targets, and source that Kconfig file from $(TOPDIR)/Kconfig. I'm not a fan od extending the Kconfig language in this way. It feels weird to me. Especially when we have a way to solve this specific issue with a script that generates the needed Kconfig symbols before any config target is called. At the very least we'd have to decide if this is available only for booleans/tristates, or for any type? Your implementation seems to make it valid for strings/ints, too. > For now I used popen() - so return value of the executed program are ignored. Yet, pclose returns the exit status of the command. If option exec was to be valid only for booleans, we could use the exit value rather than the output, which would seem more logical. It would be however a bit moretriclky for tristates, though... Maybe using output is the best, after all... So, how do you expect we use that to check for an external tool? Something like this? config HAVE_LZ4 bool option exec="which lz4c >/dev/null 2>&1 && echo y" config KERNEL_LZ4 bool "compress the kernel with lz4" depends on HAVE_LZ4 Ot did I miss something? > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c > index d550300..b7179a6 100644 > --- a/scripts/kconfig/symbol.c > +++ b/scripts/kconfig/symbol.c [--SNIP--] > @@ -1379,3 +1381,56 @@ static void prop_add_env(const char *env) > else > menu_warn(current_entry, "environment variable %s undefined", env); > } > + > +static void exec_command(const char *command, struct symbol *sym) > +{ > + char buffer[2048]; > + FILE *stream; > + > + stream = popen(command, "r"); > + > + if (stream != NULL) { > + if (fgets(buffer, sizeof(buffer), stream) != NULL) { > + int i; > + > + buffer[sizeof(buffer) - 1] = '\0'; > + > + /* Drop any trialing newlines */ > + i = strlen(buffer); > + while (i > 0 && buffer[i - 1] == '\n') { > + buffer[i - 1] = '\0'; > + i--; > + } > + /* Validate the output of the command */ > + if (strlen(buffer) == 0) { > + menu_warn(current_entry, > + "command '%s' - invalid (empty?) return value: \"%s\"", > + command, buffer); > + return; Missing pclose() before return. [--SNIP--] I'll give it a spin here. Thank you! BTW, I still need your help on solving that one: randconfig does randomise choice entries, unless KCONFIG_ALLCONFIG is specified. http://marc.info/?l=linux-kernel&m=137209757414433&w=2 My two previous attempts failed due to introducing regressions, so they were both reverted... :-( Any suggestion? ;-) Regards, Yann E. MORIN.
On 07/17/2013 03:30 PM, Yann E. MORIN wrote: > > At the very least we'd have to decide if this is available only for > booleans/tristates, or for any type? Your implementation seems to make > it valid for strings/ints, too. > I feel the test should *export* a boolean. It is worth noting that one of the things that keep getting brought up as something that need this is gcc/binutils support for a specific feature. -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 Thu, Jul 18, 2013 at 12:30 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > On 2013-07-17 23:16 +0200, Sam Ravnborg spake thusly: >> > We could extend the symbol option part to retreive values from a binary. >> > Something like this: >> > >> > config FOOBAR >> > bool >> > option exec="true" >> > >> > FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n". >> > And similar conversions for other types. >> > >> > This only extendt Kconfig slightly - using an already present method to import >> > external values. >> > >> > The drawback I see with this approach is that we may execute a lot of small programs >> > where the value is never used. >> >> Following is a quick patch implmenting this idea. >> You need to run gperf manually to enable this. >> >> "gperf -C scripts/kconfig/zconf.gperf > scripts/kconfig/zconf.hash.c" >> >> I did not figure out how to use the built-in rules to generate this file :-( > > make REGENERATE_PARSERS=y menuconfig > >> I have tested this lightly - as we should discuss if this is a viable way forward. > > Instead of extending the Kconfig language, I was thinking (as initially > suggested by Andrew) of generating a Kconfig file before all config > targets, and source that Kconfig file from $(TOPDIR)/Kconfig. I also prefer the generated Kconfig file. It keeps all these checks in a single place, instead of spreading it over all Kconfig files. This allows to keep better control over the list of checks, and notice when it gets out-of-hand. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 Thu, 18 Jul 2013 09:22:58 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Jul 18, 2013 at 12:30 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > On 2013-07-17 23:16 +0200, Sam Ravnborg spake thusly: > >> > We could extend the symbol option part to retreive values from a binary. > >> > Something like this: > >> > > >> > config FOOBAR > >> > bool > >> > option exec="true" > >> > > >> > FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n". > >> > And similar conversions for other types. > >> > > >> > This only extendt Kconfig slightly - using an already present method to import > >> > external values. > >> > > >> > The drawback I see with this approach is that we may execute a lot of small programs > >> > where the value is never used. > >> > >> Following is a quick patch implmenting this idea. > >> You need to run gperf manually to enable this. > >> > >> "gperf -C scripts/kconfig/zconf.gperf > scripts/kconfig/zconf.hash.c" > >> > >> I did not figure out how to use the built-in rules to generate this file :-( > > > > make REGENERATE_PARSERS=y menuconfig > > > >> I have tested this lightly - as we should discuss if this is a viable way forward. > > > > Instead of extending the Kconfig language, I was thinking (as initially > > suggested by Andrew) of generating a Kconfig file before all config > > targets, and source that Kconfig file from $(TOPDIR)/Kconfig. > > I also prefer the generated Kconfig file. > It keeps all these checks in a single place, instead of spreading it over all > Kconfig files. This allows to keep better control over the list of checks, and > notice when it gets out-of-hand. I prefer the "option exec" approach, actually. That way the shell-outs are colocated with the code which uses them and they will only be executed if you've actually selected that subsystem for building (I think?). -- 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
Andrew, All, On Thursday 18 July 2013 09:34:08 Andrew Morton wrote: > On Thu, 18 Jul 2013 09:22:58 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Jul 18, 2013 at 12:30 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > On 2013-07-17 23:16 +0200, Sam Ravnborg spake thusly: > > >> > We could extend the symbol option part to retreive values from a binary. > > >> > Something like this: > > >> > > > >> > config FOOBAR > > >> > bool > > >> > option exec="true" > > >> > > > >> > FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n". > > >> > And similar conversions for other types. > > >> > > > >> > This only extendt Kconfig slightly - using an already present method to import > > >> > external values. > > >> Following is a quick patch implmenting this idea. > > >> You need to run gperf manually to enable this. > > >> > > >> "gperf -C scripts/kconfig/zconf.gperf > scripts/kconfig/zconf.hash.c" > > >> > > >> I did not figure out how to use the built-in rules to generate this file :-( > > > > > > make REGENERATE_PARSERS=y menuconfig > > > > > >> I have tested this lightly - as we should discuss if this is a viable way forward. > > > > > > Instead of extending the Kconfig language, I was thinking (as initially > > > suggested by Andrew) of generating a Kconfig file before all config > > > targets, and source that Kconfig file from $(TOPDIR)/Kconfig. > > > > I also prefer the generated Kconfig file. > > It keeps all these checks in a single place, instead of spreading it over all > > Kconfig files. This allows to keep better control over the list of checks, and > > notice when it gets out-of-hand. > > I prefer the "option exec" approach, actually. That way the shell-outs > are colocated with the code which uses Indeed, but in this case, all the checks will be spread-out in the Kconfig files, and not easily locatable. Having all in a single script will also more easily raise eyebrows when that script appears in a diffstat. Noticing the 'exec' option risks being a bit less easy. But, that's not my call to decide. ;-) > them and they will only be executed > if you've actually selected that subsystem for building (I think?). If I understand the code correctly (which is still to be proven), the exec option is run at parse-time, once. Regards, Yann E. MORIN.
On Thu, 18 Jul 2013 09:47:02 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > > I prefer the "option exec" approach, actually. That way the shell-outs > > are colocated with the code which uses > > Indeed, but in this case, all the checks will be spread-out in the Kconfig > files, and not easily locatable. Having all in a single script will also > more easily raise eyebrows when that script appears in a diffstat. Noticing > the 'exec' option risks being a bit less easy. Eh, there are a million ways to screw up the kernel. > But, that's not my call to decide. ;-) > > > them and they will only be executed > > if you've actually selected that subsystem for building (I think?). > > If I understand the code correctly (which is still to be proven), the > exec option is run at parse-time, once. Yes, but not every Kconfig file in the tree gets parsed every time. For example, if arch/arm/Kconfig uses "option exec=some-slow-thing", x86 users won't execute some-slow-thing. This is good. -- 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 Thu, Jul 18, 2013 at 1:22 AM, H. Peter Anvin <hpa@zytor.com> wrote: > It is worth noting that one of the things that keep getting brought up > as something that need this is gcc/binutils support for a specific feature. Yeah, then those tests would be run only once, at kconfig time. I guess we still run some of them many times, due to the use of recursive make? Note that these will end up in your .config --- which is not necesarily a bad thing --- but it does mean that if e.g. you sent me your .config, several options may change when I try to use it. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 Thu, Jul 18, 2013 at 12:30:33AM +0200, Yann E. MORIN wrote: > Sam, All, > > Well, while I was fighting on this on my side... ;-) > > On 2013-07-17 23:16 +0200, Sam Ravnborg spake thusly: > > > We could extend the symbol option part to retreive values from a binary. > > > Something like this: > > > > > > config FOOBAR > > > bool > > > option exec="true" > > > > > > FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n". > > > And similar conversions for other types. > > > > > > This only extendt Kconfig slightly - using an already present method to import > > > external values. > > > > > > The drawback I see with this approach is that we may execute a lot of small programs > > > where the value is never used. > > > > Following is a quick patch implmenting this idea. > > You need to run gperf manually to enable this. > > > > "gperf -C scripts/kconfig/zconf.gperf > scripts/kconfig/zconf.hash.c" > > > > I did not figure out how to use the built-in rules to generate this file :-( > > make REGENERATE_PARSERS=y menuconfig Thanks! > > > I have tested this lightly - as we should discuss if this is a viable way forward. > > Instead of extending the Kconfig language, I was thinking (as initially > suggested by Andrew) of generating a Kconfig file before all config > targets, and source that Kconfig file from $(TOPDIR)/Kconfig. > > I'm not a fan od extending the Kconfig language in this way. It feels > weird to me. Especially when we have a way to solve this specific issue > with a script that generates the needed Kconfig symbols before any > config target is called. The problem we want to solve is that we want some external state to influence the Kconfig symbols. In this case if we support lz4. In other cases if we support a specific gcc feature or whatever. The relevant external state may not be relevant in all cases. We may have something that is arm specific - and we do not want to waste time on this when we are building for sparc. We can consider two general cases. - Where the external state is static and independent on any other config symbols. lz4 is such a case. - Where the external state depends on a config symbol. As we specify CROSS_COMPILE as a config symbol this may influence if gcc support a specific feature or not. Both approaches suggested (generate Kconfig and exec option) supports that we only execute relevant stuff. When we generate Kconfig we can call arch specific scripts and for exec option we include the arch specific stuff in the arch specific Kconfig files. None of the approaches presented so far can handle the fact that a config symbol may influence the external state. But I think the exec option can be updated to support this which is why that is my preference. This extends the Kconfig language - but at least it is a very minor extension. Sam -- 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/expr.h b/scripts/kconfig/expr.h index df198a5..a68258b 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -133,6 +133,7 @@ enum prop_type { P_SELECT, /* select BAR */ P_RANGE, /* range 7..100 (for a symbol) */ P_ENV, /* value from environment variable */ + P_EXEC, /* value from executable (using symbol option) */ P_SYMBOL, /* where a symbol is defined */ }; diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index 09f4edf..371f73b 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -61,6 +61,7 @@ enum conf_def_mode { #define T_OPT_MODULES 1 #define T_OPT_DEFCONFIG_LIST 2 #define T_OPT_ENV 3 +#define T_OPT_EXEC 4 struct kconf_id { int name; diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c index 7e233a6..cfa7aaa 100644 --- a/scripts/kconfig/menu.c +++ b/scripts/kconfig/menu.c @@ -213,6 +213,9 @@ void menu_add_option(int token, char *arg) case T_OPT_ENV: prop_add_env(arg); break; + case T_OPT_EXEC: + prop_add_exec(arg); + break; } } diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c index d550300..b7179a6 100644 --- a/scripts/kconfig/symbol.c +++ b/scripts/kconfig/symbol.c @@ -1331,6 +1331,8 @@ const char *prop_get_type_name(enum prop_type type) return "prompt"; case P_ENV: return "env"; + case P_EXEC: + return "exec"; case P_COMMENT: return "comment"; case P_MENU: @@ -1379,3 +1381,56 @@ static void prop_add_env(const char *env) else menu_warn(current_entry, "environment variable %s undefined", env); } + +static void exec_command(const char *command, struct symbol *sym) +{ + char buffer[2048]; + FILE *stream; + + stream = popen(command, "r"); + + if (stream != NULL) { + if (fgets(buffer, sizeof(buffer), stream) != NULL) { + int i; + + buffer[sizeof(buffer) - 1] = '\0'; + + /* Drop any trialing newlines */ + i = strlen(buffer); + while (i > 0 && buffer[i - 1] == '\n') { + buffer[i - 1] = '\0'; + i--; + } + /* Validate the output of the command */ + if (strlen(buffer) == 0) { + menu_warn(current_entry, + "command '%s' - invalid (empty?) return value: \"%s\"", + command, buffer); + return; + } + + menu_warn(current_entry, "default: %s", buffer); + sym_add_default(sym, buffer); + } else { + menu_warn(current_entry, "command '%s' - empty return value", command); + } + pclose(stream); + } else { + menu_warn(current_entry, "command '%s' failed to execute", command); + } +} + +static void prop_add_exec(const char *command) +{ + struct property *prop; + struct symbol *sym; + + sym = current_entry->sym; + sym->flags |= SYMBOL_AUTO; + + prop = prop_alloc(P_EXEC, sym); + prop->expr = expr_alloc_symbol(sym_lookup(command, SYMBOL_CONST)); + + exec_command(command, sym); +} + diff --git a/scripts/kconfig/zconf.gperf b/scripts/kconfig/zconf.gperf index f14ab41..02c6a59 100644 --- a/scripts/kconfig/zconf.gperf +++ b/scripts/kconfig/zconf.gperf @@ -44,4 +44,5 @@ on, T_ON, TF_PARAM modules, T_OPT_MODULES, TF_OPTION defconfig_list, T_OPT_DEFCONFIG_LIST,TF_OPTION env, T_OPT_ENV, TF_OPTION +exec, T_OPT_EXEC, TF_OPTION %%