[bitbake-devel] [PATCH 1/2] bitbake: BBHandler: Fix addtask and deltask

[Robert Yang]

On 4/25/19 6:48 PM, Richard Purdie wrote:
> On Thu, 2019-04-25 at 18:00 +0800, Robert Yang wrote:
>> The following commands are not supported, but they were ignored
>> silently, that
>> may suprise users:
>>
>> * addtask task1 task2
>>    task2 is ignored
>>
>> * addtask task1 before task2 before task3
>>    Should be: addtask task1 before task2 task3
>>
>> * addtask task1 after task2 after task3
>>    Should be: addtask task1 after task2 task3
>>
>> * deltask task1 task2
>>    task2 is ignore
>>
>> This patch can check and warn for them.
>>
>> [YOCTO #13282]
>>
>> Signed-off-by: Robert Yang <liezhi.yang at windriver.com>
>> ---
>>   bitbake/lib/bb/parse/parse_py/BBHandler.py | 25
>> ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py
>> b/bitbake/lib/bb/parse/parse_py/BBHandler.py
>> index 9dba5f2..4d90ff0 100644
>> --- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
>> +++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
>> @@ -42,7 +42,7 @@ __func_start_regexp__    =
>> re.compile(r"(((?P<py>python)|(?P<fr>fakeroot))\s*)*(
>>   __inherit_regexp__       = re.compile(r"inherit\s+(.+)" )
>>   __export_func_regexp__   = re.compile(r"EXPORT_FUNCTIONS\s+(.+)" )
>>   __addtask_regexp__       =
>> re.compile(r"addtask\s+(?P<func>\w+)\s*((before\s*(?P<before>((.*(?=a
>> fter))|(.*))))|(after\s*(?P<after>((.*(?=before))|(.*)))))*")
>> -__deltask_regexp__       = re.compile(r"deltask\s+(?P<func>\w+)")
>> +__deltask_regexp__       =
>> re.compile(r"deltask\s+(?P<func>\w+)(?P<ignores>.*)")
>>   __addhandler_regexp__    = re.compile(r"addhandler\s+(.+)" )
>>   __def_regexp__           = re.compile(r"def\s+(\w+).*:" )
>>   __python_func_regexp__   = re.compile(r"(\s+.*)|(^$)|(^#)" )
>> @@ -236,11 +236,34 @@ def feeder(lineno, s, fn, root, statements,
>> eof=False):
>>   
>>       m = __addtask_regexp__.match(s)
>>       if m:
>> +        if len(m.group().split()) == 2:
>> +            # Check and warn for "addtask task1 task2"
>> +            m2 = re.match(r"addtask\s+(?P<func>\w+)(?P<ignores>.*)",
>> s)
>> +            if m2 and m2.group('ignores'):
>> +                logger.warning('addtask ignored: "%s"' %
>> m2.group('ignores'))
>> +
>> +        # Check and warn for "addtask task1 before task2 before
>> task3", the
>> +        # similar to "after"
>> +        dup_after = 0
>> +        dup_before = 0
>> +        for tmp in s.split():
>> +            if 'before' == tmp:
>> +                dup_before += 1
>> +            if 'after' == tmp:
>> +                dup_after += 1
>> +        if dup_after > 1:
> 
> taskexpression = s.split()
> if taskexpression.count("after") > 1:
>     
> logger.warning("X")
> if taskexpression.count("before") > 1:
>     
> logger.warning("Y")
> 
>> +            logger.warning('addtask found more than 1 keyword
>> "after": "%s"' % s)
> 
> How about:
> 
> addtask contained multiple 'after' keywords, only one is supported
> 
>> +        if dup_before > 1:
>> +            logger.warning('addtask found more than 1 keyword
>> "before": "%s"' % s)
> 
> and:
> 
> addtask contained multiple 'before' keywords, only one is supported

Thanks for all the replies above, I will fix them.

> 
>>           ast.handleAddTask(statements, fn, lineno, m)
>>           return
>>   
>>       m = __deltask_regexp__.match(s)
>>       if m:
>> +        # Check and warn "for deltask task1 task2"
>> +        if m.group('ignores'):
>> +            logger.warning('deltask ignored: "%s"' %
>> m.group('ignores'))
> 
> deltask listing multiple tasks is not supported
> 
> I did wonder if we should support multiple tasks for deltask?

Yes, that's a good idea, but if we support "deltask multiple tasks",
I think that we should also support "addtask multiple tasks" since
they are a pair, but I have other concerns on addtask, for example,
the following code is hard to read and maintain:

__addtask_regexp__       = 
re.compile(r"addtask\s+(?P<func>\w+)\s*((before\s*(?P<before>((.*(?=after))|(.*))))|(after\s*(?P<after>((.*(?=before))|(.*)))))*")

I think that we need break it into small pieces. so I gave it up with these
concerns which looks risky.

> 
> Also, we might want to consider adding something to lib/bb/tests/ for
> this?

Thanks, I will add it.

// Robert

> 
> Cheers,
> 
> Richard
> 
>