Re: Allow SHELL to include variables and functions.

[Eli Zaretskii]

> From: Paul Smith <address@hidden>
> Cc: address@hidden
> Date: Thu, 11 Oct 2007 01:10:30 -0400
> 
> I feel like I'm definitely missing something.

I should have known better: the chances that _I_ am missing something
are much greater than yours.

The short answer is that my patch had a bug, and my testing was
skewed, so it looked to me as if it worked.  Sorry.  A modified patch
is below; with it, I verified that this trick:

>       SHELL = $($w [$@ ($^) ($?)])$(OLD_SHELL)

works on Windows (albeit with an extra expansion where the values come
out empty; see below).

> Well yes, but it looks to me like you're expanding the value of SHELL
> when it's defined (aren't you?)... so that would mean when make reads
> in:
> 
>       SHELL = $($w [$@ ($^) ($?)])$(OLD_SHELL)
> 
> won't the code you've added expand it right there, as part of the
> do_variable_define()?

Yes, the expansion does happen right there and then.

> And if it's expanded right here then won't the automatic variables like
> $@, $^, and $? not be set yet?

Yes.  That is why the modified patch below installs the original
value, not the expanded one (as the original patch mistakenly did).
IOW, the expansion in do_variable_definition is only to have
find_and_set_default_shell happy, but it's thrown away once we are
past that.  Then, when job.c expands $SHELL, the expansion happens
again, and this time it's in the context of a rule, so the automatic
variables are set, and the magic works.

Btw, I wonder whether I should recursively_expand the value, not just
expand it once, in case the first expansion still yields variables
and/or functions.  WDYT?

Oh, and I think there's no memory leak introduced by the patch,
because do_variable_definition does this just before it returns:

  if (alloc_value)
    free (alloc_value);

The corrected patch follows:


--- variable.c~1        2006-03-09 00:15:08.000000000 +0200
+++ variable.c  2007-10-12 17:12:41.605125000 +0200
@@ -1187,6 +1187,16 @@ do_variable_definition (const struct flo
                                       flocp);
           no_default_sh_exe = 0;
         }
+      else if (find_and_set_default_shell (alloc_value = 
allocated_variable_expand (p)))
+        {
+          v = define_variable_in_set (varname, strlen (varname), p,
+                                      origin, flavor == f_recursive,
+                                      (target_var
+                                       ? current_variable_set_list->set
+                                       : NULL),
+                                      flocp);
+          no_default_sh_exe = 0;
+        }
       else
         v = lookup_variable (varname, strlen (varname));
     }