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

[Alec Warner]

On Wed, Feb 19, 2014 at 3:20 PM, Brian Dolbec <dolsen@g.o> wrote:

> On Wed, 19 Feb 2014 14:32:02 -0800
> Alec Warner <antarus@g.o> wrote:
>
> > On Wed, Feb 19, 2014 at 12:31 PM, Pavel Kazakov <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.
> > > +"""
> > >
> >
> Correct ^^
>
> > """Scan for failed merges fix them."""
>
> No, your grammar is wrong
>

I didn't mean grammar, I meant.

"""foo."""

not

"""foo.
"""


>
> >
> >
> > > +
> > > +
> > > +module_spec = {
> > > +       'name': 'merges',
> > > +       'description': __doc__,
> > > +       'provides':{
> > >
> >
> > 'module1' ?
> >
>
> It is just an identifier, not used other than to group together
> everything for that module.  The plugin system can handle multiple
> sub-modules within the same general module directory.  See the
>  emaint/modules/move/__init__.py. Move supplies 2 submodules.
>
> The new websync sync module (emerge-webrsync replacement) we started
> roughing in also has 2 sub-modules, the 1st will be the module that
> runs the original bash script.
> The 2nd will be the new python converted code to replace it.
>
> >
> > > +               '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?
> >
> >
> > > +
> > > +class MergesHandler(object):
> > > +
> > > +       short_desc = "Remove failed merges"
> > > +
> > >
> >
> >  @staticmethod decorator?
>
> Yeah, that is copying legacy emaint code from the original module
> classes.
>
>
> >
> > > +       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.
> >
>
> The emaint code was not setup to handle init variable assignments.
> None of the original module classes used any.  The plugin system
> doesn't care.  But the TaskHandler class in main.py would need to be
> modified.  Also, all modules must use the same method of initializing,
> regardless whether they need it or not.  In the new sync modules all
> relevant data is passed in using kwargs, then it saves any info it
> needs.
>

So use kwargs here?


>
> >   Also..why can't self._vardb_path be queried from the vardb?
> >
> >
> > > +               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(...)) ?
> >
> > > +                       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
>

os.path.join here as well.


> > > +
> > > +       def check(self, **kwargs):
> > > +               onProgress = kwargs.get('onProgress', None)
> > > +               failed_pkgs = []
> > > +               for pkg in self._failed_packages(onProgress):
> > > +                       failed_pkgs.append(pkg)
>

I think we want a set() here, not a list.


> > > +
> > > +               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?
>
> you can turn on the unicode option.
>
> >
> >
> > > +                               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
> >
> >
> >
> >
> > > +                                               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 :)
> >
> > -A
> >
>
> Yeah, but it is the only way to actually fix a failed merge.  You must
> turn off collision protect so it can overwrite the stray files from
> the previously failed merge.  There often is not a valid CONTENTS file.
> Plus it may not be accurate as to the files actually merged.   This
> module is for fixing pkgs that fail during the merge phase to the live
> filesystem.
>

Thats not what I meant ;)


>
>
>
>
> --
> Brian Dolbec <dolsen>
>
>
>