[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.

4 files changed, 60 insertions, 18 deletions

diff --git a/NEWS b/NEWS
index 16c67059..4d635820 100644
--- a/NEWS
+++ b/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
diff --git a/doc/make.texi b/doc/make.texi
index 6fb8bb8c..1598ef65 100644
--- a/doc/make.texi
+++ b/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
-(@samp{`}) perform in most shells: it does @dfn{command expansion}.
-This means that it takes as an argument a shell command and evaluates
-to the output of the command.  The only processing @code{make} does on
-the result is to convert each newline (or carriage-return / newline
-pair) to a single space.  If there is a trailing (carriage-return
-and) newline it will simply be removed.@refill
+The @code{shell} function provides for @code{make} the same facility that
+backquotes (@samp{`}) provide in most shells: it does @dfn{command expansion}.
+This means that it takes as an argument a shell command and expands to the
+output of the command.  The only processing @code{make} does on the result is
+to convert each newline (or carriage-return / newline pair) to a single space.
+If there is a trailing (carriage-return and) newline it will simply be
+removed.
 
 The commands run by calls to the @code{shell} function are run when the
-function calls are expanded (@pxref{Reading Makefiles, , How
-@code{make} Reads a Makefile}).  Because this function involves
-spawning a new shell, you should carefully consider the performance
-implications of using the @code{shell} function within recursively
-expanded variables vs.@: simply expanded variables (@pxref{Flavors, ,The
-Two Flavors of Variables}).
+function calls are expanded (@pxref{Reading Makefiles, , How @code{make} Reads
+a Makefile}).  Because this function involves spawning a new shell, you should
+carefully consider the performance implications of using the @code{shell}
+function within recursively expanded variables vs.@: simply expanded variables
+(@pxref{Flavors, ,The 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
-add recursively-expanded variables to the @code{shell} environment rather than
-fail with an error.
+create its environment.  And so on.  In this obscure case @code{make} will use
+the value of the variable from the environment provided to @code{make}, or
+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
diff --git a/src/expand.c b/src/expand.c
index 2acb32c5..95e66ec6 100644
--- a/src/expand.c
+++ b/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 ("");
     }
 
diff --git a/tests/scripts/functions/shell b/tests/scripts/functions/shell
index a5bd5ddc..e889b5f7 100644
--- a/tests/scripts/functions/shell
+++ b/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