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

[Alexandru Elisei]

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