[prev in list] [next in list] [prev in thread] [next in thread]
List: gentoo-portage-dev
Subject: Re: [gentoo-portage-dev] [PATCH] emaint: exit with non-zero status code when module fails (bug 56747
From: Alexandru Elisei <alexandru.elisei () gmail ! com>
Date: 2017-01-18 15:52:48
Message-ID: CAB-4s4kdp6pEKGWTvCJFh2Yx-YnXmTBye_MuuTh18mky9iXbng () mail ! gmail ! com
[Download RAW message or body]
I've modified the patch as per Brian's suggestions:
- the modules return True on success and False on failure.
- TaskHandler.run_tasks() returns a list of all the module return
codes, then emaint_main() exits.
- the portage/pym/portage/tests/emerge/test_simple.py change wasn't
present in the first version of the patch because of a subtle bug that
I introduced: rval was None when the variable PORT_LOGDIR wasn't set,
it was different from os.EX_OK so TaskHandler.run_tasks() was exiting
with sys.exit(None), which is zero - success.
Below is the modified commit message and the diff.
Module functions currently 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), where returncode is boolean True if the function
was successful or False otherwise. Emaint will inspect the return codes
and exit unsuccessfully if necessary.
---
pym/portage/emaint/main.py | 12 +++++++++---
pym/portage/emaint/modules/binhost/binhost.py | 6 ++++--
pym/portage/emaint/modules/config/config.py | 6 ++++--
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 | 20 +++++++++++++-------
pym/portage/emaint/modules/world/world.py | 8 ++++++--
pym/portage/tests/emerge/test_simple.py | 1 +
10 files changed, 63 insertions(+), 30 deletions(-)
diff --git a/pym/portage/emaint/main.py b/pym/portage/emaint/main.py
index 65e3545..f448d6b 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
+ returncodes = []
for task in tasks:
inst = task()
show_progress = self.show_progress_bar and self.isatty
@@ -135,14 +136,17 @@ class TaskHandler(object):
# them for other tasks if there is more to do.
'options': options.copy()
}
- result = getattr(inst, func)(**kwargs)
+ returncode, msgs = getattr(inst, func)(**kwargs)
+ returncodes.append(returncode)
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)
+
+ return returncodes
def print_results(results):
@@ -237,4 +241,6 @@ def emaint_main(myargv):
task_opts = options.__dict__
task_opts['return-messages'] = True
taskmaster = TaskHandler(callback=print_results, module_output=sys.stdout)
- taskmaster.run_tasks(tasks, func, status, options=task_opts)
+ returncodes = taskmaster.run_tasks(tasks, func, status, options=task_opts)
+
+ sys.exit(False in returncodes)
diff --git a/pym/portage/emaint/modules/binhost/binhost.py
b/pym/portage/emaint/modules/binhost/binhost.py
index cf1213e..527b02f 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 (False, errors)
+ return (True, None)
def fix(self, **kwargs):
onProgress = kwargs.get('onProgress', None)
@@ -177,4 +179,4 @@ class BinhostHandler(object):
if maxval == 0:
maxval = 1
onProgress(maxval, maxval)
- return None
+ return (True, None)
diff --git a/pym/portage/emaint/modules/config/config.py
b/pym/portage/emaint/modules/config/config.py
index dad024b..a05a3c2 100644
--- a/pym/portage/emaint/modules/config/config.py
+++ b/pym/portage/emaint/modules/config/config.py
@@ -36,7 +36,8 @@ class CleanConfig(object):
if onProgress:
onProgress(maxval, i+1)
i += 1
- return self._format_output(messages)
+ msgs = self._format_output(messages)
+ return (True, msgs)
def fix(self, **kwargs):
onProgress = kwargs.get('onProgress', None)
@@ -65,7 +66,8 @@ class CleanConfig(object):
i += 1
if modified:
writedict(configs, self.target)
- return self._format_output(messages, True)
+ msgs = self._format_output(messages, True)
+ return (True, msgs)
def _format_output(self, messages=[], cleaned=False):
output = []
diff --git a/pym/portage/emaint/modules/logs/logs.py
b/pym/portage/emaint/modules/logs/logs.py
index fe65cf5..028084a 100644
--- a/pym/portage/emaint/modules/logs/logs.py
+++ b/pym/portage/emaint/modules/logs/logs.py
@@ -40,7 +40,6 @@ class CleanLogs(object):
'NUM': int: number of days
'pretend': boolean
"""
- messages = []
num_of_days = None
pretend = False
if kwargs:
@@ -70,10 +69,12 @@ class CleanLogs(object):
clean_cmd.remove("-delete")
if not clean_cmd:
- return []
+ return (True, None)
rval = self._clean_logs(clean_cmd, settings)
- messages += self._convert_errors(rval)
- return messages
+ errors = self._convert_errors(rval)
+ if errors:
+ return (False, errors)
+ return (True, None)
@staticmethod
diff --git a/pym/portage/emaint/modules/merges/merges.py
b/pym/portage/emaint/modules/merges/merges.py
index 8f677c2..416a725 100644
--- a/pym/portage/emaint/modules/merges/merges.py
+++ b/pym/portage/emaint/modules/merges/merges.py
@@ -239,7 +239,9 @@ class MergesHandler(object):
for pkg, mtime in failed_pkgs.items():
mtime_str = time.ctime(int(mtime))
errors.append("'%s' failed to merge on '%s'" % (pkg, mtime_str))
- return errors
+ if errors:
+ return (False, errors)
+ return (True, None)
def fix(self, **kwargs):
@@ -247,13 +249,13 @@ class MergesHandler(object):
module_output = kwargs.get('module_output', None)
failed_pkgs = self._failed_pkgs()
if not failed_pkgs:
- return ['No failed merges found.']
+ return (True, ['No failed merges found.'])
pkg_invalid_entries = set()
pkg_atoms = set()
self._get_pkg_atoms(failed_pkgs, pkg_atoms, pkg_invalid_entries)
if pkg_invalid_entries:
- return pkg_invalid_entries
+ return (False, pkg_invalid_entries)
try:
self._tracking_file.save(failed_pkgs)
@@ -261,7 +263,7 @@ class MergesHandler(object):
errors = ['Unable to save failed merges to tracking file: %s\n'
% str(ex)]
errors.append(', '.join(sorted(failed_pkgs)))
- return errors
+ return (False, errors)
self._remove_failed_dirs(failed_pkgs)
results = self._emerge_pkg_atoms(module_output, pkg_atoms)
# list any new failed merges
@@ -276,12 +278,14 @@ class MergesHandler(object):
if not vardb.cpv_exists(pkg_name):
still_failed_pkgs[pkg] = mtime
self._tracking_file.save(still_failed_pkgs)
- return results
+ if still_failed_pkgs:
+ return (False, results)
+ return (True, results)
def purge(self, **kwargs):
"""Attempt to remove previously saved tracking file."""
if not self._tracking_file.exists():
- return ['Tracking file not found.']
+ return (True, ['Tracking file not found.'])
self._tracking_file.purge()
- return ['Removed tracking file.']
+ return (True, ['Removed tracking file.'])
diff --git a/pym/portage/emaint/modules/move/move.py
b/pym/portage/emaint/modules/move/move.py
index 41ca167..1c2636d 100644
--- a/pym/portage/emaint/modules/move/move.py
+++ b/pym/portage/emaint/modules/move/move.py
@@ -123,7 +123,10 @@ class MoveHandler(object):
errors.append("'%s' has outdated metadata" % cpv)
if onProgress:
onProgress(maxval, i+1)
- return errors
+
+ if errors:
+ return (False, errors)
+ return (True, None)
def fix(self, **kwargs):
onProgress = kwargs.get('onProgress', None)
@@ -156,7 +159,9 @@ class MoveHandler(object):
# Searching for updates in all the metadata is relatively slow, so this
# is where the progress bar comes out of indeterminate mode.
self._tree.dbapi.update_ents(allupdates, onProgress=onProgress)
- return errors
+ if errors:
+ return (False, errors)
+ return (True, None)
class MoveInstalled(MoveHandler):
diff --git a/pym/portage/emaint/modules/resume/resume.py
b/pym/portage/emaint/modules/resume/resume.py
index 1bada52..1d14275 100644
--- a/pym/portage/emaint/modules/resume/resume.py
+++ b/pym/portage/emaint/modules/resume/resume.py
@@ -2,6 +2,7 @@
# Distributed under the terms of the GNU General Public License v2
import portage
+import os
class CleanResume(object):
@@ -37,7 +38,7 @@ class CleanResume(object):
finally:
if onProgress:
onProgress(maxval, i+1)
- return messages
+ return (True, messages)
def fix(self, **kwargs):
onProgress = kwargs.get('onProgress', None)
@@ -56,3 +57,4 @@ class CleanResume(object):
onProgress(maxval, i+1)
if delete_count:
mtimedb.commit()
+ return (True, None)
diff --git a/pym/portage/emaint/modules/sync/sync.py
b/pym/portage/emaint/modules/sync/sync.py
index 15d63e2..d867699 100644
--- a/pym/portage/emaint/modules/sync/sync.py
+++ b/pym/portage/emaint/modules/sync/sync.py
@@ -127,8 +127,8 @@ class SyncRepos(object):
% (bold(", ".join(repos))) + "\n ...returning"
]
if return_messages:
- return msgs
- return
+ return (False, msgs)
+ return (False, None)
return self._sync(selected, return_messages,
emaint_opts=options)
@@ -211,8 +211,8 @@ class SyncRepos(object):
msgs.append("Emaint sync, nothing to sync... returning")
if return_messages:
msgs.extend(self.rmessage([('None', os.EX_OK)], 'sync'))
- return msgs
- return
+ return (True, msgs)
+ return (True, None)
# Portage needs to ensure a sane umask for the files it creates.
os.umask(0o22)
@@ -232,9 +232,14 @@ class SyncRepos(object):
sync_scheduler.wait()
retvals = sync_scheduler.retvals
msgs.extend(sync_scheduler.msgs)
+ returncode = True
if retvals:
msgs.extend(self.rmessage(retvals, 'sync'))
+ for repo, returncode in retvals:
+ if returncode != os.EX_OK:
+ returncode = False
+ break
else:
msgs.extend(self.rmessage([('None', os.EX_OK)], 'sync'))
@@ -244,6 +249,8 @@ class SyncRepos(object):
rcode = sync_manager.perform_post_sync_hook('')
if rcode:
msgs.extend(self.rmessage([('None', rcode)], 'post-sync'))
+ if rcode != os.EX_OK:
+ returncode = False
# Reload the whole config.
portage._sync_mode = False
@@ -254,8 +261,8 @@ class SyncRepos(object):
self.emerge_config.opts)
if return_messages:
- return msgs
- return
+ return (returncode, msgs)
+ return (returncode, None)
def _do_pkg_moves(self):
@@ -355,7 +362,6 @@ class SyncScheduler(AsyncScheduler):
# that hooks will be called in a backward-compatible manner
# even if all sync tasks have failed.
hooks_enabled = True
- returncode = task.returncode
if task.returncode == os.EX_OK:
returncode, message, updatecache_flg, hooks_enabled = task.result
if message:
diff --git a/pym/portage/emaint/modules/world/world.py
b/pym/portage/emaint/modules/world/world.py
index 2c9dbff..ebc3adc 100644
--- a/pym/portage/emaint/modules/world/world.py
+++ b/pym/portage/emaint/modules/world/world.py
@@ -65,7 +65,9 @@ class WorldHandler(object):
errors += ["'%s' is not installed" % x for x in self.not_installed]
else:
errors.append(self.world_file + " could not be opened for reading")
- return errors
+ if errors:
+ return (False, errors)
+ return (True, None)
def fix(self, **kwargs):
onProgress = kwargs.get('onProgress', None)
@@ -83,7 +85,9 @@ class WorldHandler(object):
except portage.exception.PortageException:
errors.append("%s could not be opened for writing" % \
self.world_file)
- return errors
+ if errors:
+ return (False, errors)
+ return (True, None)
finally:
world_set.unlock()
diff --git a/pym/portage/tests/emerge/test_simple.py
b/pym/portage/tests/emerge/test_simple.py
index b1a2af5..5930f6c 100644
--- a/pym/portage/tests/emerge/test_simple.py
+++ b/pym/portage/tests/emerge/test_simple.py
@@ -382,6 +382,7 @@ pkg_preinst() {
"PORTAGE_PYTHON" : portage_python,
"PORTAGE_REPOSITORIES" : settings.repositories.config_string(),
"PORTAGE_TMPDIR" : portage_tmpdir,
+ "PORT_LOGDIR" : portage_tmpdir,
"PYTHONDONTWRITEBYTECODE" : os.environ.get("PYTHONDONTWRITEBYTECODE", ""),
"PYTHONPATH" : pythonpath,
"__PORTAGE_TEST_PATH_OVERRIDE" : fake_bin,
--
2.10.2
On 1/17/17, Alexandru Elisei <alexandru.elisei@gmail.com> wrote:
>>
>> 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@gentoo.org> wrote:
>
>> On Tue, 17 Jan 2017 19:48:16 +0200
>> Alexandru Elisei <alexandru.elisei@gmail.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>
>>
>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic