Retrofit PythonSetup and PythonRepos as subsystems.

src/python/pants/backend/python/BUILD

@@ -46,7 +46,8 @@ python_library(
   dependencies = [
     '3rdparty/python:pex',
     '3rdparty/python:setuptools',
-    'src/python/pants/base:config',
+    'src/python/pants/option',
+    'src/python/pants/subsystem',
   ]
 )
 

src/python/pants/backend/python/python_chroot.py

@@ -54,17 +54,19 @@ class InvalidDependencyException(Exception):
     def __init__(self, target):
       Exception.__init__(self, "Not a valid Python dependency! Found: {}".format(target))
 
+  # TODO: A little extra push and we can get rid of the 'context' argument.
   def __init__(self,
                context,
+               python_setup,
+               python_repos,
                targets,
                extra_requirements=None,
                builder=None,
                platforms=None,
                interpreter=None):
     self.context = context
-    # TODO: These should come from the caller, and we should not know about config.
-    self._python_setup = PythonSetup(self.context.config)
-    self._python_repos = PythonRepos(self.context.config)
+    self._python_setup = python_setup
+    self._python_repos = python_repos
 
     self._targets = targets
     self._extra_requirements = list(extra_requirements) if extra_requirements else []

src/python/pants/backend/python/python_setup.py

@@ -11,36 +11,50 @@
 from pex.http import Context
 from pkg_resources import Requirement
 
+from pants.option.options import Options
+from pants.subsystem.subsystem import Subsystem
 
-# TODO(benjy): These are basically proto-subsystems. There's some obvious commonality, but
-# rather than factor that out now I'll retrofit these to use whatever general subsystem
-# implementation we come up with in the near future.
 
-class PythonSetup(object):
-  """Configuration data for a python environment."""
-  def __init__(self, config):
-    self._config = config
+class PythonSetup(Subsystem):
+  """A python environment."""
+
+  @classmethod
+  def scope_qualifier(cls):
+    return 'python-setup'
+
+  @classmethod
+  def register_options(cls, register):
+    super(PythonSetup, cls).register_options(register)
+    register('--interpreter-requirement', advanced=True, default='CPython>=2.7,<3',
+             help='The interpreter requirement string for this python environment.')
+    register('--setuptools-version', advanced=True, default='5.4.1',
+             help='The setuptools version for this python environment.')
+    register('--wheel-version', advanced=True, default='0.23.0',
+             help='The wheel version for this python environment.')
+    register('--platforms', advanced=True, type=Options.list, default=['current'],
+             help='The wheel version for this python environment.')
 
   @property
   def interpreter_requirement(self):
-    """Returns the repo-wide interpreter requirement."""
-    return self._get_config('interpreter_requirement')
+    return self.get_options().interpreter_requirement
 
   @property
   def setuptools_version(self):
-    return self._get_config('setuptools_version', default='5.4.1')
+    return self.get_options().setuptools_version
 
   @property
   def wheel_version(self):
-    return self._get_config('wheel_version', default='0.23.0')
+    return self.get_options().wheel_version
 
   @property
   def platforms(self):
-    return self._get_config_list('platforms', default=['current'])
+    return self.get_options().platforms
 
   @property
   def scratch_dir(self):
-    return os.path.join(self._config.getdefault('pants_workdir'), 'python')
+    # Note that this gives us a separate scratch dir for every instance of this subsystem,
+    # which allows us to have multiple python setups in play in a single pants run.
+    return os.path.join(self.get_options().pants_workdir, *self.options_scope.split('.'))
 
   def setuptools_requirement(self):
     return self._failsafe_parse('setuptools=={0}'.format(self.setuptools_version))
@@ -60,25 +74,29 @@ def _failsafe_parse(self, requirement):
     except TypeError:
       return Requirement.parse(requirement)
 
-  def _get_config(self, *args, **kwargs):
-    return self._config.get('python-setup', *args, **kwargs)
 
-  def _get_config_list(self, *args, **kwargs):
-    return self._config.getlist('python-setup', *args, **kwargs)
+class PythonRepos(Subsystem):
+  """A python code repository."""
 
+  @classmethod
+  def scope_qualifier(cls):
+    return 'python-repos'
 
-class PythonRepos(object):
-  """Configuration data for a python code repository."""
-  def __init__(self, config):
-    self._config = config
+  @classmethod
+  def register_options(cls, register):
+    super(PythonRepos, cls).register_options(register)
+    register('--repos', advanced=True, type=Options.list, default=[],
+             help='URLs of code repositories.')
+    register('--indexes', advanced=True, type=Options.list, default=[],
+             help='URLs of code repository indexes.')
 
   @property
   def repos(self):
-    return self._get_config_list('repos', [])
+    return self.get_options().repos
 
   @property
   def indexes(self):
-    return self._get_config_list('indexes', [])
+    return self.get_options().indexes
 
   def get_fetchers(self):
     fetchers = []
@@ -89,6 +107,3 @@ def get_fetchers(self):
   def get_network_context(self):
     # TODO(wickman): Add retry, conn_timeout, threads, etc configuration here.
     return Context.get()
-
-  def _get_config_list(self, *args, **kwargs):
-    return self._config.getlist('python-repos', *args, **kwargs)

src/python/pants/backend/python/tasks/pytest_run.py

@@ -22,9 +22,10 @@
 
 from pants.backend.python.python_chroot import PythonChroot
 from pants.backend.python.python_requirement import PythonRequirement
+from pants.backend.python.python_setup import PythonRepos, PythonSetup
 from pants.backend.python.targets.python_tests import PythonTests
 from pants.backend.python.tasks.python_task import PythonTask
-from pants.base.exceptions import TaskError, TestFailedTaskError
+from pants.base.exceptions import TestFailedTaskError
 from pants.base.target import Target
 from pants.base.workunit import WorkUnit
 from pants.util.contextutil import (environment_as, temporary_dir, temporary_file,
@@ -75,6 +76,10 @@ class PytestRun(PythonTask):
     PythonRequirement('unittest2py3k', version_filter=lambda py, pl: py.startswith('3'))
   ]
 
+  @classmethod
+  def global_subsystems(cls):
+    return super(PytestRun, cls).global_subsystems() + (PythonSetup, PythonRepos)
+
   @classmethod
   def register_options(cls, register):
     super(PytestRun, cls).register_options(register)
@@ -234,15 +239,15 @@ def compute_coverage_modules(target):
     with temporary_dir() as plugin_root:
       plugin_root = os.path.realpath(plugin_root)
       with safe_open(os.path.join(plugin_root, 'pants_reporter.py'), 'w') as fp:
-        fp.write(dedent('''
+        fp.write(dedent("""
           def pytest_configure(__multicall__, config):
             # This executes the rest of the pytest_configures ensuring the `pytest_cov` plugin is
             # registered so we can grab it below.
             __multicall__.execute()
             pycov = config.pluginmanager.getplugin('_cov')
             # Squelch console reporting
             pycov.pytest_terminal_summary = lambda *args, **kwargs: None
-        '''))
+        """))
 
       pythonpath = os.environ.get('PYTHONPATH')
       existing_pythonpath = pythonpath.split(os.pathsep) if pythonpath else []
@@ -337,6 +342,8 @@ def _test_runner(self, targets, workunit):
     builder.info.entry_point = 'pytest'
     chroot = PythonChroot(
       context=self.context,
+      python_setup=PythonSetup.global_instance(),
+      python_repos=PythonRepos.global_instance(),
       targets=targets,
       extra_requirements=self._TESTING_TARGETS,
       builder=builder,

src/python/pants/backend/python/tasks/python_task.py

@@ -19,6 +19,10 @@
 
 
 class PythonTask(Task):
+  @classmethod
+  def global_subsystems(cls):
+    return super(PythonTask, cls).global_subsystems() + (PythonSetup, PythonRepos)
+
   def __init__(self, *args, **kwargs):
     super(PythonTask, self).__init__(*args, **kwargs)
     self._compatibilities = self.get_options().interpreter or [b'']
@@ -28,8 +32,8 @@ def __init__(self, *args, **kwargs):
   @property
   def interpreter_cache(self):
     if self._interpreter_cache is None:
-      self._interpreter_cache = PythonInterpreterCache(PythonSetup(self.context.config),
-                                                       PythonRepos(self.context.config),
+      self._interpreter_cache = PythonInterpreterCache(PythonSetup.global_instance(),
+                                                       PythonRepos.global_instance(),
                                                        logger=self.context.log.debug)
 
       # Cache setup's requirement fetching can hang if run concurrently by another pants proc.
@@ -96,6 +100,8 @@ def temporary_chroot(self, interpreter=None, pex_info=None, targets=None,
     with self.context.new_workunit('chroot'):
       chroot = PythonChroot(
         context=self.context,
+        python_setup=PythonSetup.global_instance(),
+        python_repos=PythonRepos.global_instance(),
         targets=targets,
         extra_requirements=extra_requirements,
         builder=builder,

src/python/pants/subsystem/subsystem.py

@@ -65,13 +65,12 @@ def global_instance(cls):
     return cls._instance_for_scope(Options.GLOBAL_SCOPE)
 
   @classmethod
-  def reset_instance_for_scope(cls, scope):
-    key = (cls, scope)
-    cls._scoped_instances.pop(key, None)
+  def reset(cls):
+    """Forget all cached subsystem instances.
 
-  @classmethod
-  def reset_global_instance(cls):
-    cls.reset_instance_for_scope(Options.GLOBAL_SCOPE)
+    Used for test isolation.
+    """
+    cls._scoped_instances = {}
 
   @classmethod
   def instance_for_task(cls, task):

tests/python/pants_test/backend/python/tasks/BUILD

@@ -13,6 +13,7 @@ python_library(
   sources=['python_task_test.py'],
   dependencies=[
     'src/python/pants/backend/python:plugin',
+    'src/python/pants/backend/python:python_setup',
     'src/python/pants/base:address',
     'src/python/pants/base:build_file_aliases',
     'src/python/pants/util:dirutil',

tests/python/pants_test/backend/python/tasks/python_task_test.py

@@ -8,6 +8,7 @@
 import os
 from textwrap import dedent
 
+from pants.backend.python.python_setup import PythonRepos, PythonSetup
 from pants.backend.python.register import build_file_aliases as register_python
 from pants.base.address import SyntheticAddress
 from pants_test.tasks.task_test_base import TaskTestBase

tests/python/pants_test/base_test.py

@@ -127,6 +127,7 @@ def setUp(self):
       'cache_key_gen_version': '0-test',
     }
     BuildRoot().path = self.build_root
+    Subsystem.reset()
 
     self.create_file('pants.ini')
     build_configuration = BuildConfiguration()

tests/python/pants_test/tasks/BUILD

@@ -13,6 +13,7 @@ python_library(
     'src/python/pants/base:target',
     'src/python/pants/goal:context',
     'src/python/pants/goal',
+    'src/python/pants/ivy',
     'tests/python/pants_test:base_test',
     'tests/python/pants_test/base:context_utils',
   ]
@@ -209,7 +210,7 @@ python_tests(
   sources = ['test_depmap_integration.py'],
   dependencies = [
     'src/python/pants/base:build_environment',
-    'src/python/pants/ivy:ivy',
+    'src/python/pants/ivy',
     'src/python/pants/util:contextutil',
     'tests/python/pants_test:int-test',
   ],
@@ -240,7 +241,7 @@ python_tests(
   sources = ['test_export_integration.py'],
   dependencies = [
     'src/python/pants/base:build_environment',
-    'src/python/pants/ivy:ivy',
+    'src/python/pants/ivy',
     'src/python/pants/util:contextutil',
     'tests/python/pants_test:int-test',
   ],

tests/python/pants_test/tasks/task_test_base.py

@@ -15,7 +15,6 @@
 from pants.backend.core.tasks.console_task import ConsoleTask
 from pants.goal.goal import Goal
 from pants.ivy.bootstrapper import Bootstrapper
-from pants.ivy.ivy_subsystem import IvySubsystem
 from pants_test.base_test import BaseTest
 
 
@@ -41,8 +40,9 @@ def setUp(self):
     self._tmpdir = tempfile.mkdtemp(dir=self.pants_workdir)
     self._test_workdir = os.path.join(self._tmpdir, 'workdir')
     os.mkdir(self._test_workdir)
+    # TODO: Push this down to JVM-related tests only? Seems wrong to have an ivy-specific
+    # action in this non-JVM-specific, high-level base class.
     Bootstrapper.reset_instance()
-    IvySubsystem.reset_global_instance()
 
   @property
   def test_workdir(self):