[Buildroot] conditional assigment check in check-package [was: Re: [PATCH v2, 2/2] package/knock: add knockd option]

[Arnout Vandecappelle]

On 21/10/2021 22:12, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2021-10-21 20:43 +0200, Arnout Vandecappelle spake thusly:
>> On 18/10/2021 22:18, Yann E. MORIN wrote:
>>> Thomas, All,
>>> On 2021-10-18 22:04 +0200, Thomas Petazzoni spake thusly:
>>>> On Sat, 16 Oct 2021 11:32:34 +0200
>>>> Fabrice Fontaine <fontaine.fabrice at gmail.com> wrote:
>>>>> +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y)
>>>>> +KNOCK_DEPENDENCIES = libpcap
>>>>  From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1:
>>>>      Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a
>>>>      conditional is the root of all evil."
>>>> Care to repeat after me ? :-)
>>>
>>> OK, I missed that in my review of v1...
>>> /me hides in shame...
>>>
>>> But seriously though: why is that not caught by check-package?
>>>
>>> Because it is not actually an override: the variable is not yet set
>>> there, so it is valid to unconditionally set it.
>>>
>>> check-package should probably recognise that the variable is
>>> <FOO>_DEPENDENCIES and consider that conditional assignment should
>>> still be forbidden in that case.
>>
>>   I had a look at this, and it is not entirely trivial to test for in check-package.
>>
>>   Basically, this is allowed:
>>
>> ifeq(...)
>> FOO_VAR = ...
>> endif
>> ... $(FOO_VAR) ... # Empty if condition not true
>>
>>   And this as well:
>>
>> ifeq(...)
>> FOO_VAR = ...
>> else
>> FOO_VAR = ...
>> endif
>>
>>   But this is not:
>>
>> ifeq(...)
>> FOO_VAR = ...
>> endif
>>
>> ifeq(...)
>> FOO_VAR = ...
>> endif
>>
>>
>>   So basically:
>> - after a variable is assigned, it can't be assigned to any more in the same
>> conditional branch;
>> - it can be assigned to again in the else branch (including else ifeq constructs);
>> - it can not be assigned to any more after the endif.
>>
>>   In addition, conditions can be nested...
>>
>>   The only way I can think of implementing this is by keeping a stack of
>> conditionally assigned variables. In the else branch, the top of the stack
>> is saved en empties. At the endif, the top of the stack is popped, merged
>> with the saved one, and merged into the new top of the stack.
>>
>>   Wow, by writing it down, I almost implemented it :-) Let's see if I can
>> still find the time to actually do it tonight...
> 
> Sorry, but I think I got lost somewhere...
> 
> Currently, check-package does not whine for:
> 
>      ifeq(...)
>      FOO_VAR = ...
>      endif
> 
> but it does whine for:
> 
>      FOO_VAR = ...
>      ifeq(...)
>      FOO_VAR = ...
>      endif
> 
> And these two behaviours are, I think, correct.
> 
> What I am arguing, is that FOO_DEPENDENCIES should be treated specially,
> where check-package should whine for:
> 
>      ifeq(...)
>      FOO_DEPENDENCIES = ...
>      endif
> 
> as well as for:
> 
>      FOO_DEPENDENCIES = ...
>      ifeq(...)
>      FOO_DEPENDENCIES = ...
>      endif

  Yeah, special-casing DEPENDENCIES is definitely an option.

  My "wanted behaviour" was for other variables which can have the same problem. 
Probably doesn't happen very often though.

  Regards,
  Arnout