[SV 10593] Export variables to $(shell ...) commands

Export all variables, including exported makefile variables, when
invoking a shell for the $(shell ...) function.  If we detect a
recursive variable expansion, silently ignore that variable and do
not export it.  We do print a debug message.

* NEWS: Announce the potential backward-incompatibility.
* doc/make.texi (Shell Function): Document the export behavior.
* src/main.c (main): Add "shell-export" to .FEATURES.
* src/job.h: New function to free struct childbase.
* src/job.c (free_childbase): Implement it; call from free_child.
* src/function.c (func_shell_base): Use target_environment() to
obtain the proper environment for the shell function.
Use free_childbase() to free memory.
(windows32_openpipe): Don't reset the environment: the caller
already provided a proper PATH variable in envp.
* src/variable.c (target_environment): If we detect a recursive
expansion and we're called from func_shell, ignore the variable.
(sync_Path_environment): Simplify and reduce memory allocation.
* tests/scripts/functions/shell: Add tests for this.

NEWS

@@ -33,6 +33,12 @@ https://sv.gnu.org/bugs/index.php?group=make&report_id=111&fix_release_id=109&se
   variable that was visible while parsing makefiles.  Now, all options
   are available in MAKEFLAGS.
 
+* WARNING: Backward-incompatibility!
+  Previously makefile variables marked as export were not exported to commands
+  started by the $(shell ...) function.  Now, all exported variables are
+  exported to $(shell ...).
+  To detect this change search for 'shell-export' in the .FEATURES variable.
+
 * WARNING: New build requirement
   GNU make utilizes facilities from GNU Gnulib: Gnulib requires certain C99
   features in the C compiler and so these features are required by GNU make:

doc/make.texi

@@ -8616,6 +8616,23 @@ using a very strange shell, this has the same result as
 @w{@samp{$(wildcard *.c)}} (as long as at least one @samp{.c} file
 exists).@refill
 
+All variables that are marked as @code{export} will also be passed to the
+shell started by the @code{shell} function.  It is possible to create a
+variable expansion loop: consider this @file{makefile}:
+
+@example
+export HI = $(shell echo hi)
+all: ; @@echo $$HI
+@end example
+
+When @code{make} wants to run the recipe it must add the variable @var{HI} to
+the environment; to do so it must be expanded.  The value of this variable
+requires an invocation of the @code{shell} function, and to invoke it we must
+create its environment.  Since @var{HI} is exported, we need to expand it to
+create its environment.  And so on.  In this obscure case @code{make} will not
+add recursively-expanded variables to the @code{shell} environment rather than
+fail with an error.
+
 @node Guile Function,  , Shell Function, Functions
 @section The @code{guile} Function
 @findex guile

src/function.c

@@ -1725,12 +1725,6 @@ windows32_openpipe (int *pipedes, int errfd, pid_t *pid_p, char **command_argv,
       return -1;
     }
 
-  /* make sure that CreateProcess() has Path it needs */
-  sync_Path_environment ();
-  /* 'sync_Path_environment' may realloc 'environ', so take note of
-     the new value.  */
-  envp = environ;
-
   if (! process_begin (hProcess, command_argv, envp, command_argv[0], NULL))
     {
       /* register process for wait */
@@ -1845,13 +1839,13 @@ func_shell_base (char *o, char **argv, int trim_newlines)
 char *
 func_shell_base (char *o, char **argv, int trim_newlines)
 {
+  struct childbase child = {0};
   char *batch_filename = NULL;
   int errfd;
 #ifdef __MSDOS__
   FILE *fpipe;
 #endif
   char **command_argv = NULL;
-  char **envp;
   int pipedes[2];
   pid_t pid;
 
@@ -1876,26 +1870,14 @@ func_shell_base (char *o, char **argv, int trim_newlines)
     }
 #endif /* !__MSDOS__ */
 
-  /* Using a target environment for 'shell' loses in cases like:
-       export var = $(shell echo foobie)
-       bad := $(var)
-     because target_environment hits a loop trying to expand $(var) to put it
-     in the environment.  This is even more confusing when 'var' was not
-     explicitly exported, but just appeared in the calling environment.
-
-     See Savannah bug #10593.
-
-  envp = target_environment (NULL);
-  */
-
-  envp = environ;
-
   /* Set up the output in case the shell writes something.  */
   output_start ();
 
   errfd = (output_context && output_context->err >= 0
            ? output_context->err : FD_STDERR);
 
+  child.environment = target_environment (NULL);
+
 #if defined(__MSDOS__)
   fpipe = msdos_openpipe (pipedes, &pid, argv[0]);
   if (pipedes[0] < 0)
@@ -1906,7 +1888,7 @@ func_shell_base (char *o, char **argv, int trim_newlines)
     }
 
 #elif defined(WINDOWS32)
-  windows32_openpipe (pipedes, errfd, &pid, command_argv, envp);
+  windows32_openpipe (pipedes, errfd, &pid, command_argv, child.environment);
   /* Restore the value of just_print_flag.  */
   just_print_flag = j_p_f;
 
@@ -1931,19 +1913,12 @@ func_shell_base (char *o, char **argv, int trim_newlines)
   fd_noinherit (pipedes[1]);
   fd_noinherit (pipedes[0]);
 
-  {
-    struct childbase child;
-    child.cmd_name = NULL;
   child.output.syncout = 1;
   child.output.out = pipedes[1];
   child.output.err = errfd;
-    child.environment = envp;
 
   pid = child_execute_job (&child, 1, command_argv);
 
-    free (child.cmd_name);
-  }
-
   if (pid < 0)
     {
       shell_completed (127, 0);
@@ -2029,6 +2004,8 @@ func_shell_base (char *o, char **argv, int trim_newlines)
       free (command_argv);
     }
 
+  free_childbase (&child);
+
   return o;
 }
 

src/job.c

@@ -1117,6 +1117,20 @@ reap_children (int block, int err)
 
 /* Free the storage allocated for CHILD.  */
 
+void
+free_childbase (struct childbase *child)
+{
+  if (child->environment != 0)
+    {
+      char **ep = child->environment;
+      while (*ep != 0)
+        free (*ep++);
+      free (child->environment);
+    }
+
+  free (child->cmd_name);
+}
+
 static void
 free_child (struct child *child)
 {
@@ -1149,15 +1163,8 @@ free_child (struct child *child)
       free (child->command_lines);
     }
 
-  if (child->environment != 0)
-    {
-      char **ep = child->environment;
-      while (*ep != 0)
-        free (*ep++);
-      free (child->environment);
-    }
+  free_childbase ((struct childbase*)child);
 
-  free (child->cmd_name);
   free (child);
 }
 

src/job.h

@@ -73,6 +73,7 @@ int is_bourne_compatible_shell(const char *path);
 void new_job (struct file *file);
 void reap_children (int block, int err);
 void start_waiting_jobs (void);
+void free_childbase (struct childbase* child);
 
 char **construct_command_argv (char *line, char **restp, struct file *file,
                                int cmd_flags, char** batch_file);

src/main.c

@@ -1344,6 +1344,7 @@ main (int argc, char **argv, char **envp)
     const char *features = "target-specific order-only second-expansion"
                            " else-if shortest-stem undefine oneshell nocomment"
                            " grouped-target extra-prereqs notintermediate"
+                           " shell-export"
 #ifndef NO_ARCHIVES
                            " archives"
 #endif

src/variable.c

@@ -19,6 +19,7 @@ this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <assert.h>
 
 #include "filedef.h"
+#include "debug.h"
 #include "dep.h"
 #include "job.h"
 #include "commands.h"
@@ -1099,8 +1100,14 @@ target_environment (struct file *file)
         /* If V is recursively expanded and didn't come from the environment,
            expand its value.  If it came from the environment, it should
            go back into the environment unchanged.  */
-        if (v->recursive
-            && v->origin != o_env && v->origin != o_env_override)
+        if (v->recursive && v->origin != o_env && v->origin != o_env_override)
+          {
+            /* If V is being recursively expanded and this is for a shell
+               function, just skip it.  */
+            if (v->expanding && file == NULL)
+              DB (DB_VERBOSE, (_("%s:%lu: Skipping export of %s to shell function due to recursive expansion"),
+                               v->fileinfo.filenm, v->fileinfo.lineno, v->name));
+            else
               {
                 char *value = recursively_expand_for_file (v, file);
 #ifdef WINDOWS32
@@ -1111,6 +1118,7 @@ target_environment (struct file *file)
                 *result++ = xstrdup (concat (3, v->name, "=", value));
                 free (value);
               }
+          }
         else
           {
 #ifdef WINDOWS32
@@ -1858,21 +1866,20 @@ print_target_variables (const struct file *file)
 
 #ifdef WINDOWS32
 void
-sync_Path_environment (void)
+sync_Path_environment ()
 {
-  char *path = allocated_variable_expand ("$(PATH)");
   static char *environ_path = NULL;
+  char *oldpath = environ_path;
+  char *path = allocated_variable_expand ("PATH=$(PATH)");
 
   if (!path)
     return;
 
-  /* If done this before, free the previous entry before allocating new one.  */
-  free (environ_path);
-
-  /* Create something WINDOWS32 world can grok.  */
+  /* Convert PATH into something WINDOWS32 world can grok.  */
   convert_Path_to_windows32 (path, ';');
-  environ_path = xstrdup (concat (3, "PATH", "=", path));
+
+  environ_path = path;
   putenv (environ_path);
-  free (path);
+  free (oldpath);
 }
 #endif

tests/scripts/functions/shell

@@ -32,14 +32,61 @@ foo: ; echo '$(FOO)'
 !,
               '', "echo '#'\n#\n");
 
-# Test shells inside exported environment variables.
-# This is the test that fails if we try to put make exported variables into
-# the environment for a $(shell ...) call.
+# Test that exported variables are passed to $(shell ...)
+$ENV{FOO} = 'baz';
+run_make_test(q!
+OUT = $(shell echo $$FOO)
+foo: ; @echo '$(OUT)'
+!,
+              '', 'baz');
+
+$ENV{FOO} = 'baz';
+run_make_test(q!
+FOO = bar
+OUT = $(shell echo $$FOO)
+foo: ; @echo '$(OUT)'
+!,
+              '', 'bar');
+
+run_make_test(q!
+export FOO = bar
+OUT = $(shell echo $$FOO)
+foo: ; @echo '$(OUT)'
+!,
+              '', 'bar');
+
+# Test shells inside exported environment variables, simply expanded.
+run_make_test('
+export HI := $(shell echo hi)
+.PHONY: all
+all: ; @echo $$HI
+',
+              '','hi');
+
+# Test shells inside exported environment variables.  See SV 10593
 run_make_test('
 export HI = $(shell echo hi)
 .PHONY: all
 all: ; @echo $$HI
-    ','','hi');
+',
+              '',"hi");
+
+$ENV{HI} = 'foo';
+run_make_test('
+HI = $(shell echo hi)
+.PHONY: all
+all: ; @echo $$HI
+',
+              '',"hi");
+
+$ENV{HI} = 'foo';
+run_make_test('
+HI = $(shell echo hi)
+bad := $(HI)
+.PHONY: all
+all: ; @echo $$HI $(bad)
+',
+              '',"hi hi");
 
 if ($port_type ne 'W32') {
     # Test shell errors in recipes including offset