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

[Arnout Vandecappelle]

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


  Regards,
  Arnout