[SV 63040] shell: Fall back to the callers environment

If we detect a recursive variable reference when constructing the
environment for the shell function, return the original value from the
caller's environment.  Other options such as failing, returning the
empty string, or returning the unexpanded make variable value have
been shown to not behave well in real-world environments.  If the
variable doesn't exist in the caller's environment, return the empty
string.

Found by Sergei Trofimovich <slyich@gmail.com> when testing older
versions of autoconf.

* NEWS: Clarify this behavior.
* doc/make.texi (Shell Function): Ditto.  Also add info about !=.
* src/expand.c (recursively_expand_for_file): Search the caller's
environment if we detect a recursive variable expansion.
* tests/scripts/functions/shell: Add tests for this behavior.

NEWS

@@ -36,7 +36,8 @@ https://sv.gnu.org/bugs/index.php?group=make&report_id=111&fix_release_id=109&se
 * 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 ...).
+  exported to $(shell ...).  If this leads to recursion during expansion, then
+  for backward-compatibility the value from the original environment is used.
   To detect this change search for 'shell-export' in the .FEATURES variable.
 
 * WARNING: New build requirement

doc/make.texi

@@ -8629,21 +8629,25 @@ The @code{shell} function is unlike any other function other than the
 (@pxref{Wildcard Function, ,The Function @code{wildcard}}) in that it
 communicates with the world outside of @code{make}.
 
-The @code{shell} function performs the same function that backquotes
+The @code{shell} function provides for @code{make} the same facility that
-(@samp{`}) perform in most shells: it does @dfn{command expansion}.
+backquotes (@samp{`}) provide in most shells: it does @dfn{command expansion}.
-This means that it takes as an argument a shell command and evaluates
+This means that it takes as an argument a shell command and expands to the
-to the output of the command.  The only processing @code{make} does on
+output of the command.  The only processing @code{make} does on the result is
-the result is to convert each newline (or carriage-return / newline
+to convert each newline (or carriage-return / newline pair) to a single space.
-pair) to a single space.  If there is a trailing (carriage-return
+If there is a trailing (carriage-return and) newline it will simply be
-and) newline it will simply be removed.@refill
+removed.
 
 The commands run by calls to the @code{shell} function are run when the
-function calls are expanded (@pxref{Reading Makefiles, , How
+function calls are expanded (@pxref{Reading Makefiles, , How @code{make} Reads
-@code{make} Reads a Makefile}).  Because this function involves
+a Makefile}).  Because this function involves spawning a new shell, you should
-spawning a new shell, you should carefully consider the performance
+carefully consider the performance implications of using the @code{shell}
-implications of using the @code{shell} function within recursively
+function within recursively expanded variables vs.@: simply expanded variables
-expanded variables vs.@: simply expanded variables (@pxref{Flavors, ,The
+(@pxref{Flavors, ,The Two Flavors of Variables}).
-Two Flavors of Variables}).
+
+An alternative to the @code{shell} function is the @samp{!=} assignment
+operator; it provides a similar behavior but has subtle differences
+(@pxref{Setting, , Setting Variables}).  The @samp{!=} assignment operator is
+included in newer POSIX standards.
 
 @vindex .SHELLSTATUS
 After the @code{shell} function or @samp{!=} assignment operator is
@@ -8682,9 +8686,17 @@ 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
+create its environment.  And so on.  In this obscure case @code{make} will use
-add recursively-expanded variables to the @code{shell} environment rather than
+the value of the variable from the environment provided to @code{make}, or
-fail with an error.
+else the empty string if there was none, rather than looping or issuing an
+error.  This is often what you want; for example:
+
+@example
+export PATH = $(shell echo /usr/local/bin:$$PATH)
+@end example
+
+However, it would be simpler and more efficient to use a simply-expanded
+variable here (@samp{:=}) in the first place.
 
 @node Guile Function,  , Shell Function, Functions
 @section The @code{guile} Function

src/expand.c

@@ -103,12 +103,25 @@ recursively_expand_for_file (struct variable *v, struct file *file)
   int set_reading = 0;
 
   /* If we're expanding to put into the environment of a shell function then
-     ignore any recursion issues.  */
+     ignore any recursion issues: for backward-compatibility we will use
+     the value of the environment variable we were started with.  */
   if (v->expanding && env_recursion)
     {
+      size_t nl = strlen (v->name);
+      char **ep;
       DB (DB_VERBOSE,
           (_("%s:%lu: not recursively expanding %s to export to shell function\n"),
            v->fileinfo.filenm, v->fileinfo.lineno, v->name));
+
+      /* We could create a hash for the original environment for speed, but a
+         reasonably written makefile shouldn't hit this situation...  */
+      for (ep = environ; *ep != 0; ++ep)
+        if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0)
+          return xstrdup ((*ep) + nl + 1);
+
+      /* If there's nothing in the parent environment, use the empty string.
+         This isn't quite correct since the variable should not exist at all,
+         but getting that to work would be involved. */
       return xstrdup ("");
     }
 

tests/scripts/functions/shell

@@ -98,6 +98,22 @@ all:; : $(HI)
 ',
               '',": hi");
 
+$ENV{HI} = 'outer';
+run_make_test('
+export HI = $(shell echo $$HI)
+.PHONY: all
+all:; @echo $$HI
+',
+              '',"outer");
+
+$ENV{HI} = 'outer';
+run_make_test('
+export HI = $(shell echo $$HI)
+.PHONY: all
+all:; : $(HI)
+',
+              '',": outer");
+
 if ($port_type ne 'W32') {
     # Test shell errors in recipes including offset
     # This needs to be ported to Windows, or else Windows error messages