| Message ID | 1518806331-7101-12-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | kconfig: move compiler capability tests to Kconfig | expand |
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
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
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
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
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
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
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
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
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)
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