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

diff --git a/NEWS b/NEWS
index 16c670593ad1374c070a72951d122e7f17a241d5..4d6358209e727ab0266739c24058162d309bb3d8 100644 (file)
--- 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 6fb8bb8c9f715bc7ebcef62f017b981ed9c16049..1598ef65172d269002d1c44e7e738b7bef5d26f6 100644 (file)
--- 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 2acb32c5445d9eb9f0a478aa83c3e89a1b8c2e54..95e66ec60d8cc8d155c7d9c0aa3721420e059373 100644 (file)
--- 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 a5bd5ddcffb866999bb44d2a1626338196c8db4c..e889b5f74e5793413a9e7541795557e156742344 100644 (file)
--- 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