[11/23] kconfig: add 'shell-stdout' function

Message ID 1518806331-7101-12-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series
  • kconfig: move compiler capability tests to Kconfig
Related show

Commit Message

Masahiro Yamada Feb. 16, 2018, 6:38 p.m.
This is the second built-in function, which retrieves the first line
of stdout from the given shell command.

Example code:

  config CC_IS_GCC
          bool
          default $(shell $CC --version | grep -q gcc)

  config GCC_VERSION
          int
          default $(shell-stdout $srctree/scripts/gcc-version.sh $CC | sed 's/^0*//') if CC_IS_GCC
          default 0

Result:

  $ make -s alldefconfig && tail -n 2 .config
  CONFIG_CC_IS_GCC=y
  CONFIG_GCC_VERSION=504

  $ make CC=clang -s alldefconfig && tail -n 2 .config
  # CONFIG_CC_IS_GCC is not set
  CONFIG_GCC_VERSION=0

By the way, function calls can be nested, so the following works.

Example code:

  config FOO
          bool
          default $(shell $(shell-stdout echo $COMMAND_IN_CAPITAL | tr [A-Z] [a-z]))

Result:
  $ make -s COMMAND=TRUE alldefconfig && tail -n 1 .config
  CONFIG_FOO=y
  $ make -s COMMAND=FALSE alldefconfig && tail -n 1 .config
  # CONFIG_FOO is not set

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 scripts/kconfig/function.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

-- 
2.7.4

Comments

Linus Torvalds Feb. 16, 2018, 7:17 p.m. | #1
On Fri, Feb 16, 2018 at 10:38 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> This is the second built-in function, which retrieves the first line

> of stdout from the given shell command.


This is the only part I really don't much like in your patch series.

Most of it is just lovely and looks very nice and powerful, but the
difference between "$(shell ..." and "$(shell-stdout ..." to me is
bvery ugly.

Can we *please* make "shell-stdout" go away, and make this just be "shell"?

The rule would be very simple:

 - if the result of the shell command is a failure, the result is 'n'

 - otherwise, the result is the first line of stdout

 - if the result is empty, we replace it with 'y'.

So doing $(shell true) would be 100% equivalent to $(shell echo y),
and you could still do that

          default $(shell $CC --version | grep -q gcc)

because it would just automatically do the right thing.

Basically, the only difference is how $(shell ) works in the success
case: the result won't necessarily be 'y', it will be whatever output.
But if you want to always turn it into 'y' (say, you don't have a "-q"
flag for the grep equivalent above), you can always do so with

          default $(shell $CC --version | noqgrep gcc > /dev/null)

So it seems to me that there is never any fundamental reason why we'd
want both "shell" and "shell-stdout", since "shell-stdout" is
fundamentally more powerful than "shell", and can always be used as
such (and just renamed to "shell").

Because I really think that it's just much prettier and more intuitive
to be able to say

        default "/boot/config-$(shell uname --release)"

without that "-stdout" thing.

Hmm?

But I do want to say how much I liked this series. Just this part
seemed to result in uglier Kconfig scripts.

         Linus
Ulf Magnusson Feb. 19, 2018, 4:48 a.m. | #2
On Fri, Feb 16, 2018 at 11:17:52AM -0800, Linus Torvalds wrote:
> On Fri, Feb 16, 2018 at 10:38 AM, Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

> > This is the second built-in function, which retrieves the first line

> > of stdout from the given shell command.

> 

> This is the only part I really don't much like in your patch series.

> 

> Most of it is just lovely and looks very nice and powerful, but the

> difference between "$(shell ..." and "$(shell-stdout ..." to me is

> bvery ugly.

> 

> Can we *please* make "shell-stdout" go away, and make this just be "shell"?

> 

> The rule would be very simple:

> 

>  - if the result of the shell command is a failure, the result is 'n'

> 

>  - otherwise, the result is the first line of stdout

> 

>  - if the result is empty, we replace it with 'y'.

> 

> So doing $(shell true) would be 100% equivalent to $(shell echo y),

> and you could still do that

> 

>           default $(shell $CC --version | grep -q gcc)

> 

> because it would just automatically do the right thing.

> 

> Basically, the only difference is how $(shell ) works in the success

> case: the result won't necessarily be 'y', it will be whatever output.

> But if you want to always turn it into 'y' (say, you don't have a "-q"

> flag for the grep equivalent above), you can always do so with

> 

>           default $(shell $CC --version | noqgrep gcc > /dev/null)

> 

> So it seems to me that there is never any fundamental reason why we'd

> want both "shell" and "shell-stdout", since "shell-stdout" is

> fundamentally more powerful than "shell", and can always be used as

> such (and just renamed to "shell").

> 

> Because I really think that it's just much prettier and more intuitive

> to be able to say

> 

>         default "/boot/config-$(shell uname --release)"

> 

> without that "-stdout" thing.

> 

> Hmm?

> 

> But I do want to say how much I liked this series. Just this part

> seemed to result in uglier Kconfig scripts.

> 

>          Linus


Could there be cases where you'd legitimately want to put empty output
from a command in a string (that would be common enough to matter)?
That'd get messier with the above rule, as it never generates an empty
string as output.

For an environment variable, stuff like prefixes come to mind, but I
can't think of anything for a command. I'm more familiar with Kconfig
than the rest of the kernel build system though.


Would you still be as opposed (or more opposed...) to having two
functions if they were called something like 'success' and 'stdout'
instead?

This reads pretty naturally to me:

	config CC_IS_GCC
		def_bool "$(success $CC --version | grep gcc)"

As does this:

	default "/boot/config-$(stdout uname --release)"

The rule for $(success ...) would be that it's textually replaced by "y"
if the exit status of the command is 0, and with "n" in all other cases.

$(stdout ...) would be textually replaced by the first line from stdout.
Maybe it'd be helpful to spit out a warning if the exit status is non-0.

All functions would just do dumb and simple-to-understand text
replacement, and all the interpretation would happen later in the normal
way. Enforcing the quotes would make this behavior obvious. That would
indirectly turn the expanded values into constant symbols internally in
Kconfig.

Thoughts?

Cheers,
Ulf
Linus Torvalds Feb. 19, 2018, 5:44 p.m. | #3
On Sun, Feb 18, 2018 at 8:48 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote:
> On Fri, Feb 16, 2018 at 11:17:52AM -0800, Linus Torvalds wrote:

>>

>> Can we *please* make "shell-stdout" go away, and make this just be "shell"?

>>

>> The rule would be very simple:

>>

>>  - if the result of the shell command is a failure, the result is 'n'

>>

>>  - otherwise, the result is the first line of stdout

>>

>>  - if the result is empty, we replace it with 'y'.

>

> Could there be cases where you'd legitimately want to put empty output

> from a command in a string (that would be common enough to matter)?

> That'd get messier with the above rule, as it never generates an empty

> string as output.


Hmm. Maybe. Something like "LOCALVERSION_AUTO" might want that where
you add a version string if something is true, and maybe you'd use a
shell script for it and generate it at Kconfig time.

I'm not seeing anything like that right now, but I could imagine it in
theory, so your worry is valid.

> Would you still be as opposed (or more opposed...) to having two

> functions if they were called something like 'success' and 'stdout'

> instead?


Maybe the naming is indeed what annoyed me the most.

I do like your "success"/"stdout" more than "shell"/"shell-stdout",
because with that naming I don't get the feeling that one should
subsume the other.

          Linus
Linus Torvalds Feb. 19, 2018, 6:01 p.m. | #4
On Mon, Feb 19, 2018 at 9:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>

> I do like your "success"/"stdout" more than "shell"/"shell-stdout",

> because with that naming I don't get the feeling that one should

> subsume the other.


Hmm. Thinking about it some more, I really would prefer just "$(shell
...)" everywhere.

But it would be nice if perhaps the error handling would match the
context somehow.

I'm wondering if this might tie into the whole quoting discussion in
the other thread.

Because the rule could be:

(a) unquoted $(shell ) is a bool, and failing is ok (and turns into
y/n depending on whether successful or failing)

So

  config CC_IS_GCC
          bool
          default $(shell $CC --version | grep -q gcc)

works automatically.

(b) but with quoting, $(shell ) is a string, and failing is an error

So

  config GCC_VERSION
          int
          default "$(shell-stdout $srctree/scripts/gcc-version.sh $CC
| sed 's/^0*//')" if CC_IS_GCC
          default 0

would need those quotes, and if the shell-script returns a failure,
we'd _abort_.

Which is actually what we want there.

Hmm? Is that too nasty?

                Linus
Ulf Magnusson Feb. 19, 2018, 6:54 p.m. | #5
On Mon, Feb 19, 2018 at 10:01:49AM -0800, Linus Torvalds wrote:
> On Mon, Feb 19, 2018 at 9:44 AM, Linus Torvalds

> <torvalds@linux-foundation.org> wrote:

> >

> > I do like your "success"/"stdout" more than "shell"/"shell-stdout",

> > because with that naming I don't get the feeling that one should

> > subsume the other.

> 

> Hmm. Thinking about it some more, I really would prefer just "$(shell

> ...)" everywhere.

> 

> But it would be nice if perhaps the error handling would match the

> context somehow.

> 

> I'm wondering if this might tie into the whole quoting discussion in

> the other thread.

> 

> Because the rule could be:

> 

> (a) unquoted $(shell ) is a bool, and failing is ok (and turns into

> y/n depending on whether successful or failing)

> 

> So

> 

>   config CC_IS_GCC

>           bool

>           default $(shell $CC --version | grep -q gcc)

> 

> works automatically.

> 

> (b) but with quoting, $(shell ) is a string, and failing is an error

> 

> So

> 

>   config GCC_VERSION

>           int

>           default "$(shell-stdout $srctree/scripts/gcc-version.sh $CC

> | sed 's/^0*//')" if CC_IS_GCC

>           default 0

> 

> would need those quotes, and if the shell-script returns a failure,

> we'd _abort_.

> 

> Which is actually what we want there.

> 

> Hmm? Is that too nasty?

> 

>                 Linus


One minor drawback would be slight kludginess if you want "n"/"y" put
into a string depending on the success of a command:

	default	"foo-$(shell cmd && echo y || echo n)"

As opposed to:

	default "foo-$(success cmd)"

I don't know if that's significant enough to matter in practice.

Keeping it objective, I can't see any major downsides, though I'd really
prefer to just have $() do string interpolation within "". That keeps
the implementation trivial and makes the behavior and limitations
obvious once you know that n/m/y is just shorthand for "n"/"m"/"y".

Cheers,
Ulf
Masahiro Yamada Feb. 21, 2018, 4:59 a.m. | #6
2018-02-20 3:01 GMT+09:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Mon, Feb 19, 2018 at 9:44 AM, Linus Torvalds

> <torvalds@linux-foundation.org> wrote:

>>

>> I do like your "success"/"stdout" more than "shell"/"shell-stdout",

>> because with that naming I don't get the feeling that one should

>> subsume the other.

>

> Hmm. Thinking about it some more, I really would prefer just "$(shell

> ...)" everywhere.

>

> But it would be nice if perhaps the error handling would match the

> context somehow.

>

> I'm wondering if this might tie into the whole quoting discussion in

> the other thread.

>

> Because the rule could be:

>

> (a) unquoted $(shell ) is a bool, and failing is ok (and turns into

> y/n depending on whether successful or failing)

>

> So

>

>   config CC_IS_GCC

>           bool

>           default $(shell $CC --version | grep -q gcc)

>

> works automatically.

>

> (b) but with quoting, $(shell ) is a string, and failing is an error

>

> So

>

>   config GCC_VERSION

>           int

>           default "$(shell-stdout $srctree/scripts/gcc-version.sh $CC

> | sed 's/^0*//')" if CC_IS_GCC

>           default 0

>

> would need those quotes, and if the shell-script returns a failure,

> we'd _abort_.



GCC_VERSION is int type.

Setting aside the Kconfig internal, I prefer 50700 to "50700"

According to my common sense, I do not want to quote integers.




IMO, I prefer to use different names for different purpose.
So, 'stdout' and 'success' look good to me.



BTW, I noticed just one built-in function is enough
because 'success' can be derived from 'stdout'.


So, my plan is, implement $(shell ...) as a built-in function.
This returns the stdout from the command.


Then, implement 'success' as a textual shorthand
by using macro, like this:

macro success $(shell ($(1) && echo y) || echo n)


macro can be expanded recursively, so cc-option
can be implemented based on 'success' macro.





-- 
Best Regards
Masahiro Yamada
Ulf Magnusson Feb. 21, 2018, 4:41 p.m. | #7
On Wed, Feb 21, 2018 at 01:59:59PM +0900, Masahiro Yamada wrote:
> 2018-02-20 3:01 GMT+09:00 Linus Torvalds <torvalds@linux-foundation.org>:

> > On Mon, Feb 19, 2018 at 9:44 AM, Linus Torvalds

> > <torvalds@linux-foundation.org> wrote:

> >>

> >> I do like your "success"/"stdout" more than "shell"/"shell-stdout",

> >> because with that naming I don't get the feeling that one should

> >> subsume the other.

> >

> > Hmm. Thinking about it some more, I really would prefer just "$(shell

> > ...)" everywhere.

> >

> > But it would be nice if perhaps the error handling would match the

> > context somehow.

> >

> > I'm wondering if this might tie into the whole quoting discussion in

> > the other thread.

> >

> > Because the rule could be:

> >

> > (a) unquoted $(shell ) is a bool, and failing is ok (and turns into

> > y/n depending on whether successful or failing)

> >

> > So

> >

> >   config CC_IS_GCC

> >           bool

> >           default $(shell $CC --version | grep -q gcc)

> >

> > works automatically.

> >

> > (b) but with quoting, $(shell ) is a string, and failing is an error

> >

> > So

> >

> >   config GCC_VERSION

> >           int

> >           default "$(shell-stdout $srctree/scripts/gcc-version.sh $CC

> > | sed 's/^0*//')" if CC_IS_GCC

> >           default 0

> >

> > would need those quotes, and if the shell-script returns a failure,

> > we'd _abort_.

> 

> 

> GCC_VERSION is int type.

> 

> Setting aside the Kconfig internal, I prefer 50700 to "50700"

> 

> According to my common sense, I do not want to quote integers.


Yeah, definitely looks a bit weird coming from other languages. "" just
means constant (a literal value, implemented as a constant symbol, as
opposed to a symbol reference).


If I were to redesign things, something like this would be closer to
what people intuitively expect:

  - n, m, y produces three predefined tristate symbols (they already do,
    through the n/m/y -> "n"/"m"/"y" shorthand)

  - "foo" produces a symbol of type string. Get rid of "n", "m", "y"
    (would require cleanup in Kconfig files).

  - 123 produces a symbol of type int

  - 0x123 produces a symbol of type hex

  - Everything else is a symbol reference

A nice side effect would be making all undefined symbol references just
that. I think that might come in handy. Some simple type checking could
be added to expressions as well.

You could also get rid of the obscure "undefined symbols produce their
name as their string value" magic at that point if you wanted to. That's
what makes 123 work currently.

One thing that gets a bit icky is that Kconfig currently stores constant
symbols in the symbol table, meaning "123" as a string might clash with
123 as an int, if you use the string value as the key (this would be a
problem if you want to give them the proper type). I wonder if there's
even any point to storing constant symbols in the symbol table though,
as opposed to just having them appear in expressions.

> IMO, I prefer to use different names for different purpose.

> So, 'stdout' and 'success' look good to me.

> 

> 

> 

> BTW, I noticed just one built-in function is enough

> because 'success' can be derived from 'stdout'.

> 

> 

> So, my plan is, implement $(shell ...) as a built-in function.

> This returns the stdout from the command.

> 

> 

> Then, implement 'success' as a textual shorthand

> by using macro, like this:

> 

> macro success $(shell ($(1) && echo y) || echo n)

> 

> 

> macro can be expanded recursively, so cc-option

> can be implemented based on 'success' macro.


Was worried about the error handling for a second there, but this looks
like it might work out pretty nicely. $(success) would never have non-0
exit status, so you could still warn for that in $(shell).

There's also the question of shared Windows/Linux support in other
projects that use Kconfig, though my feeling is kernel devs don't care.
Might be pretty easy to work around anyway, by source'ing a different
Kconfig file with macro definitions depending on the OS. Would need that
for more complicated stuff anyway.

Cheers,
Ulf
Linus Torvalds Feb. 21, 2018, 5:01 p.m. | #8
On Tue, Feb 20, 2018 at 8:59 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>

> IMO, I prefer to use different names for different purpose.

> So, 'stdout' and 'success' look good to me.

>

> BTW, I noticed just one built-in function is enough

> because 'success' can be derived from 'stdout'.

>

> So, my plan is, implement $(shell ...) as a built-in function.

> This returns the stdout from the command.

>

> Then, implement 'success' as a textual shorthand

> by using macro, like this:

>

> macro success $(shell ($(1) && echo y) || echo n)


I like it. This is nice and clean, and seems to be very generic. I see
no issues with this approach.

                Linus

Patch

diff --git a/scripts/kconfig/function.c b/scripts/kconfig/function.c
index f7f154d..266f4ec 100644
--- a/scripts/kconfig/function.c
+++ b/scripts/kconfig/function.c
@@ -189,10 +189,56 @@  static char *do_shell(struct function *f, int argc, char *argv[])
 	return xstrdup(ret == 0 ? "y" : "n");
 }
 
+static char *do_shell_stdout(struct function *f, int argc, char *argv[])
+{
+	static const char *pre = "(";
+	static const char *post = ") 2>/dev/null";
+	FILE *p;
+	char buf[256];
+	char *cmd;
+	int ret;
+
+	if (argc != 2)
+		return NULL;
+
+	/*
+	 * Surround the command with ( ) in case it is piped commands.
+	 * Also, redirect stderr to /dev/null.
+	 */
+	cmd = xmalloc(strlen(pre) + strlen(argv[1]) + strlen(post) + 1);
+	strcpy(cmd, pre);
+	strcat(cmd, argv[1]);
+	strcat(cmd, post);
+
+	p = popen(cmd, "r");
+	if (!p) {
+		perror(cmd);
+		goto free;
+	}
+	if (fgets(buf, sizeof(buf), p)) {
+		size_t len = strlen(buf);
+
+		if (buf[len - 1] == '\n')
+			buf[len - 1] = '\0';
+	} else {
+		buf[0] = '\0';
+	}
+
+	ret = pclose(p);
+	if (ret == -1)
+		perror(cmd);
+
+free:
+	free(cmd);
+
+	return xstrdup(buf);
+}
+
 void func_init(void)
 {
 	/* register built-in functions */
 	func_add("shell", do_shell, NULL);
+	func_add("shell-stdout", do_shell_stdout, NULL);
 }
 
 void func_exit(void)