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

[Pavel Kazakov]

On 03/13/14 15:09, Alec Warner wrote:
>
>
>
> On Wed, Mar 12, 2014 at 11:10 PM, Pavel Kazakov
> <nullishzero@g.o <mailto:nullishzero@g.o>> wrote:
>
>     ---
>      pym/portage/emaint/main.py                    |   6 +-
>

...

> Perhaps add an __iter__ function to TrackingFile, that implicitly
> calls load?
>
> Then this can be:
> for pkg, mtime in self._tracking_file:
>
> ?
>
> Just a thought.

That's actually a neat idea. Going to do it.

>
>
>     +               for pkg, mtime in self._tracking_file.load().items():
>     +                       if pkg not in failed_pkgs:
>     +                               failed_pkgs[pkg] = mtime
>     +               return failed_pkgs
>     +
>     +
>     +       def _remove_failed_dirs(self, failed_pkgs):
>     +               """
>     +               Remove the directories of packages that failed to
>     merge.
>     +
>     +               @param failed_pkgs: failed packages whose
>     directories to remove
>     +               @type failed_pkg: dict
>     +               """
>     +               for failed_pkg in failed_pkgs:
>     +                       pkg_path = os.path.join(self._vardb_path,
>     failed_pkg)
>     +                       # delete failed merge directory if it
>     exists (it might not exist
>     +                       # if loaded from tracking file)
>     +                       if os.path.exists(pkg_path):
>     +                               shutil.rmtree(pkg_path)
>     +                       # TODO: try removing package contents to
>     prevent orphaned
>     +                       # files
>
>
> I don't get this TODO, can you elaborate?

Right now the current code deletes only the failed merge directory (e.g.
/var/db/pkg/app-misc/-MERGING-lsx-0.1/).

In /var/db/pkg/app-misc/-MERGING-lsx-0.1/, there should be a file called
CONTENTS that records the package contents (e.g. obj
/usr/bin/lsx-suckless 478333aa1cb7761bc8e3a400cbd976ab 1394688298).
However, during a failed merge, the CONTENTS file may not be there or
may be corrupt, so my code doesn't handle removing the contents right
now, only the failed merge directory.

Any suggestions for better wording?

>
>
>     +
>     +
>     +       def _get_pkg_atoms(self, failed_pkgs, pkg_atoms, pkg_dne):
>     +               """
>     +               Get the package atoms for the specified failed
>     packages.
>     +
>     +               @param failed_pkgs: failed packages to iterate
>     +               @type failed_pkgs: dict
>     +               @param pkg_atoms: append package atoms to this set
>     +               @type pkg_atoms: set
>     +               @param pkg_dne: append any packages atoms that are
>     invalid to this set
>     +               @type pkg_dne: set
>
>
> Not following what dne means.

How about pkg_invalid_entries? And I can change the comment to '@param
pkg_invalid_entries: append any packages that are invalid to this set'

After looking at the function more, I think it might make sense to
instead return pkg_atoms and pkg_invalid_entries as a tuple, instead of
having the calling code pass them in.

>
>
>     +               """
>     +
>     +               emerge_config = load_emerge_config()
>     +               portdb =
>     emerge_config.target_config.trees['porttree'].dbapi
>     +               for failed_pkg in failed_pkgs:
>     +                       # validate pkg name
>     +                       pkg_name = '%s' %
>     failed_pkg.replace(MERGING_IDENTIFIER, '')
>     +                       pkg_atom = '=%s' % pkg_name
>     +
>     +                       if not isvalidatom(pkg_atom):
>     +                               pkg_dne.append( "'%s' is an
>     invalid package atom." % pkg_atom)
>     +                       if not portdb.cpv_exists(pkg_name):
>     +                               pkg_dne.append( "'%s' does not
>     exist in the portage tree."
>     +                                       % pkg_name)
>     +                       pkg_atoms.add(pkg_atom)
>     +
>     +
>     +       def _emerge_pkg_atoms(self, module_output, pkg_atoms):
>     +               """
>     +               Emerge the specified packages atoms.
>     +
>     +               @param module_output: output will be written to
>     +               @type module_output: Class
>     +               @param pkg_atoms: packages atoms to emerge
>     +               @type pkg_atoms: set
>     +               @rtype: list
>     +               @return: List of results
>     +               """
>     +               # TODO: rewrite code to use portage's APIs instead
>     of a subprocess
>     +               env = {
>     +                       "FEATURES" : "-collision-detect
>     -protect-owned",
>     +                       "PATH" : os.environ["PATH"]
>     +               }
>     +               emerge_cmd = (
>     +                       portage._python_interpreter,
>     +                       '-b',
>     +                       os.path.join(PORTAGE_BIN_PATH, 'emerge'),
>     +                       '--quiet',
>     +                       '--oneshot',
>     +                       '--complete-graph=y'
>     +               )
>     +               results = []
>     +               msg = 'Re-Emerging packages that failed to merge...\n'
>     +               if module_output:
>     +                       module_output.write(msg)
>     +               else:
>     +                       module_output = subprocess.PIPE
>     +                       results.append(msg)
>     +               proc = subprocess.Popen(emerge_cmd +
>     tuple(pkg_atoms), env=env,
>     +                       stdout=module_output, stderr=sys.stderr)
>     +               output = proc.communicate()[0]
>
>
> This seems sort of weird, perhaps:
>
> output, _ = proc.communicate()

I'll change it, but in fairness 'proc.communicate()[0]' is how both the
official python documentation and other areas of portage do it ;)

>
>
>     +               if output:
>     +                       results.append(output)
>     +               if proc.returncode != os.EX_OK:
>     +                        emerge_status = "Failed to emerge '%s'" %
>     (' '.join(pkg_atoms))
>     +               else:
>
...

I'll incorporate your other changes.

Regards,
Pavel