diff mbox series

[3/4] BBHandler/ast: Improve addtask handling

Message ID 20240812155335.904273-3-richard.purdie@linuxfoundation.org
State Accepted, archived
Commit 417016b83c21fca7616b2ee768d5d08e1edd1e06
Headers show
Series [1/4] cache: Drop unused function | expand

Commit Message

Richard Purdie Aug. 12, 2024, 3:53 p.m. UTC
The recent addtask improvement to handle comments complicated the regex significantly
and there are already a number of corner cases in that code which aren't handled well.

Instead of trying to complicate the regex further, switch to code logic instead. This
means the following cases are now handled:

* addtask with multiple task names
* addtask with multiple before constraints
* addtask with multiple after constraints

The testcase is updated to match the improvements.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/parse/ast.py                | 26 +++++++-------------
 lib/bb/parse/parse_py/BBHandler.py | 38 +++++++++++++++++-------------
 lib/bb/tests/parse.py              | 15 ++++--------
 3 files changed, 35 insertions(+), 44 deletions(-)

Comments

Peter Kjellerstedt Aug. 12, 2024, 7:56 p.m. UTC | #1
> -----Original Message-----
> From: bitbake-devel@lists.openembedded.org <bitbake-devel@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 12 augusti 2024 17:54
> To: bitbake-devel@lists.openembedded.org
> Subject: [bitbake-devel] [PATCH 3/4] BBHandler/ast: Improve addtask handling
> 
> The recent addtask improvement to handle comments complicated the regex significantly
> and there are already a number of corner cases in that code which aren't handled well.
> 
> Instead of trying to complicate the regex further, switch to code logic instead. This
> means the following cases are now handled:
> 
> * addtask with multiple task names
> * addtask with multiple before constraints
> * addtask with multiple after constraints
> 
> The testcase is updated to match the improvements.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  lib/bb/parse/ast.py                | 26 +++++++-------------
>  lib/bb/parse/parse_py/BBHandler.py | 38 +++++++++++++++++-------------
>  lib/bb/tests/parse.py              | 15 ++++--------
>  3 files changed, 35 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
> index 7581d003fd..001ba8d289 100644
> --- a/lib/bb/parse/ast.py
> +++ b/lib/bb/parse/ast.py
> @@ -240,14 +240,16 @@ class ExportFuncsNode(AstNode):
>                  data.setVar(func, sentinel + "    " + calledfunc + "\n", parsing=True)
> 
>  class AddTaskNode(AstNode):
> -    def __init__(self, filename, lineno, func, before, after):
> +    def __init__(self, filename, lineno, tasks, before, after):
>          AstNode.__init__(self, filename, lineno)
> -        self.func = func
> +        self.tasks = tasks
>          self.before = before
>          self.after = after
> 
>      def eval(self, data):
> -        bb.build.addtask(self.func, self.before, self.after, data)
> +        tasks = self.tasks.split()
> +        for task in tasks:
> +            bb.build.addtask(task, self.before, self.after, data)
> 
>  class DelTaskNode(AstNode):
>      def __init__(self, filename, lineno, tasks):
> @@ -348,21 +350,11 @@ def handlePythonMethod(statements, filename, lineno, funcname, modulename, body)
>  def handleExportFuncs(statements, filename, lineno, m, classname):
>      statements.append(ExportFuncsNode(filename, lineno, m.group(1), classname))
> 
> -def handleAddTask(statements, filename, lineno, m):
> -    func = m.group("func")
> -    before = m.group("before")
> -    after = m.group("after")
> -    if func is None:
> -        return
> -
> -    statements.append(AddTaskNode(filename, lineno, func, before, after))
> -
> -def handleDelTask(statements, filename, lineno, m):
> -    func = m.group(1)
> -    if func is None:
> -        return
> +def handleAddTask(statements, filename, lineno, tasks, before, after):
> +    statements.append(AddTaskNode(filename, lineno, tasks, before, after))
> 
> -    statements.append(DelTaskNode(filename, lineno, func))
> +def handleDelTask(statements, filename, lineno, tasks):
> +    statements.append(DelTaskNode(filename, lineno, tasks))
> 
>  def handleBBHandlers(statements, filename, lineno, m):
>      statements.append(BBHandlerNode(filename, lineno, m.group(1)))
> diff --git a/lib/bb/parse/parse_py/BBHandler.py b/lib/bb/parse/parse_py/BBHandler.py
> index c1653faeee..87c03e9238 100644
> --- a/lib/bb/parse/parse_py/BBHandler.py
> +++ b/lib/bb/parse/parse_py/BBHandler.py
> @@ -23,7 +23,7 @@ __func_start_regexp__    = re.compile(r"(((?P<py>python(?=(\s|\()))|(?P<fr>faker
>  __inherit_regexp__       = re.compile(r"inherit\s+(.+)" )
>  __inherit_def_regexp__   = re.compile(r"inherit_defer\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>(([^#\n]*(?=after))|([^#\n]*))))|(after\s*(?P<after>(([^#\n]*(?=before))|([^#\n]*)))))*(?P<comment>#.*|.*?)")
> +__addtask_regexp__       = re.compile(r"addtask\s+([^#\n]+)(?P<comment>#.*|.*?)")
>  __deltask_regexp__       = re.compile(r"deltask\s+([^#\n]+)(?P<comment>#.*|.*?)")
>  __addhandler_regexp__    = re.compile(r"addhandler\s+(.+)" )
>  __def_regexp__           = re.compile(r"def\s+(\w+).*:" )
> @@ -239,29 +239,35 @@ 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"
> -        taskexpression = s.split()
> -        for word in ('before', 'after'):
> -            if taskexpression.count(word) > 1:
> -                logger.warning("addtask contained multiple '%s' keywords, only one is supported" % word)
> +        after = ""
> +        before = ""
> +        tasks = m.group(1).split(" before ")[0].split(" after ")[0]
> +
> +        for exp in m.group(1).split(" before "):
> +            exp2 = exp.split(" after ")
> +            if len(exp2) > 1:
> +                after = after + " ".join(exp2[1:])
> +
> +        for exp in m.group(1).split(" after "):
> +            exp2 = exp.split(" before ")
> +            if len(exp2) > 1:
> +                before = before + " ".join(exp2[1:])

I found it hard to understand what the code above is actually doing. 
How about something like this instead (untested):

        cur = "tasks"
        tasks = {"tasks": [], "before": [], "after": []}
        for task in m.group(1).split():
            if task in ("before", "after"):
                cur = task
            else:
                # Check and warn for having a task with a keyword as part of the task name
                if any(("%s_" % keyword) in task for keyword in bb.data_smart.__setvar_keyword__):
                    raise ParseError("Task name '%s' contains a keyword which is not recommended/supported.\nPlease rename the task not to include the keyword.\n%s" % (task, ("\n".join(map(str, bb.data_smart.__setvar_keyword__)))), fn)

                tasks[cur].append(task)

        if tasks["tasks"]:
            ast.handleAddTask(statements, fn, lineno, tasks["tasks"], tasks["before"], tasks["after"])
        return

> 
> -        # Check and warn for having task with exprssion as part of task name
> +        # Check and warn for having task with a keyword as part of task name
> +        taskexpression = s.split()
>          for te in taskexpression:
>              if any( ( "%s_" % keyword ) in te for keyword in bb.data_smart.__setvar_keyword__ ):
>                  raise ParseError("Task name '%s' contains a keyword which is not recommended/supported.\nPlease rename the task not to include the keyword.\n%s" % (te, ("\n".join(map(str, bb.data_smart.__setvar_keyword__)))), fn)
> -        ast.handleAddTask(statements, fn, lineno, m)
> +
> +        if tasks is not None:
> +            ast.handleAddTask(statements, fn, lineno, tasks, before, after)
>          return
> 
>      m = __deltask_regexp__.match(s)
>      if m:
> -        ast.handleDelTask(statements, fn, lineno, m)
> +        task = m.group(1)
> +        if task is not None:
> +            ast.handleDelTask(statements, fn, lineno, task)
>          return
> 
>      m = __addhandler_regexp__.match(s)
> diff --git a/lib/bb/tests/parse.py b/lib/bb/tests/parse.py
> index d076fcc208..97df2c4590 100644
> --- a/lib/bb/tests/parse.py
> +++ b/lib/bb/tests/parse.py
> @@ -177,7 +177,7 @@ python () {
> 
>      addtask_deltask = """
>  addtask do_patch after do_foo after do_unpack before do_configure before do_compile
> -addtask do_fetch do_patch
> +addtask do_fetch2 do_patch2
> 
>  addtask do_myplaintask
>  addtask do_myplaintask2
> @@ -194,18 +194,11 @@ deltask do_fetch ${MYVAR} ${EMPTYVAR}
>  deltask ${EMPTYVAR}
>  """
>      def test_parse_addtask_deltask(self):
> -        import sys
> 
> -        with self.assertLogs() as logs:
> -            f = self.parsehelper(self.addtask_deltask)
> -            d = bb.parse.handle(f.name, self.d)['']
> +        f = self.parsehelper(self.addtask_deltask)
> +        d = bb.parse.handle(f.name, self.d)['']
> 
> -        output = "".join(logs.output)
> -        self.assertTrue("addtask contained multiple 'before' keywords" in output)
> -        self.assertTrue("addtask contained multiple 'after' keywords" in output)
> -        self.assertTrue('addtask ignored: " do_patch"' in output)
> -        self.assertEqual(['do_myplaintask', 'do_mytask', 'do_mytask2'], d.getVar("__BBTASKS"))
> -        #self.assertTrue('dependent task do_foo for do_patch does not exist' in output)
> +        self.assertEqual(['do_fetch2', 'do_patch2', 'do_myplaintask', 'do_mytask', 'do_mytask2'], d.getVar("__BBTASKS"))
> 
>      broken_multiline_comment = """
>  # First line of comment \\

//Peter
Richard Purdie Aug. 12, 2024, 10:26 p.m. UTC | #2
On Mon, 2024-08-12 at 19:56 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: bitbake-devel@lists.openembedded.org <bitbake-devel@lists.openembedded.org> On Behalf Of Richard Purdie
> > Sent: den 12 augusti 2024 17:54
> > To: bitbake-devel@lists.openembedded.org
> > Subject: [bitbake-devel] [PATCH 3/4] BBHandler/ast: Improve addtask handling
> > 
> > The recent addtask improvement to handle comments complicated the regex significantly
> > and there are already a number of corner cases in that code which aren't handled well.
> > 
> > Instead of trying to complicate the regex further, switch to code logic instead. This
> > means the following cases are now handled:
> > 
> > * addtask with multiple task names
> > * addtask with multiple before constraints
> > * addtask with multiple after constraints
> > 
> > The testcase is updated to match the improvements.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  lib/bb/parse/ast.py                | 26 +++++++-------------
> >  lib/bb/parse/parse_py/BBHandler.py | 38 +++++++++++++++++-------------
> >  lib/bb/tests/parse.py              | 15 ++++--------
> >  3 files changed, 35 insertions(+), 44 deletions(-)
> > 
> > diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
> > index 7581d003fd..001ba8d289 100644
> > --- a/lib/bb/parse/ast.py
> > +++ b/lib/bb/parse/ast.py
> > @@ -240,14 +240,16 @@ class ExportFuncsNode(AstNode):
> >                  data.setVar(func, sentinel + "    " + calledfunc + "\n", parsing=True)
> > 
> >  class AddTaskNode(AstNode):
> > -    def __init__(self, filename, lineno, func, before, after):
> > +    def __init__(self, filename, lineno, tasks, before, after):
> >          AstNode.__init__(self, filename, lineno)
> > -        self.func = func
> > +        self.tasks = tasks
> >          self.before = before
> >          self.after = after
> > 
> >      def eval(self, data):
> > -        bb.build.addtask(self.func, self.before, self.after, data)
> > +        tasks = self.tasks.split()
> > +        for task in tasks:
> > +            bb.build.addtask(task, self.before, self.after, data)
> > 
> >  class DelTaskNode(AstNode):
> >      def __init__(self, filename, lineno, tasks):
> > @@ -348,21 +350,11 @@ def handlePythonMethod(statements, filename, lineno, funcname, modulename, body)
> >  def handleExportFuncs(statements, filename, lineno, m, classname):
> >      statements.append(ExportFuncsNode(filename, lineno, m.group(1), classname))
> > 
> > -def handleAddTask(statements, filename, lineno, m):
> > -    func = m.group("func")
> > -    before = m.group("before")
> > -    after = m.group("after")
> > -    if func is None:
> > -        return
> > -
> > -    statements.append(AddTaskNode(filename, lineno, func, before, after))
> > -
> > -def handleDelTask(statements, filename, lineno, m):
> > -    func = m.group(1)
> > -    if func is None:
> > -        return
> > +def handleAddTask(statements, filename, lineno, tasks, before, after):
> > +    statements.append(AddTaskNode(filename, lineno, tasks, before, after))
> > 
> > -    statements.append(DelTaskNode(filename, lineno, func))
> > +def handleDelTask(statements, filename, lineno, tasks):
> > +    statements.append(DelTaskNode(filename, lineno, tasks))
> > 
> >  def handleBBHandlers(statements, filename, lineno, m):
> >      statements.append(BBHandlerNode(filename, lineno, m.group(1)))
> > diff --git a/lib/bb/parse/parse_py/BBHandler.py b/lib/bb/parse/parse_py/BBHandler.py
> > index c1653faeee..87c03e9238 100644
> > --- a/lib/bb/parse/parse_py/BBHandler.py
> > +++ b/lib/bb/parse/parse_py/BBHandler.py
> > @@ -23,7 +23,7 @@ __func_start_regexp__    = re.compile(r"(((?P<py>python(?=(\s|\()))|(?P<fr>faker
> >  __inherit_regexp__       = re.compile(r"inherit\s+(.+)" )
> >  __inherit_def_regexp__   = re.compile(r"inherit_defer\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>(([^#\n]*(?=after))|([^#\n]*))))|(after\s*(?P<after>(([^#\n]*(?=before))|([^#\n]*)))))*(?P<comment>#.*|.*?)")
> > +__addtask_regexp__       = re.compile(r"addtask\s+([^#\n]+)(?P<comment>#.*|.*?)")
> >  __deltask_regexp__       = re.compile(r"deltask\s+([^#\n]+)(?P<comment>#.*|.*?)")
> >  __addhandler_regexp__    = re.compile(r"addhandler\s+(.+)" )
> >  __def_regexp__           = re.compile(r"def\s+(\w+).*:" )
> > @@ -239,29 +239,35 @@ 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"
> > -        taskexpression = s.split()
> > -        for word in ('before', 'after'):
> > -            if taskexpression.count(word) > 1:
> > -                logger.warning("addtask contained multiple '%s' keywords, only one is supported" % word)
> > +        after = ""
> > +        before = ""
> > +        tasks = m.group(1).split(" before ")[0].split(" after ")[0]
> > +
> > +        for exp in m.group(1).split(" before "):
> > +            exp2 = exp.split(" after ")
> > +            if len(exp2) > 1:
> > +                after = after + " ".join(exp2[1:])
> > +
> > +        for exp in m.group(1).split(" after "):
> > +            exp2 = exp.split(" before ")
> > +            if len(exp2) > 1:
> > +                before = before + " ".join(exp2[1:])
> 
> I found it hard to understand what the code above is actually doing. 
> How about something like this instead (untested):
> 
>         cur = "tasks"
>         tasks = {"tasks": [], "before": [], "after": []}
>         for task in m.group(1).split():
>             if task in ("before", "after"):
>                 cur = task
>             else:
>                 # Check and warn for having a task with a keyword as part of the task name
>                 if any(("%s_" % keyword) in task for keyword in bb.data_smart.__setvar_keyword__):
>                     raise ParseError("Task name '%s' contains a keyword which is not recommended/supported.\nPlease rename the task not to include the keyword.\n%s" % (task, ("\n".join(map(str, bb.data_smart.__setvar_keyword__)))), fn)
> 
>                 tasks[cur].append(task)
> 
>         if tasks["tasks"]:
>             ast.handleAddTask(statements, fn, lineno, tasks["tasks"], tasks["before"], tasks["after"])
>         return

This isn't entirely straightforward. I was trying to split only on
"before" and "after" and not on space characters. This leaves the
expansion and splitting as late as possible. I appreciate it still
isn't as late as it could be but it seemed to be a right step in the
direction of allowing expressions which deltask does but addtask does
not. The change brings them more into parity.

Cheers,

Richard
Peter Kjellerstedt Aug. 13, 2024, 1:43 p.m. UTC | #3
> -----Original Message-----
> From: bitbake-devel@lists.openembedded.org <bitbake-
> devel@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 13 augusti 2024 00:26
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; bitbake-
> devel@lists.openembedded.org
> Subject: Re: [bitbake-devel] [PATCH 3/4] BBHandler/ast: Improve addtask
> handling
> 
> On Mon, 2024-08-12 at 19:56 +0000, Peter Kjellerstedt wrote:
> > > -----Original Message-----
> > > From: bitbake-devel@lists.openembedded.org <bitbake-
> devel@lists.openembedded.org> On Behalf Of Richard Purdie
> > > Sent: den 12 augusti 2024 17:54
> > > To: bitbake-devel@lists.openembedded.org
> > > Subject: [bitbake-devel] [PATCH 3/4] BBHandler/ast: Improve addtask
> handling
> > >
> > > The recent addtask improvement to handle comments complicated the
> regex significantly
> > > and there are already a number of corner cases in that code which
> aren't handled well.
> > >
> > > Instead of trying to complicate the regex further, switch to code
> logic instead. This
> > > means the following cases are now handled:
> > >
> > > * addtask with multiple task names
> > > * addtask with multiple before constraints
> > > * addtask with multiple after constraints
> > >
> > > The testcase is updated to match the improvements.
> > >
> > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > > ---
> > >  lib/bb/parse/ast.py                | 26 +++++++-------------
> > >  lib/bb/parse/parse_py/BBHandler.py | 38 +++++++++++++++++------------
> -
> > >  lib/bb/tests/parse.py              | 15 ++++--------
> > >  3 files changed, 35 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
> > > index 7581d003fd..001ba8d289 100644
> > > --- a/lib/bb/parse/ast.py
> > > +++ b/lib/bb/parse/ast.py
> > > @@ -240,14 +240,16 @@ class ExportFuncsNode(AstNode):
> > >                  data.setVar(func, sentinel + "    " + calledfunc +
> "\n", parsing=True)
> > >
> > >  class AddTaskNode(AstNode):
> > > -    def __init__(self, filename, lineno, func, before, after):
> > > +    def __init__(self, filename, lineno, tasks, before, after):
> > >          AstNode.__init__(self, filename, lineno)
> > > -        self.func = func
> > > +        self.tasks = tasks
> > >          self.before = before
> > >          self.after = after
> > >
> > >      def eval(self, data):
> > > -        bb.build.addtask(self.func, self.before, self.after, data)
> > > +        tasks = self.tasks.split()
> > > +        for task in tasks:
> > > +            bb.build.addtask(task, self.before, self.after, data)
> > >
> > >  class DelTaskNode(AstNode):
> > >      def __init__(self, filename, lineno, tasks):
> > > @@ -348,21 +350,11 @@ def handlePythonMethod(statements, filename,
> lineno, funcname, modulename, body)
> > >  def handleExportFuncs(statements, filename, lineno, m, classname):
> > >      statements.append(ExportFuncsNode(filename, lineno, m.group(1),
> classname))
> > >
> > > -def handleAddTask(statements, filename, lineno, m):
> > > -    func = m.group("func")
> > > -    before = m.group("before")
> > > -    after = m.group("after")
> > > -    if func is None:
> > > -        return
> > > -
> > > -    statements.append(AddTaskNode(filename, lineno, func, before,
> after))
> > > -
> > > -def handleDelTask(statements, filename, lineno, m):
> > > -    func = m.group(1)
> > > -    if func is None:
> > > -        return
> > > +def handleAddTask(statements, filename, lineno, tasks, before,
> after):
> > > +    statements.append(AddTaskNode(filename, lineno, tasks, before,
> after))
> > >
> > > -    statements.append(DelTaskNode(filename, lineno, func))
> > > +def handleDelTask(statements, filename, lineno, tasks):
> > > +    statements.append(DelTaskNode(filename, lineno, tasks))
> > >
> > >  def handleBBHandlers(statements, filename, lineno, m):
> > >      statements.append(BBHandlerNode(filename, lineno, m.group(1)))
> > > diff --git a/lib/bb/parse/parse_py/BBHandler.py
> b/lib/bb/parse/parse_py/BBHandler.py
> > > index c1653faeee..87c03e9238 100644
> > > --- a/lib/bb/parse/parse_py/BBHandler.py
> > > +++ b/lib/bb/parse/parse_py/BBHandler.py
> > > @@ -23,7 +23,7 @@ __func_start_regexp__    =
> re.compile(r"(((?P<py>python(?=(\s|\()))|(?P<fr>faker
> > >  __inherit_regexp__       = re.compile(r"inherit\s+(.+)" )
> > >  __inherit_def_regexp__   = re.compile(r"inherit_defer\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>(([^#\n]*(?=a
> fter))|([^#\n]*))))|(after\s*(?P<after>(([^#\n]*(?=before))|([^#\n]*)))))*
> (?P<comment>#.*|.*?)")
> > > +__addtask_regexp__       =
> re.compile(r"addtask\s+([^#\n]+)(?P<comment>#.*|.*?)")
> > >  __deltask_regexp__       =
> re.compile(r"deltask\s+([^#\n]+)(?P<comment>#.*|.*?)")
> > >  __addhandler_regexp__    = re.compile(r"addhandler\s+(.+)" )
> > >  __def_regexp__           = re.compile(r"def\s+(\w+).*:" )
> > > @@ -239,29 +239,35 @@ 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"
> > > -        taskexpression = s.split()
> > > -        for word in ('before', 'after'):
> > > -            if taskexpression.count(word) > 1:
> > > -                logger.warning("addtask contained multiple '%s'
> keywords, only one is supported" % word)
> > > +        after = ""
> > > +        before = ""
> > > +        tasks = m.group(1).split(" before ")[0].split(" after ")[0]
> > > +
> > > +        for exp in m.group(1).split(" before "):
> > > +            exp2 = exp.split(" after ")
> > > +            if len(exp2) > 1:
> > > +                after = after + " ".join(exp2[1:])
> > > +
> > > +        for exp in m.group(1).split(" after "):
> > > +            exp2 = exp.split(" before ")
> > > +            if len(exp2) > 1:
> > > +                before = before + " ".join(exp2[1:])
> >
> > I found it hard to understand what the code above is actually doing.
> > How about something like this instead (untested):
> >
> >         cur = "tasks"
> >         tasks = {"tasks": [], "before": [], "after": []}
> >         for task in m.group(1).split():
> >             if task in ("before", "after"):
> >                 cur = task
> >             else:
> >                 # Check and warn for having a task with a keyword as
> part of the task name
> >                 if any(("%s_" % keyword) in task for keyword in
> bb.data_smart.__setvar_keyword__):
> >                     raise ParseError("Task name '%s' contains a keyword
> which is not recommended/supported.\nPlease rename the task not to include
> the keyword.\n%s" % (task, ("\n".join(map(str,
> bb.data_smart.__setvar_keyword__)))), fn)
> >
> >                 tasks[cur].append(task)
> >
> >         if tasks["tasks"]:
> >             ast.handleAddTask(statements, fn, lineno, tasks["tasks"],
> tasks["before"], tasks["after"])
> >         return
> 
> This isn't entirely straightforward. I was trying to split only on
> "before" and "after" and not on space characters. This leaves the
> expansion and splitting as late as possible. I appreciate it still
> isn't as late as it could be but it seemed to be a right step in the
> direction of allowing expressions which deltask does but addtask does
> not. The change brings them more into parity.
> 
> Cheers,
> 
> Richard

Ok, I understand. It might be a good idea to add a comment in the code 
along the above to explain why the code is doing what is doing.

//Peter
diff mbox series

Patch

diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
index 7581d003fd..001ba8d289 100644
--- a/lib/bb/parse/ast.py
+++ b/lib/bb/parse/ast.py
@@ -240,14 +240,16 @@  class ExportFuncsNode(AstNode):
                 data.setVar(func, sentinel + "    " + calledfunc + "\n", parsing=True)
 
 class AddTaskNode(AstNode):
-    def __init__(self, filename, lineno, func, before, after):
+    def __init__(self, filename, lineno, tasks, before, after):
         AstNode.__init__(self, filename, lineno)
-        self.func = func
+        self.tasks = tasks
         self.before = before
         self.after = after
 
     def eval(self, data):
-        bb.build.addtask(self.func, self.before, self.after, data)
+        tasks = self.tasks.split()
+        for task in tasks:
+            bb.build.addtask(task, self.before, self.after, data)
 
 class DelTaskNode(AstNode):
     def __init__(self, filename, lineno, tasks):
@@ -348,21 +350,11 @@  def handlePythonMethod(statements, filename, lineno, funcname, modulename, body)
 def handleExportFuncs(statements, filename, lineno, m, classname):
     statements.append(ExportFuncsNode(filename, lineno, m.group(1), classname))
 
-def handleAddTask(statements, filename, lineno, m):
-    func = m.group("func")
-    before = m.group("before")
-    after = m.group("after")
-    if func is None:
-        return
-
-    statements.append(AddTaskNode(filename, lineno, func, before, after))
-
-def handleDelTask(statements, filename, lineno, m):
-    func = m.group(1)
-    if func is None:
-        return
+def handleAddTask(statements, filename, lineno, tasks, before, after):
+    statements.append(AddTaskNode(filename, lineno, tasks, before, after))
 
-    statements.append(DelTaskNode(filename, lineno, func))
+def handleDelTask(statements, filename, lineno, tasks):
+    statements.append(DelTaskNode(filename, lineno, tasks))
 
 def handleBBHandlers(statements, filename, lineno, m):
     statements.append(BBHandlerNode(filename, lineno, m.group(1)))
diff --git a/lib/bb/parse/parse_py/BBHandler.py b/lib/bb/parse/parse_py/BBHandler.py
index c1653faeee..87c03e9238 100644
--- a/lib/bb/parse/parse_py/BBHandler.py
+++ b/lib/bb/parse/parse_py/BBHandler.py
@@ -23,7 +23,7 @@  __func_start_regexp__    = re.compile(r"(((?P<py>python(?=(\s|\()))|(?P<fr>faker
 __inherit_regexp__       = re.compile(r"inherit\s+(.+)" )
 __inherit_def_regexp__   = re.compile(r"inherit_defer\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>(([^#\n]*(?=after))|([^#\n]*))))|(after\s*(?P<after>(([^#\n]*(?=before))|([^#\n]*)))))*(?P<comment>#.*|.*?)")
+__addtask_regexp__       = re.compile(r"addtask\s+([^#\n]+)(?P<comment>#.*|.*?)")
 __deltask_regexp__       = re.compile(r"deltask\s+([^#\n]+)(?P<comment>#.*|.*?)")
 __addhandler_regexp__    = re.compile(r"addhandler\s+(.+)" )
 __def_regexp__           = re.compile(r"def\s+(\w+).*:" )
@@ -239,29 +239,35 @@  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"
-        taskexpression = s.split()
-        for word in ('before', 'after'):
-            if taskexpression.count(word) > 1:
-                logger.warning("addtask contained multiple '%s' keywords, only one is supported" % word)
+        after = ""
+        before = ""
+        tasks = m.group(1).split(" before ")[0].split(" after ")[0]
+
+        for exp in m.group(1).split(" before "):
+            exp2 = exp.split(" after ")
+            if len(exp2) > 1:
+                after = after + " ".join(exp2[1:])
+
+        for exp in m.group(1).split(" after "):
+            exp2 = exp.split(" before ")
+            if len(exp2) > 1:
+                before = before + " ".join(exp2[1:])
 
-        # Check and warn for having task with exprssion as part of task name
+        # Check and warn for having task with a keyword as part of task name
+        taskexpression = s.split()
         for te in taskexpression:
             if any( ( "%s_" % keyword ) in te for keyword in bb.data_smart.__setvar_keyword__ ):
                 raise ParseError("Task name '%s' contains a keyword which is not recommended/supported.\nPlease rename the task not to include the keyword.\n%s" % (te, ("\n".join(map(str, bb.data_smart.__setvar_keyword__)))), fn)
-        ast.handleAddTask(statements, fn, lineno, m)
+
+        if tasks is not None:
+            ast.handleAddTask(statements, fn, lineno, tasks, before, after)
         return
 
     m = __deltask_regexp__.match(s)
     if m:
-        ast.handleDelTask(statements, fn, lineno, m)
+        task = m.group(1)
+        if task is not None:
+            ast.handleDelTask(statements, fn, lineno, task)
         return
 
     m = __addhandler_regexp__.match(s)
diff --git a/lib/bb/tests/parse.py b/lib/bb/tests/parse.py
index d076fcc208..97df2c4590 100644
--- a/lib/bb/tests/parse.py
+++ b/lib/bb/tests/parse.py
@@ -177,7 +177,7 @@  python () {
 
     addtask_deltask = """
 addtask do_patch after do_foo after do_unpack before do_configure before do_compile
-addtask do_fetch do_patch
+addtask do_fetch2 do_patch2
 
 addtask do_myplaintask
 addtask do_myplaintask2
@@ -194,18 +194,11 @@  deltask do_fetch ${MYVAR} ${EMPTYVAR}
 deltask ${EMPTYVAR}
 """
     def test_parse_addtask_deltask(self):
-        import sys
 
-        with self.assertLogs() as logs:
-            f = self.parsehelper(self.addtask_deltask)
-            d = bb.parse.handle(f.name, self.d)['']
+        f = self.parsehelper(self.addtask_deltask)
+        d = bb.parse.handle(f.name, self.d)['']
 
-        output = "".join(logs.output)
-        self.assertTrue("addtask contained multiple 'before' keywords" in output)
-        self.assertTrue("addtask contained multiple 'after' keywords" in output)
-        self.assertTrue('addtask ignored: " do_patch"' in output)
-        self.assertEqual(['do_myplaintask', 'do_mytask', 'do_mytask2'], d.getVar("__BBTASKS"))
-        #self.assertTrue('dependent task do_foo for do_patch does not exist' in output)
+        self.assertEqual(['do_fetch2', 'do_patch2', 'do_myplaintask', 'do_mytask', 'do_mytask2'], d.getVar("__BBTASKS"))
 
     broken_multiline_comment = """
 # First line of comment \\