Re: [gentoo-portage-dev] [PATCH] emaint: exit with non-zero status code when module fails (bug 567478)

[Alexandru Elisei]

>
> Please move this sys.exit to the main().  So this should just return
> a list of the rcodes to match the tasks it was passed.
> TaskHandler can be used easily via api, so we don't want it to exit out
> of someone else's code.  That will also keep main() as the primary cli
> interface.  There process the list of rcodes and exit appropriately.


This was an oversight on my part, I will make the necessary changes.

Alexandru, I tend to not prefer using numbers to indicate success/fail.
> They can be confusing at times.  In this case 1 means fail, 0 means
> success.  Which is the opposite of True/False.  It would be different
> if there were multiple return code values.  Then they would be
> meaningful.


 Sure, I understand your point, we are only interested in success of
failure of the modules, so having True or False as return values makes
sense. I got the idea to use 1 as a failure return code because that's how
the portage sync modules work (for example:
https://gitweb.gentoo.org/proj/portage.git/tree/pym/portage/sync/modules/git/git.py#n46).
That's also why I chose to return (returncode, message) and not (message,
returncode).



On Tue, Jan 17, 2017 at 8:23 PM, Brian Dolbec <dolsen@g.o> wrote:

> On Tue, 17 Jan 2017 19:48:16 +0200
> Alexandru Elisei <alexandru.elisei@×××××.com> wrote:
>
> > Currently module functions return a message to emaint after
> > invocation. Emaint prints this message then exits normally (with a
> > success return code) even if the module encountered an error. This
> > patch aims to change this by having each module public function
> > return a tuple of (returncode, message). Emaint will inspect the
> > return code and will exit unsuccessfully if necessary.
> > ---
> >  pym/portage/emaint/main.py                    | 11 +++++++++--
> >  pym/portage/emaint/modules/binhost/binhost.py |  6 ++++--
> >  pym/portage/emaint/modules/config/config.py   |  8 ++++++--
> >  pym/portage/emaint/modules/logs/logs.py       |  9 +++++----
> >  pym/portage/emaint/modules/merges/merges.py   | 18 +++++++++++-------
> >  pym/portage/emaint/modules/move/move.py       |  9 +++++++--
> >  pym/portage/emaint/modules/resume/resume.py   |  4 +++-
> >  pym/portage/emaint/modules/sync/sync.py       | 19
> > +++++++++++++------ pym/portage/emaint/modules/world/world.py     |
> > 8 ++++++-- 9 files changed, 64 insertions(+), 28 deletions(-)
> >
> > diff --git a/pym/portage/emaint/main.py b/pym/portage/emaint/main.py
> > index 65e3545..ef4383a 100644
> > --- a/pym/portage/emaint/main.py
> > +++ b/pym/portage/emaint/main.py
> > @@ -115,6 +115,7 @@ class TaskHandler(object):
> >               """Runs the module tasks"""
> >               if tasks is None or func is None:
> >                       return
> > +             returncode = os.EX_OK
> >               for task in tasks:
> >                       inst = task()
> >                       show_progress = self.show_progress_bar and
> > self.isatty @@ -135,14 +136,20 @@ class TaskHandler(object):
> >                               # them for other tasks if there is
> > more to do. 'options': options.copy()
> >                               }
> > -                     result = getattr(inst, func)(**kwargs)
> > +                     rcode, msgs = getattr(inst, func)(**kwargs)
> >                       if show_progress:
> >                               # make sure the final progress is
> > displayed self.progress_bar.display()
> >                               print()
> >                               self.progress_bar.stop()
> >                       if self.callback:
> > -                             self.callback(result)
> > +                             self.callback(msgs)
> > +                     # Keep the last error code when using the
> > 'all' command.
> > +                     if rcode != os.EX_OK:
> > +                             returncode = rcode
> > +
> > +             if returncode != os.EX_OK:
> > +                     sys.exit(returncode)
> >
>
>
> Please move this sys.exit to the main().  So this should just return
> a list of the rcodes to match the tasks it was passed.
> TaskHandler can be used easily via api, so we don't want it to exit out
> of someone else's code.  That will also keep main() as the primary cli
> interface.  There process the list of rcodes and exit appropriately.
>
>
>
> >
> >  def print_results(results):
> > diff --git a/pym/portage/emaint/modules/binhost/binhost.py
> > b/pym/portage/emaint/modules/binhost/binhost.py
> > index cf1213e..8cf3da6 100644
> > --- a/pym/portage/emaint/modules/binhost/binhost.py
> > +++ b/pym/portage/emaint/modules/binhost/binhost.py
> > @@ -86,7 +86,9 @@ class BinhostHandler(object):
> >               stale = set(metadata).difference(cpv_all)
> >               for cpv in stale:
> >                       errors.append("'%s' is not in the
> > repository" % cpv)
> > -             return errors
> > +             if errors:
> > +                     return (1, errors)
> > +             return (os.EX_OK, None)
> >
>
> Alexandru, I tend to not prefer using numbers to indicate success/fail.
> They can be confusing at times.  In this case 1 means fail, 0 means
> success.  Which is the opposite of True/False.  It would be different
> if there were multiple return code values.  Then they would be
> meaningful.  I want to keep these modules pythonic in the sense that
> they return standard True/False for API use.  In this case returning
> True/False would also make it easy to process the list of rcodes
> returned to main()
>
> sys.exit(False in rcodes)
>
> which will reverse the True/False to 0/1 automatically for us and make
> it super simple to process a variable length list success/fail results.
>
> --
> Brian Dolbec <dolsen>
>
>
>