refactor(sync): hide FD passing as an internal thing.

Effectively, add '_spawn' and '_spawn_interactive' to the base class,
and expect consumers to invoke the appropriate one.

This addition doesn't fully fix the fundamental lack of an observer (or log)
based approach to getting info out of syncers, but it at least hides some of
the internals so the syncer classes can do things like knowing what the
correct encoding is for stdout, thus being able to write their own messages
to it.

TL;DR: This subsystem's interaction w/ CLI (and used as a library) needs rewriting,
this just reduces that problem when we visit it.

Signed-off-by: Brian Harring <ferringb@gmail.com>
Signed-off-by: Arthur Zamarin <arthurzam@gentoo.org>

5 files changed, 30 insertions, 24 deletions

diff --git a/src/pkgcore/sync/base.py b/src/pkgcore/sync/base.py
index 2a63545e..1114532f 100644
--- a/src/pkgcore/sync/base.py
+++ b/src/pkgcore/sync/base.py
@@ -14,6 +14,7 @@ __all__ = (
 import os
 import pwd
 import stat
+import typing
 from importlib import import_module
 
 from snakeoil import process
@@ -123,7 +124,7 @@ class Syncer:
         except KeyError as exc:
             raise MissingLocalUser(raw_uri, str(exc))
 
-    def sync(self, verbosity=None, force=False):
+    def sync(self, verbosity: typing.Optional[int] = None, force=False):
         if self.disabled:
             return False
         kwds = {}
@@ -131,10 +132,9 @@ class Syncer:
             kwds["force"] = True
         if verbosity is None:
             verbosity = self.verbosity
-        # output_fd is harded coded as stdout atm.
-        return self._sync(verbosity, 1, **kwds)
+        return self._sync(verbosity, **kwds)
 
-    def _sync(self, verbosity, output_fd, **kwds):
+    def _sync(self, verbosity: int, **kwds):
         raise NotImplementedError(self, "_sync")
 
     def __str__(self):
@@ -191,11 +191,19 @@ class ExternalSyncer(Syncer):
                 disabled = cls._disabled = os.path.exists(path)
         return disabled
 
-    def _spawn(self, command, pipes, **kwargs):
+    def _spawn(self, command, **kwargs):
+        # Note: stderr is explicitly forced to stdout since that's how it was originally done.
+        # This can be changed w/ a discussion.
+        kwargs.setdefault("fd_pipes", {1: 1, 2: 1})
         return process.spawn.spawn(
-            command, fd_pipes=pipes, uid=self.uid, gid=self.gid, env=self.env, **kwargs
+            command, uid=self.uid, gid=self.gid, env=self.env, **kwargs
         )
 
+    def _spawn_interactive(self, command, **kwargs):
+        # Note: stderr is explicitly forced to stdout since that's how it was originally done.
+        # This can be changed w/ a discussion.
+        return self._spawn(command, fd_pipes={0: 0, 1: 1, 2: 1}, **kwargs)
+
     @staticmethod
     def _rewrite_uri_from_stat(path, uri):
         chunks = uri.split("//", 1)
@@ -209,7 +217,7 @@ class ExternalSyncer(Syncer):
 
 
 class VcsSyncer(ExternalSyncer):
-    def _sync(self, verbosity, output_fd):
+    def _sync(self, verbosity):
         try:
             st = os.stat(self.basedir)
         except FileNotFoundError:
@@ -229,7 +237,7 @@ class VcsSyncer(ExternalSyncer):
         elif verbosity > 0:
             command.append("-" + "v" * verbosity)
 
-        ret = self._spawn(command, pipes={1: output_fd, 2: output_fd, 0: 0}, cwd=chdir)
+        ret = self._spawn_interactive(command, cwd=chdir)
         return ret == 0
 
     def _initial_pull(self):
diff --git a/src/pkgcore/sync/http.py b/src/pkgcore/sync/http.py
index b422d94a..a3ec1898 100644
--- a/src/pkgcore/sync/http.py
+++ b/src/pkgcore/sync/http.py
@@ -22,7 +22,7 @@ class http_syncer(base.Syncer):
         self.basename = os.path.basename(uri)
         super().__init__(basedir, uri, **kwargs)
 
-    def _sync(self, verbosity, output_fd, force=False, **kwargs):
+    def _sync(self, verbosity, force=False, **kwargs):
         dest = self._pre_download()
 
         if self.uri.lower().startswith("https://"):
diff --git a/src/pkgcore/sync/rsync.py b/src/pkgcore/sync/rsync.py
index c6b95c70..b0f03273 100644
--- a/src/pkgcore/sync/rsync.py
+++ b/src/pkgcore/sync/rsync.py
@@ -128,8 +128,7 @@ class rsync_syncer(base.ExternalSyncer):
                 f"DNS resolution failed for {self.hostname!r}: {e.strerror}"
             )
 
-    def _sync(self, verbosity, output_fd):
-        fd_pipes = {1: output_fd, 2: output_fd}
+    def _sync(self, verbosity):
         opts = list(self.opts)
         if self.rsh:
             opts.append("-e")
@@ -150,7 +149,7 @@ class rsync_syncer(base.ExternalSyncer):
                 self.basedir,
             ] + opts
 
-            ret = self._spawn(cmd, fd_pipes)
+            ret = self._spawn(cmd)
             if ret == 0:
                 return True
             elif ret == 1:
@@ -203,7 +202,7 @@ class rsync_timestamp_syncer(rsync_syncer):
             # malformed timestamp
             return None
 
-    def _sync(self, verbosity, output_fd, force=False):
+    def _sync(self, verbosity, force=False):
         doit = force or self.last_timestamp is None
         ret = None
         try:
@@ -213,7 +212,7 @@ class rsync_timestamp_syncer(rsync_syncer):
                     timestamp_uri = pjoin(self.uri, "metadata", "timestamp.chk")
                     timestamp_path = new_timestamp.name
                     timestamp_syncer = _RsyncFileSyncer(timestamp_path, timestamp_uri)
-                    ret = timestamp_syncer._sync(verbosity, output_fd)
+                    ret = timestamp_syncer._sync(verbosity)
                     if not ret:
                         doit = True
                     else:
@@ -226,7 +225,7 @@ class rsync_timestamp_syncer(rsync_syncer):
                             doit = delta > self.negative_sync_delay
             if not doit:
                 return True
-            ret = super()._sync(verbosity, output_fd)
+            ret = super()._sync(verbosity)
             # force a reset of the timestamp
             self.last_timestamp = self.current_timestamp()
         finally:
diff --git a/src/pkgcore/sync/svn.py b/src/pkgcore/sync/svn.py
index e8cd6059..6b9e29a5 100644
--- a/src/pkgcore/sync/svn.py
+++ b/src/pkgcore/sync/svn.py
@@ -50,19 +50,17 @@ class svn_syncer(base.ExternalSyncer):
             raise base.UriError(raw_uri, "protocol unknown")
         return raw_uri
 
-    def _sync(self, verbosity, output_fd):
+    def _sync(self, verbosity):
         uri = self.uri
         if uri.startswith("svn+http://"):
             uri = uri.replace("svn+http://", "http://")
         elif uri.startswith("svn+https://"):
             uri = uri.replace("svn+https://", "https://")
         if not os.path.exists(self.basedir):
-            return 0 == self._spawn(
-                [self.binary_path, "co", uri, self.basedir],
-                {1: output_fd, 2: output_fd, 0: 0},
+            return 0 == self._spawn_interactive(
+                [self.binary_path, "co", uri, self.basedir]
             )
-        return 0 == self._spawn(
+        return 0 == self._spawn_interactive(
             [self.binary_path, "update"],
-            {1: output_fd, 2: output_fd, 0: 0},
             cwd=self.basedir,
         )
diff --git a/tests/scripts/test_pmaint.py b/tests/scripts/test_pmaint.py
index 63846456..c9af122f 100644
--- a/tests/scripts/test_pmaint.py
+++ b/tests/scripts/test_pmaint.py
@@ -1,6 +1,9 @@
 from functools import partial
 from io import BytesIO
 
+from snakeoil.formatters import PlainTextFormatter
+from snakeoil.mappings import AttrAccessible
+
 from pkgcore.config import basics
 from pkgcore.config.hint import ConfigHint
 from pkgcore.ebuild.cpv import CPV
@@ -9,8 +12,6 @@ from pkgcore.repository import syncable, util
 from pkgcore.scripts import pmaint
 from pkgcore.sync import base
 from pkgcore.test.scripts.helpers import ArgParseMixin
-from snakeoil.formatters import PlainTextFormatter
-from snakeoil.mappings import AttrAccessible
 
 Options = AttrAccessible
 
@@ -96,7 +97,7 @@ class FakeSyncer(base.Syncer):
         super().__init__(*args, **kwargs)
         self.synced = False
 
-    def _sync(self, verbosity, output_fd, **kwds):
+    def _sync(self, verbosity, **kwds):
         self.synced = True
         return self.succeed