Fix for bug #1276: Handle SHELL according to POSIX requirements.

POSIX requires that the value of SHELL in the makefile NOT be exported
to sub-commands.  Instead, the value in the environment when make was
invoked should be passed to the environment of sub-commands.  Note that
make still uses SHELL to _run_ sub-commands; it just doesn't change the
value of the SHELL variable in the environment of sub-commands.

As an extension to POSIX, if the makefile explicitly exports SHELL then
GNU make _will_ use it in the environment of sub-commands.

8 files changed, 150 insertions, 46 deletions

diff --git a/ChangeLog b/ChangeLog
index 6ec8a1d..b1dcefd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,17 @@
 2004-11-28  Paul D. Smith  <psmith@gnu.org>
 
+	Fix for bug #1276: Handle SHELL according to POSIX requirements.
+
+	* main.c (main): Set SHELL to v_noexport by default.  Remember the
+	original environment setting of SHELL in the env_shell variable.
+	* main.h: Export new env_shell variable.
+	* variable.c (target_environment): If we find a v_noexport
+	variable for SHELL, add a SHELL variable with the env_shell value.
+	* doc/make.texi (Quick Reference): Document the POSIX behavior.
+	* doc/make.texi (Variables/Recursion): Ditto.
+
+2004-11-28  Paul D. Smith  <psmith@gnu.org>
+
 	* main.c (find_and_set_default_shell) [WINDOWS32]: check for
 	equality of "cmd"/"cmd.exe", not inequality.  Fixes bug #11155.
 	Patch by Alessandro Vesely.
diff --git a/doc/make.texi b/doc/make.texi
index a31c3ad..14d7278 100644
--- a/doc/make.texi
+++ b/doc/make.texi
@@ -3732,9 +3732,15 @@ line, and if its name consists only of letters, numbers, and underscores.
 Some shells cannot cope with environment variable names consisting of
 characters other than letters, numbers, and underscores.
 
-The special variables @code{SHELL} and @code{MAKEFLAGS} are always
-exported (unless you unexport them).
-@code{MAKEFILES} is exported if you set it to anything.
+@cindex SHELL, exported value
+The value of the @code{make} variable @code{SHELL} is not exported.
+Instead, the value of the @code{SHELL} variable from the invoking
+environment is passed to the sub-@code{make}.  You can force
+@code{make} to export its value for @code{SHELL} by using the
+@code{export} directive, described below.
+
+The special variable @code{MAKEFLAGS} is always exported (unless you
+unexport it).  @code{MAKEFILES} is exported if you set it to anything.
 
 @code{make} automatically passes down variable values that were defined
 on the command line, by putting them in the @code{MAKEFLAGS} variable.
@@ -5137,28 +5143,28 @@ endef
 @cindex variables, environment
 @cindex environment
 Variables in @code{make} can come from the environment in which
-@code{make} is run.  Every environment variable that @code{make} sees when
-it starts up is transformed into a @code{make} variable with the same name
-and value.  But an explicit assignment in the makefile, or with a command
-argument, overrides the environment.  (If the @samp{-e} flag is specified,
-then values from the environment override assignments in the makefile.
-@xref{Options Summary, ,Summary of Options}.
-But this is not recommended practice.)
+@code{make} is run.  Every environment variable that @code{make} sees
+when it starts up is transformed into a @code{make} variable with the
+same name and value.  However, an explicit assignment in the makefile,
+or with a command argument, overrides the environment.  (If the
+@samp{-e} flag is specified, then values from the environment override
+assignments in the makefile.  @xref{Options Summary, ,Summary of
+Options}.  But this is not recommended practice.)
 
 Thus, by setting the variable @code{CFLAGS} in your environment, you can
 cause all C compilations in most makefiles to use the compiler switches you
 prefer.  This is safe for variables with standard or conventional meanings
-because you know that no makefile will use them for other things.  (But
+because you know that no makefile will use them for other things.  (Note
 this is not totally reliable; some makefiles set @code{CFLAGS} explicitly
 and therefore are not affected by the value in the environment.)
 
-When @code{make} is invoked recursively, variables defined in the
-outer invocation can be passed to inner invocations through the
-environment (@pxref{Recursion, ,Recursive Use of @code{make}}).  By
-default, only variables that came from the environment or the command
-line are passed to recursive invocations.  You can use the
-@code{export} directive to pass other variables.
-@xref{Variables/Recursion, , Communicating Variables to a
+When @code{make} runs a command script, variables defined in the
+makefile are placed into the environment of that command.  This allows
+you to pass values to sub-@code{make} invocations. (@pxref{Recursion,
+,Recursive Use of @code{make}}).  By default, only variables that came
+from the environment or the command line are passed to recursive
+invocations.  You can use the @code{export} directive to pass other
+variables.  @xref{Variables/Recursion, , Communicating Variables to a
 Sub-@code{make}}, for full details.
 
 Other use of variables from the environment is not recommended.  It is not
@@ -5167,6 +5173,7 @@ set up outside their control, since this would cause different users to get
 different results from the same makefile.  This is against the whole
 purpose of most makefiles.
 
+@cindex SHELL, import from environment
 Such problems would be especially likely with the variable @code{SHELL},
 which is normally present in the environment to specify the user's choice
 of interactive shell.  It would be very undesirable for this choice to
@@ -5175,6 +5182,15 @@ affect @code{make}.  So @code{make} ignores the environment value of
 usually not set.  @xref{Execution, ,Special handling of SHELL on
 MS-DOS}.)@refill
 
+@cindex SHELL, export to environment
+The @code{SHELL} variable is special in another way: just as the value
+of the @code{make} variable @code{SHELL} is not taken from the
+environment, so also it is not placed into the environment of commands
+that @code{make} invokes.  Instead, the value of @code{SHELL} from the
+invoking environment is provided to the command.  You can use
+@code{export SHELL} to force the value of the @code{make} variable
+@code{SHELL} to be placed in the environment of commands.
+
 @node Target-specific, Pattern-specific, Environment, Using Variables
 @section Target-specific Variable Values
 @cindex target-specific variables
@@ -9865,7 +9881,9 @@ Directory search path for files not found in the current directory.@*
 
 The name of the system default command interpreter, usually @file{/bin/sh}.
 You can set @code{SHELL} in the makefile to change the shell used to run
-commands.  @xref{Execution, ,Command Execution}.
+commands.  @xref{Execution, ,Command Execution}.  The @code{SHELL}
+variable is handled specially when importing from and exporting to the
+environment.  @xref{Environment, ,Using Variable from the Environment}.
 
 @item MAKESHELL
 
diff --git a/main.c b/main.c
index ca6318b..f0229ee 100644
--- a/main.c
+++ b/main.c
@@ -264,6 +264,10 @@ int always_make_flag = 0;
 
 int rebuilding_makefiles = 0;
 
+/* Remember the original value of the SHELL variable, from the environment.  */
+
+const char *env_shell = 0;
+
 
 /* The usage output.  We write it this way to make life easier for the
    translators, especially those trying to translate to right-to-left
@@ -1045,11 +1049,8 @@ main (int argc, char **argv, char **envp)
 #ifndef _AMIGA
   for (i = 0; envp[i] != 0; ++i)
     {
-      int do_not_define;
-      register char *ep = envp[i];
-
-      /* by default, everything gets defined and exported */
-      do_not_define = 0;
+      int do_not_define = 0;
+      char *ep = envp[i];
 
       while (*ep != '=')
         ++ep;
@@ -1065,17 +1066,27 @@ main (int argc, char **argv, char **envp)
 	 machines where ptrdiff_t is a different size that doesn't widen
 	 the same.  */
       if (!do_not_define)
-        define_variable (envp[i], (unsigned int) (ep - envp[i]),
-                         ep + 1, o_env, 1)
-	/* Force exportation of every variable culled from the environment.
-	   We used to rely on target_environment's v_default code to do this.
-	   But that does not work for the case where an environment variable
-	   is redefined in a makefile with `override'; it should then still
-	   be exported, because it was originally in the environment.
-           Another wrinkle is that POSIX says the value of SHELL set in the
-           makefile should not change the value of SHELL given to
-           subprocesses, which seems silly to me but...  */
-	->export = strncmp(envp[i], "SHELL=", 6) ? v_noexport : v_export;
+        {
+          struct variable *v;
+
+          v = define_variable (envp[i], (unsigned int) (ep - envp[i]),
+                               ep + 1, o_env, 1);
+          /* Force exportation of every variable culled from the environment.
+             We used to rely on target_environment's v_default code to do this.
+             But that does not work for the case where an environment variable
+             is redefined in a makefile with `override'; it should then still
+             be exported, because it was originally in the environment.  */
+          v->export = v_export;
+
+          /* Another wrinkle is that POSIX says the value of SHELL set in the
+             makefile should not change the value of SHELL given to
+             subprocesses, which seems silly to me but...  */
+          if (strncmp (envp[i], "SHELL=", 6) == 0)
+            {
+              v->export = v_noexport;
+              env_shell = xstrdup (ep + 1);
+            }
+        }
     }
 #ifdef WINDOWS32
     /*
diff --git a/make.h b/make.h
index ad46e1f..55dcc21 100644
--- a/make.h
+++ b/make.h
@@ -496,6 +496,8 @@ extern int print_version_flag, print_directory_flag;
 extern int warn_undefined_variables_flag, posix_pedantic, not_parallel;
 extern int clock_skew_detected, rebuilding_makefiles;
 
+extern const char *env_shell;
+
 /* can we run commands via 'sh -c xxx' or must we use batch files? */
 extern int batch_mode_shell;
 
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 8cc25db..a6423b0 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2004-11-28  Paul D. Smith  <psmith@gnu.org>
+
+	* scripts/variables/SHELL: New file: test proper handling of SHELL
+	according to POSIX rules.  Fixes bug #1276.
+
 2004-10-21  Boris Kolpackov  <boris@kolpackov.net>
 
 	* scripts/functions/word: Test $(firstword ) and $(lastword ).
diff --git a/tests/scripts/variables/MAKELEVEL b/tests/scripts/variables/MAKELEVEL
index 79a184e..96a4e74 100644
--- a/tests/scripts/variables/MAKELEVEL
+++ b/tests/scripts/variables/MAKELEVEL
@@ -1,4 +1,4 @@
-#                                                               -*-perl-mode-*-
+#                                                                    -*-perl-*-
 
 $description = "The following test creates a makefile to test
 makelevels in Make. It prints \$(MAKELEVEL) and then
@@ -9,7 +9,6 @@ open(MAKEFILE,"> $makefile");
 # The Contents of the MAKEFILE ...
 
 print MAKEFILE <<EOF;
-SHELL = /bin/sh
 all:
 \t\@echo MAKELEVEL is \$(MAKELEVEL)
 \techo \$\$MAKELEVEL
diff --git a/tests/scripts/variables/SHELL b/tests/scripts/variables/SHELL
new file mode 100644
index 0000000..9ff5c4b
--- /dev/null
+++ b/tests/scripts/variables/SHELL
@@ -0,0 +1,49 @@
+#                                                                    -*-perl-*-
+
+$description = "Test proper handling of SHELL.";
+
+# Find the default value when SHELL is not set.  On UNIX it will be /bin/sh,
+# but on other platforms who knows?
+$oshell = $ENV{SHELL};
+delete $ENV{SHELL};
+$mshell = `echo 'all:;\@echo \$(SHELL)' | $make_name -f-`;
+chop $mshell;
+
+# According to POSIX, the value of SHELL in the environment has no impact on
+# the value in the makefile.
+
+$ENV{SHELL} = '/dev/null';
+run_make_test('all:;@echo "$(SHELL)"', '', $mshell);
+
+# According to POSIX, any value of SHELL set in the makefile should _NOT_ be
+# exported to the subshell!  I wanted to set SHELL to be $^X (perl) in the
+# makefile, but make runs $(SHELL) -c 'commandline' and that doesn't work at
+# all when $(SHELL) is perl :-/.  So, we just add an extra initial / and hope
+# for the best on non-UNIX platforms :-/.
+
+$ENV{SHELL} = $mshell;
+
+run_make_test("SHELL := /$mshell\n".'
+all:;@echo "$(SHELL) $$SHELL"
+', '', "/$mshell $mshell");
+
+# As a GNU make extension, if make's SHELL variable is explicitly exported,
+# then we really _DO_ export it.
+
+run_make_test("export SHELL := /$mshell\n".'
+all:;@echo "$(SHELL) $$SHELL"
+', '', "/$mshell /$mshell");
+
+
+# Test out setting of SHELL, both exported and not, as a target-specific
+# variable.
+
+run_make_test("all: SHELL := /$mshell\n".'
+all:;@echo "$(SHELL) $$SHELL"
+', '', "/$mshell $mshell");
+
+run_make_test("all: export SHELL := /$mshell\n".'
+all:;@echo "$(SHELL) $$SHELL"
+', '', "/$mshell $mshell");
+
+1;
diff --git a/variable.c b/variable.c
index 31b84a4..8398d18 100644
--- a/variable.c
+++ b/variable.c
@@ -739,16 +739,14 @@ define_automatic_variables (void)
 
 #endif
 
-  /* This won't override any definition, but it
-     will provide one if there isn't one there.  */
+  /* This won't override any definition, but it will provide one if there
+     isn't one there.  */
   v = define_variable ("SHELL", 5, default_shell, o_default, 0);
-  v->export = v_export;		/* Always export SHELL.  */
 
-  /* On MSDOS we do use SHELL from environment, since
-     it isn't a standard environment variable on MSDOS,
-     so whoever sets it, does that on purpose.
-     On OS/2 we do not use SHELL from environment but
-     we have already handled that problem above. */
+  /* On MSDOS we do use SHELL from environment, since it isn't a standard
+     environment variable on MSDOS, so whoever sets it, does that on purpose.
+     On OS/2 we do not use SHELL from environment but we have already handled
+     that problem above. */
 #if !defined(__MSDOS__) && !defined(__EMX__)
   /* Don't let SHELL come from the environment.  */
   if (*v->value == '\0' || v->origin == o_env || v->origin == o_env_override)
@@ -809,6 +807,11 @@ target_environment (struct file *file)
   struct variable makelevel_key;
   char **result_0;
   char **result;
+  struct variable ev;
+
+  /* Set up a fake variable struct for the original SHELL value.  */
+  ev.name = "SHELL";
+  ev.value = env_shell;
 
   if (file == 0)
     set_list = current_variable_set_list;
@@ -865,7 +868,12 @@ target_environment (struct file *file)
 		break;
 
 	      case v_noexport:
-		continue;
+                if (!streq (v->name, "SHELL"))
+                  continue;
+                /* If this is the SHELL variable and it's not exported, then
+                   add the value from our original environment.  */
+                v = &ev;
+                break;
 
 	      case v_ifset:
 		if (v->origin == o_default)