[RFC PATCH 0/3] kbuild: change the default shell to bash

[RFC PATCH 0/3] kbuild: change the default shell to bash

[Masahiro Yamada]

This is related to the discussion about the portability [1].

I wrote this code some time ago for some reason. My motivation at that
time was trap handling.

We use the EXIT trap in several places. The POSIX [2] mentions the 'EXIT'
as the trap condition, but nothing about the actual conditions that trigger
the EXIT trap. Bash invokes the EXIT trap when it exits after receiving
an unhandled signal, while dash does not.

I did not submit the patches because I lost the use-case of the EXIT trap
except cleaning temporary files. It is harmless to have temp files left over.

Recently, I saw a bug report regarding the portability of 'echo'
('echo' is a shell's built-in command. The behavior is different between
bash and dash).

I am sharing this patch set as RFC in case somebody might have interest or comment.
I am still wondering if this might be a big hammer, though.

[1]: https://lore.kernel.org/all/e902a360e3759c7f87d98d71d79a0d5cbe935e3e.camel@linuxfoundation.org/
[2]: https://pubs.opengroup.org/onlinepubs/009604599/utilities/trap.html



Masahiro Yamada (3):
  kconfig: move declarations for prepossessing to internal.h
  kconfig: allow to choose the shell for $(shell ) functions
  kbuild: use bash as the default shell for Make and Kconfig

 .../kbuild/kconfig-macro-language.rst         |  4 ++
 Makefile                                      |  7 ++
 scripts/Kconfig.include                       |  3 +
 scripts/kconfig/confdata.c                    |  1 +
 scripts/kconfig/internal.h                    | 18 +++++
 scripts/kconfig/lexer.l                       |  1 +
 scripts/kconfig/lkc_proto.h                   | 13 ----
 scripts/kconfig/parser.y                      |  1 +
 scripts/kconfig/preprocess.c                  | 66 +++++++++++++++----
 9 files changed, 87 insertions(+), 27 deletions(-)

-- 
2.34.1

[RFC PATCH 1/3] kconfig: move declarations for prepossessing to internal.h

[Masahiro Yamada]

These are used by the kconfig core part.

The user interface part (conf, mconf, nconf, gconf, or xconf variants)
does not need them.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c   |  1 +
 scripts/kconfig/internal.h   | 17 +++++++++++++++++
 scripts/kconfig/lexer.l      |  1 +
 scripts/kconfig/lkc_proto.h  | 13 -------------
 scripts/kconfig/preprocess.c |  1 +
 5 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c4340c90e172..aa8e73828de3 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -19,6 +19,7 @@
 #include <unistd.h>
 
 #include "lkc.h"
+#include "internal.h"
 
 /* return true if 'path' exists, false otherwise */
 static bool is_present(const char *path)
diff --git a/scripts/kconfig/internal.h b/scripts/kconfig/internal.h
index 2f7298c21b64..8e0e6d315b6c 100644
--- a/scripts/kconfig/internal.h
+++ b/scripts/kconfig/internal.h
@@ -1,4 +1,8 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Declarations necessary for the kconfig core, but not for
+ * conf, mconf, nconf, gconf, or xconf variants.
+ */
 #ifndef INTERNAL_H
 #define INTERNAL_H
 
@@ -6,4 +10,17 @@ struct menu;
 
 extern struct menu *current_menu, *current_entry;
 
+/* preprocess.c */
+enum variable_flavor {
+	VAR_SIMPLE,
+	VAR_RECURSIVE,
+	VAR_APPEND,
+};
+void env_write_dep(FILE *f, const char *auto_conf_name);
+void variable_add(const char *name, const char *value,
+		  enum variable_flavor flavor);
+void variable_all_del(void);
+char *expand_dollar(const char **str);
+char *expand_one_token(const char **str);
+
 #endif /* INTERNAL_H */
diff --git a/scripts/kconfig/lexer.l b/scripts/kconfig/lexer.l
index cc386e443683..43e12d0945a7 100644
--- a/scripts/kconfig/lexer.l
+++ b/scripts/kconfig/lexer.l
@@ -14,6 +14,7 @@
 #include <string.h>
 
 #include "lkc.h"
+#include "internal.h"
 #include "parser.tab.h"
 
 #define YY_DECL		static int yylex1(void)
diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h
index edd1e617b25c..7fb4953d16ca 100644
--- a/scripts/kconfig/lkc_proto.h
+++ b/scripts/kconfig/lkc_proto.h
@@ -35,18 +35,5 @@ const char * sym_get_string_value(struct symbol *sym);
 
 const char * prop_get_type_name(enum prop_type type);
 
-/* preprocess.c */
-enum variable_flavor {
-	VAR_SIMPLE,
-	VAR_RECURSIVE,
-	VAR_APPEND,
-};
-void env_write_dep(FILE *f, const char *auto_conf_name);
-void variable_add(const char *name, const char *value,
-		  enum variable_flavor flavor);
-void variable_all_del(void);
-char *expand_dollar(const char **str);
-char *expand_one_token(const char **str);
-
 /* expr.c */
 void expr_print(struct expr *e, void (*fn)(void *, struct symbol *, const char *), void *data, int prevtoken);
diff --git a/scripts/kconfig/preprocess.c b/scripts/kconfig/preprocess.c
index 748da578b418..aeb3fe362c04 100644
--- a/scripts/kconfig/preprocess.c
+++ b/scripts/kconfig/preprocess.c
@@ -11,6 +11,7 @@
 
 #include "list.h"
 #include "lkc.h"
+#include "internal.h"
 
 #define ARRAY_SIZE(arr)		(sizeof(arr) / sizeof((arr)[0]))
 
-- 
2.34.1

[RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[Masahiro Yamada]

GNU Make uses /bin/sh by default for running recipe lines and $(shell )
functions. You can change the shell by setting the 'SHELL' variable.
Unlike most variables, 'SHELL' is never set from the environment. [1]

Currently, Kconfig does not provide any way to change the default shell.
/bin/sh is always used for running $(shell,...) because do_shell() is
implemented by using popen(3).

This commit allows users to change the shell for Kconfig in a similar
way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
override the default shell. It is not taken from the environment. The
change is effective only for $(shell,...) invocations called after the
'SHELL' assignment.

[1]: https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 .../kbuild/kconfig-macro-language.rst         |  4 ++
 scripts/kconfig/internal.h                    |  1 +
 scripts/kconfig/parser.y                      |  1 +
 scripts/kconfig/preprocess.c                  | 65 +++++++++++++++----
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/Documentation/kbuild/kconfig-macro-language.rst b/Documentation/kbuild/kconfig-macro-language.rst
index 6163467f6ae4..fe8c6982179e 100644
--- a/Documentation/kbuild/kconfig-macro-language.rst
+++ b/Documentation/kbuild/kconfig-macro-language.rst
@@ -112,6 +112,10 @@ Kconfig currently supports the following built-in functions.
   replaced with a space. Any trailing newlines are deleted. The standard error
   is not returned, nor is any program exit status.
 
+  The program used as the shell is taken from the variable SHELL. If it is not
+  set anywhere in your Kconfig file, /bin/sh is used as the shell.
+  Unlike most variables, the variable SHELL is never set from the environment.
+
  - $(info,text)
 
   The "info" function takes a single argument and prints it to stdout.
diff --git a/scripts/kconfig/internal.h b/scripts/kconfig/internal.h
index 8e0e6d315b6c..c18017650e54 100644
--- a/scripts/kconfig/internal.h
+++ b/scripts/kconfig/internal.h
@@ -19,6 +19,7 @@ enum variable_flavor {
 void env_write_dep(FILE *f, const char *auto_conf_name);
 void variable_add(const char *name, const char *value,
 		  enum variable_flavor flavor);
+void variable_init(void);
 void variable_all_del(void);
 char *expand_dollar(const char **str);
 char *expand_one_token(const char **str);
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 2af7ce4e1531..436afaef9228 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -483,6 +483,7 @@ void conf_parse(const char *name)
 	zconf_initscan(name);
 
 	_menu_init();
+	variable_init();
 
 	if (getenv("ZCONF_DEBUG"))
 		yydebug = 1;
diff --git a/scripts/kconfig/preprocess.c b/scripts/kconfig/preprocess.c
index aeb3fe362c04..608a84e7f5d6 100644
--- a/scripts/kconfig/preprocess.c
+++ b/scripts/kconfig/preprocess.c
@@ -8,6 +8,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "list.h"
 #include "lkc.h"
@@ -141,24 +142,59 @@ static char *do_lineno(int argc, char *argv[])
 
 static char *do_shell(int argc, char *argv[])
 {
-	FILE *p;
+	int pipefd[2];
+	pid_t pid;
 	char buf[4096];
-	char *cmd;
-	size_t nread;
+	ssize_t nread;
 	int i;
 
-	cmd = argv[0];
-
-	p = popen(cmd, "r");
-	if (!p) {
-		perror(cmd);
+	if (pipe(pipefd) < 0) {
+		perror("pipe");
+		exit(1);
+	}
+	pid = fork();
+	if (pid < -1) {
+		perror("fork");
 		exit(1);
 	}
 
-	nread = fread(buf, 1, sizeof(buf), p);
+	if (pid == 0) { /* child */
+		char *shell;
+
+		/* duplicate the write end to stdout */
+		if (dup2(pipefd[1], STDOUT_FILENO) < 0) {
+			perror("dup2");
+			_exit(1);
+		}
+
+		/*
+		 * Do not leak file descriptors to the child process
+		 * (including the read end of the pipe).
+		 * Closing up to 15 is enough for us?
+		 */
+		for (i = STDERR_FILENO + 1; i < 16; i++)
+			close(i);
+
+		shell = expand_string("$(SHELL)");
+
+		execl(shell, shell, "-c", argv[0], NULL);
+		perror("execl");
+
+		free(shell);
+
+		_exit(1);
+	}
+
+	/* parent */
+
+	close(pipefd[1]);	/* the write end is unneeded */
+
+	nread = read(pipefd[0], buf, sizeof(buf));
 	if (nread == sizeof(buf))
 		nread--;
 
+	close(pipefd[0]);	/* now close the read end */
+
 	/* remove trailing new lines */
 	while (nread > 0 && buf[nread - 1] == '\n')
 		nread--;
@@ -171,11 +207,6 @@ static char *do_shell(int argc, char *argv[])
 			buf[i] = ' ';
 	}
 
-	if (pclose(p) == -1) {
-		perror(cmd);
-		exit(1);
-	}
-
 	return xstrdup(buf);
 }
 
@@ -330,6 +361,12 @@ static void variable_del(struct variable *v)
 	free(v);
 }
 
+void variable_init(void)
+{
+	/* Set the default shell */
+	variable_add("SHELL", "/bin/sh", VAR_RECURSIVE);
+}
+
 void variable_all_del(void)
 {
 	struct variable *v, *tmp;
-- 
2.34.1

Re: [RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[Bagas Sanjaya]

On 8/19/22 13:56, Masahiro Yamada wrote:
> GNU Make uses /bin/sh by default for running recipe lines and $(shell )
> functions. You can change the shell by setting the 'SHELL' variable.
> Unlike most variables, 'SHELL' is never set from the environment. [1]
> 
> Currently, Kconfig does not provide any way to change the default shell.
> /bin/sh is always used for running $(shell,...) because do_shell() is
> implemented by using popen(3).
> 
> This commit allows users to change the shell for Kconfig in a similar
> way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
> override the default shell. It is not taken from the environment. The
> change is effective only for $(shell,...) invocations called after the
> 'SHELL' assignment.
> 

Hmmm...

Can we say that if we run SHELL=/bin/bash make nconfig, Kconfig will use
$SHELL but we can't set it as environment variable?

-- 
An old man doll... just what I always wanted! - Clara

RE: [RFC PATCH 2/3] kconfig: allow to choose the shell for $(shell ) functions

[David Laight]

From: Bagas Sanjaya
> Sent: 19 August 2022 09:59
> 
> On 8/19/22 13:56, Masahiro Yamada wrote:
> > GNU Make uses /bin/sh by default for running recipe lines and $(shell )
> > functions. You can change the shell by setting the 'SHELL' variable.
> > Unlike most variables, 'SHELL' is never set from the environment. [1]
> >
> > Currently, Kconfig does not provide any way to change the default shell.
> > /bin/sh is always used for running $(shell,...) because do_shell() is
> > implemented by using popen(3).
> >
> > This commit allows users to change the shell for Kconfig in a similar
> > way to GNU Make; you can set the 'SHELL' variable in a Kconfig file to
> > override the default shell. It is not taken from the environment. The
> > change is effective only for $(shell,...) invocations called after the
> > 'SHELL' assignment.
> >
> 
> Hmmm...
> 
> Can we say that if we run SHELL=/bin/bash make nconfig, Kconfig will use
> $SHELL but we can't set it as environment variable?

That just puts it into the environment for the single command.
You'd need to pass it as a command line argument to make.

(Or pass it in a different environment variable and then assign
it within the makefile.)

Or just remove the crappy bashisms and write portable shell scripts.
Just be glad you're not trying to use the SYSV /bin/sh.
(And avoid the other bugs that make dash fail to run most of the
scripts I write.)

Compex echo requests can be replaced by printf (without penalty
since it will be a shell builtin).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[RFC PATCH 3/3] kbuild: use bash as the default shell for Make and Kconfig

[Masahiro Yamada]

GNU Make uses /bin/sh by default for running recipe lines and $(shell ),
and so does Kconfig for $(shell,...).

We are sometimes hit by subtle portability issues like [1] because the
code was only tested on systems where /bin/sh is a symlink to bash.

One way is to continue whack-a-mole game as somebody will eventually
notice that the code is not working on their build environments.

Another is a big hammer approach; force bash in the build system so that
it works in the same way for everyone. In fact, there exists such a
guideline that permits only Bash as the shell scripting language. [2]
The kbuild test robot also specifies SHELL=/bin/bash to eliminate the
shell portability issue.

This commit changes the shell for Make and Kconfig to bash. As the GNU
Make manual [3] says, the 'SHELL' is never set from the environment.
You cannot do 'export SHELL:=/bin/bash' to propagate it to sub-makes.
Instead, use MAKEOVERRIDES to make SHELL:=/bin/bash globally effective.

[1]: https://lore.kernel.org/all/20220804190320.262510-1-alexandre.belloni@bootlin.com/
[2]: https://google.github.io/styleguide/shellguide.html
[3]: https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Makefile                | 7 +++++++
 scripts/Kconfig.include | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index f09673b6c11d..cc01987560b3 100644
--- a/Makefile
+++ b/Makefile
@@ -36,6 +36,13 @@ __all:
 
 ifneq ($(sub_make_done),1)
 
+# Use /bin/bash for repices and $(shell ) invocations.
+SHELL := /bin/bash
+
+# Unlike the other variables, the 'SHELL' is never set from the environment.
+# Use MAKEOVERRIDES to pass 'SHELL' down to sub-makes.
+MAKEOVERRIDES += SHELL=$(SHELL)
+
 # Do not use make's built-in rules and variables
 # (this increases performance and avoids hard-to-debug behaviour)
 MAKEFLAGS += -rR
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index a0ccceb22cf8..d37b864ba1f2 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -1,6 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
 # Kconfig helper macros
 
+# Use /bin/bash to evaluate $(shell,) functions
+SHELL := /bin/bash
+
 # Convenient variables
 comma       := ,
 quote       := "
-- 
2.34.1