Re: [PATCH v4 03/31] kconfig: reference environment variables directly and remove 'option env='

[Ulf Magnusson]

On Mon, May 21, 2018 at 1:06 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> On Mon, May 21, 2018 at 6:43 AM, Masahiro Yamada
> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> Hi.
>>
>>
>>
>> 2018-05-21 0:46 GMT+09:00 Ulf Magnusson <ulfalizer@xxxxxxxxx>:
>>
>>> s/environments/environment variables/
>>
>> Will fix.
>>
>>
>>>
>>>> +        * They will be written out to include/config/auto.conf.cmd
>>>> +        */
>>>> +       env_add(name, value);
>>>> +
>>>> +       return xstrdup(value);
>>>> +}
>>>> +
>>>> +void env_write_dep(FILE *f, const char *autoconfig_name)
>>>> +{
>>>> +       struct env *e, *tmp;
>>>> +
>>>> +       list_for_each_entry_safe(e, tmp, &env_list, node) {
>>>> +               fprintf(f, "ifneq \"$(%s)\" \"%s\"\n", e->name, e->value);
>>>> +               fprintf(f, "%s: FORCE\n", autoconfig_name);
>>>> +               fprintf(f, "endif\n");
>>>> +               env_del(e);
>>>> +       }
>>>> +}
>>>> +
>>>> +static char *eval_clause(const char *in)
>>>> +{
>>>> +       char *res, *name;
>>>> +
>>>> +       /*
>>>> +        * Returns an empty string because '$()' should be evaluated
>>>> +        * to a null string.
>>>> +        */
>>>
>>> Do you know of cases where this is more useful than erroring out?
>>>
>>> Not saying it doesn't make sense. Just curious.
>>
>>
>> I just followed how Make works.
>>
>> Anyway, eval_clause() will return null string for null input.
>> I will remove that hunk.
>>
>>
>>
>>
>>>> +       if (!*in)
>>>> +               return xstrdup("");
>>>> +
>>>> +       name = expand_string(in);
>>>> +
>>>> +       res = env_expand(name);
>>>> +       free(name);
>>>> +
>>>> +       return res;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Expand a string that follows '$'
>>>> + *
>>>> + * For example, if the input string is
>>>> + *     ($(FOO)$($(BAR)))$(BAZ)
>>>> + * this helper evaluates
>>>> + *     $($(FOO)$($(BAR)))
>>>> + * and returns the resulted string, then updates 'str' to point to the next
>>>
>>> s/resulted/resulting/
>>>
>>> Maybe something like this would make the behavior a bit clearer:
>>>
>>>   ...and returns a new string containing the expansion, also advancing
>>>   'str' to point to the next character after... (note that this function does
>>>   a recursive expansion) ...
>>
>>
>> Is this OK?
>>
>> /*
>>  * Expand a string that follows '$'
>>  *
>>  * For example, if the input string is
>>  *     ($(FOO)$($(BAR)))$(BAZ)
>>  * this helper evaluates
>>  *     $($(FOO)$($(BAR)))
>>  * and returns a new string containing the expansion (note that the string is
>>  * recursively expanded), also advancing 'str' to point to the next character
>>  * after the corresponding closing parenthesis, in this case, *str will be
>>  *     $(BAR)
>>  */
>
> Looks fine to me.
>
>>
>>
>>
>>>> + * character after the corresponding closing parenthesis, in this case, *str
>>>> + * will be
>>>> + *     $(BAR)
>>>> + */
>>>> +char *expand_dollar(const char **str)
>>>> +{
>>>> +       const char *p = *str;
>>>> +       const char *q;
>>>> +       char *tmp, *out;
>>>> +       int nest = 0;
>>>> +
>>>> +       /* '$$' represents an escaped '$' */
>>>> +       if (*p == '$') {
>>>> +               *str = p + 1;
>>>> +               return xstrdup("$");
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * Kconfig does not support single-letter variable as in $A
>>>> +        * or curly braces as in ${CC}.
>>>> +        */
>>>> +       if (*p != '(')
>>>> +               pperror("syntax error: '$' not followed by '('", p);
>>>
>>> Could say "not followed by '(' by or '$'".
>>
>> Will do.
>>
>>
>>>> +
>>>> +       p++;
>>>> +       q = p;
>>>> +       while (*q) {
>>>> +               if (*q == '(') {
>>>> +                       nest++;
>>>> +               } else if (*q == ')') {
>>>> +                       if (nest-- == 0)
>>>> +                               break;
>>>> +               }
>>>> +               q++;
>>>> +       }
>>>> +
>>>> +       if (!*q)
>>>> +               pperror("unterminated reference to '%s': missing ')'", p);
>>>> +
>>>> +       tmp = xmalloc(q - p + 1);
>>>> +       memcpy(tmp, p, q - p);
>>>> +       tmp[q - p] = 0;
>>>> +       *str = q + 1;
>>>> +       out = eval_clause(tmp);
>>>> +       free(tmp);
>>>> +
>>>> +       return out;
>>>
>>> This might be a bit clearer, since the 'str' update and the expansion
>>> are independent:
>>
>> Indeed, will do.
>>
>>>
>>>   /* Advance 'str' to after the expanded initial portion of the string */
>>>   *str = q + 1;
>>>
>>>   /* Save the "FOO" part of "(FOO)" into 'tmp' and expand it recursively */
>>>   tmp = xmalloc(q - p + 1);
>>>   memcpy(tmp, p, q - p);
>>>   tmp[q - p] = '\0';
>>>   out = eval_clause(tmp);
>>>   free(tmp);
>>>
>>>   return out;
>>>
>>> ...or switched around, but thought putting the 'str' bit first might
>>> emphasize that it isn't modified.
>>
>>
>> I prefer advancing the pointer at last.
>
> Yeah, it's a bit less weird in a way...
>
>>
>>
>>
>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * Expand variables in the given string.  Undefined variables
>>>> + * expand to an empty string.
>>>
>>> I wonder what the tradeoffs are vs. erroring out here (or leaving
>>> undefined variables as-is).
>>
>>
>> I want to stick to the Make-like behavior here and exploit it in some cases.
>> We can set some variables in some arch/*/Kconfig,
>> but unset in the others.
>>
>>
>>
>>
>> In some arch/*/Kconfig:
>>
>> min-gcc-version := 40900
>>
>>
>>
>>
>> In init/Kconfig:
>>
>> # GCC 4.5 is required to build Linux Kernel.
>> # Some architectures may override it (from arch/*/Kconfig) for their
>> requirement.
>> min-gcc-version := $(if,$(min-gcc-version),$(min-gcc-version),40500)
>>
>> ... check the gcc version, then show error
>> if it is older than $(min-gcc-version).
>
> OK, makes sense. Fine by me.
>
>>>> + * The returned string must be freed when done.
>>>> + */
>>>> +char *expand_string(const char *in)
>>>> +{
>>>> +       const char *prev_in, *p;
>>>> +       char *new, *out;
>>>> +       size_t outlen;
>>>> +
>>>> +       out = xmalloc(1);
>>>> +       *out = 0;
>>>> +
>>>> +       while ((p = strchr(in, '$'))) {
>>>> +               prev_in = in;
>>>> +               in = p + 1;
>>>> +               new = expand_dollar(&in);
>>>> +               outlen = strlen(out) + (p - prev_in) + strlen(new) + 1;
>>>> +               out = xrealloc(out, outlen);
>>>> +               strncat(out, prev_in, p - prev_in);
>>>> +               strcat(out, new);
>>>> +               free(new);
>>>
>>> Some code duplication with expand_one_token() here. Do you think
>>> something could be factored out/reorganized?
>>
>>
>> Yes.  For example, the following would work.
>> If you prefer that, I can update it in v5.
>>
>>
>>
>>
>> static char *__expand_string(const char **str, bool (*is_end)(const char *))
>> {
>>         const char *in, *prev_in, *p;
>>         char *new, *out;
>>         size_t outlen;
>>
>>         out = xmalloc(1);
>>         *out = 0;
>>
>>         p = in = *str;
>>
>>         while (1) {
>>                 if (*p == '$') {
>>                         prev_in = in;
>>                         in = p + 1;
>>                         new = expand_dollar(&in);
>>                         outlen = strlen(out) + (p - prev_in) + strlen(new) + 1;
>>                         out = xrealloc(out, outlen);
>>                         strncat(out, prev_in, p - prev_in);
>>                         strcat(out, new);
>>                         free(new);
>>                         p = in;
>>                         continue;
>>                 }
>>
>>                 if (is_end(p))
>>                         break;
>>
>>                 p++;
>>         }
>>
>>         outlen = strlen(out) + (p - in) + 1;
>>         out = xrealloc(out, outlen);
>>         strncat(out, in, p - in);
>>
>>         *str = p;
>>
>>         return out;
>> }
>>
>> static bool is_end_of_str(const char *s)
>> {
>>         return !*s;
>> }
>>
>> char *expand_string(const char *in)
>> {
>>         return __expand_string(&in, is_end_of_str);
>> }
>>
>> static bool is_end_of_token(const char *s)
>> {
>>         return !(isalnum(*s) || *s == '_' || *s == '-' || *s == '.' ||
>> *s == '/');
>> }
>>
>> char *expand_one_token(const char **str)
>> {
>>         return __expand_string(str, is_end_of_token);
>> }
>
> Yep, something like that would be nicer I think.
>
> This variant might work too (untested):
>
>   dollar_i = p;

dollar_p makes more sense.

Cheers,
Ulf