[-next,2/2] kbuild: fix for updated LZ4 tool with the new streaming format
diff mbox

Message ID 20130717211649.GA5619@merkur.ravnborg.org
State New, archived
Headers show

Commit Message

Sam Ravnborg July 17, 2013, 9:16 p.m. UTC
> 
> 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 :-(

I have tested this lightly - as we should discuss if this is a viable way forward.
For now I used popen() - so return value of the executed program are ignored.

To test this I used:

config FOOBAR
	bool
	option exec="echo y"

config FOOBAR_SELECTOR
	bool
	default FOOBAR

Nothing fancy - but it shows the idea.

	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

Comments

Borislav Petkov July 17, 2013, 9:44 p.m. UTC | #1
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);
> +	}
> +}
Yann E. MORIN July 17, 2013, 10:30 p.m. UTC | #2
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.
H. Peter Anvin July 17, 2013, 11:22 p.m. UTC | #3
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
Geert Uytterhoeven July 18, 2013, 7:22 a.m. UTC | #4
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
Andrew Morton July 18, 2013, 7:34 a.m. UTC | #5
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
Yann E. MORIN July 18, 2013, 7:47 a.m. UTC | #6
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.
Andrew Morton July 18, 2013, 7:52 a.m. UTC | #7
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
Geert Uytterhoeven July 18, 2013, 8:02 a.m. UTC | #8
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
Sam Ravnborg July 18, 2013, 8:47 p.m. UTC | #9
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

Patch
diff mbox

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