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

[Brian Dolbec]

On Wed, 18 Jan 2017 19:15:54 +0200
Alexandru Elisei <alexandru.elisei@×××××.com> wrote:

> 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.
>
> The variable PORT_LOGDIR was added to the test environment to prevent
> CleanLogs.clean() from failing when the variable isn't set or not a
> directory.
> ---
>  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   |  3 ++-
>  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, 62 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..0d65d41 100644
> --- a/pym/portage/emaint/modules/resume/resume.py
> +++ b/pym/portage/emaint/modules/resume/resume.py
> @@ -37,7 +37,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 +56,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,

merged :) thank you

-- 
Brian Dolbec <dolsen>