Re: [gentoo-portage-dev] [PATCH] Add an emaint module that can scan for failed merges and that can fix failed merges.

[Pavel Kazakov]

Hey Alec,

Thanks for the comments--good stuff.

On 02/19/2014 02:32 PM, Alec Warner wrote:
> On Wed, Feb 19, 2014 at 12:31 PM, Pavel Kazakov <nullishzero@g.o
> <mailto:nullishzero@g.o>> wrote:
>
>     ---
>      pym/portage/emaint/modules/merges/__init__.py | 20 ++++++++
>      pym/portage/emaint/modules/merges/merges.py   | 70
>     +++++++++++++++++++++++++++
>      2 files changed, 90 insertions(+)
>      create mode 100644 pym/portage/emaint/modules/merges/__init__.py
>      create mode 100644 pym/portage/emaint/modules/merges/merges.py
>
>     diff --git a/pym/portage/emaint/modules/merges/__init__.py
>     b/pym/portage/emaint/modules/merges/__init__.py
>     new file mode 100644
>     index 0000000..2cd79af
>     --- /dev/null
>     +++ b/pym/portage/emaint/modules/merges/__init__.py
>     @@ -0,0 +1,20 @@
>     +# Copyright 2005-2014 Gentoo Foundation
>     +# Distributed under the terms of the GNU General Public License v2
>     +
>     +"""Scan for failed merges and fix them.
>     +"""
>
>
> """Scan for failed merges fix them."""
All the other modules put the second """ on a newline, so I wanted to be
consistent. Should I still make the change?
>
>
>     +
>     +
>     +module_spec = {
>     +       'name': 'merges',
>     +       'description': __doc__,
>     +       'provides':{
>
>
> 'module1' ?
All the other modules did it this way, so I wanted consistency.

>
>
>     +               'module1': {
>     +                       'name': "merges",
>     +                       'class': "MergesHandler",
>     +                       'description': __doc__,
>     +                       'functions': ['check', 'fix',],
>     +                       'func_desc': {}
>     +                       }
>     +               }
>     +       }
>     diff --git a/pym/portage/emaint/modules/merges/merges.py
>     b/pym/portage/emaint/modules/merges/merges.py
>     new file mode 100644
>     index 0000000..b243082
>     --- /dev/null
>     +++ b/pym/portage/emaint/modules/merges/merges.py
>     @@ -0,0 +1,70 @@
>     +# Copyright 2005-2014 Gentoo Foundation
>     +# Distributed under the terms of the GNU General Public License v2
>     +
>     +import portage
>     +from portage import os
>     +from portage.const import PRIVATE_PATH, VDB_PATH
>     +from portage.util import writemsg
>     +
>     +import shutil
>     +import sys
>     +
>     +if sys.hexversion >= 0x3000000:
>     +       # pylint: disable=W0622
>     +       long = int
>
>
> What is this little guy? Can we just do this in a library someplace?
Checks if python version is greater than or equal to 3, but I probably
don't even need it since I'm not using longs.
>
>
>     +
>     +class MergesHandler(object):
>     +
>     +       short_desc = "Remove failed merges"
>     +
>
>
>  @staticmethod decorator?
Yeah, I copied legacy emaint code from other modules, but I can switch.
>
>     +       def name():
>     +               return "merges"
>     +       name = staticmethod(name)
>     +
>     +       def __init__(self):
>
>
> Generally you want to be able to change these later, so you might do
> something like:
>
> def __init__(self, eroot=None, vardb=None, vardb_path=None):
>   self._eroot = error or portage.settings['EROOT']
>   ... and so forth.
>
>   Also..why can't self._vardb_path be queried from the vardb?
To be honest, I noticed the other modules assigning such variables to
self, but they could all just as easily be calculated without needing to
be instance member variables.
>
>
>     +               self._eroot = portage.settings['EROOT']
>     +               self._vardb = portage.db[self._eroot]["vartree"].dbapi
>     +               self._vardb_path = os.path.join(self._eroot,
>     VDB_PATH) + os.path.sep
>     +
>     +       def can_progressbar(self, func):
>     +               return True
>     +
>     +       def _failed_packages(self, onProgress):
>     +               for cat in os.listdir(self._vardb_path):
>
>   os.listdir(os.path.join(...)) ?
Bad habits die hard ;/. I need to update the rest of my path
concatenation to use os.path.join().
>
>     +                       pkgs = os.listdir(self._vardb_path + cat)
>     +                       maxval = len(pkgs)
>     +                       for i, pkg in enumerate(pkgs):
>     +                               if onProgress:
>     +                                       onProgress(maxval, i+1)
>     +
>     +                               if '-MERGING-' in pkg:
>     +                                       yield cat + os.path.sep + pkg
>     +
>     +       def check(self, **kwargs):
>     +               onProgress = kwargs.get('onProgress', None)
>     +               failed_pkgs = []
>     +               for pkg in self._failed_packages(onProgress):
>     +                       failed_pkgs.append(pkg)
>     +
>     +               errors = ["'%s' failed to merge." % x for x in
>     failed_pkgs]
>     +               return errors
>     +
>     +       def fix(self, **kwargs):
>     +               onProgress = kwargs.get('onProgress', None)
>     +               tracking_path = os.path.join(self._eroot,
>     PRIVATE_PATH, 'failed-merges');
>     +               try:
>     +                       with open(tracking_path, 'w') as tracking_file:
>
>
> is this unicode safe?
I can do a unicode encode to make sure it is.
>
>
>     +                               for failed_pkg in
>     self._failed_packages(onProgress):
>     +
>     tracking_file.write(failed_pkg + '\n')
>     +                                               pkg_path =
>     self._vardb_path + failed_pkg
>
>
> os.path.join(...)
>
>
>     +                                               # Delete failed
>     merge directory
>     +                                               # XXX: Would be a
>     good idea to attempt try removing
>     +                                               # package contents
>     to prevent orphaned files
>
>
> # XXX is terrible style. I realize a bunch of code does that, and it is
> stupid.
> # use
> # TODO: foo
Fair enough, I'll switch over to TODO.
>
>
>
>
>     +                                               shutil.rmtree(pkg_path)
>     +                                               # Re-emerge package
>
>     +                                               pkg_name = '=' +
>     failed_pkg.replace('-MERGING-', '')
>     +
>     features='FEATURES="-collision-detect -protect-owned"'
>     +                                               emerge_cmd="emerge
>     --verbose --oneshot --complete-graph=y"
>     +                                               os.system('%s %s %s'
>     % (features, emerge_cmd, pkg_name))
>
>
> This is a security vulnerability :)
Yikes ;/
>
> -A
>
>
>     +               except Exception as ex:
>     +                       writemsg('Unable to fix failed package: %s'
>     % str(ex))
>     --
>     1.8.3.2
>
>
>

Regards,
Pavel