A few fixes to config path computation, esp. in tests.

This gets us part of the way towards making integration
tests hermetic, and unreliant on our real pants.ini.

With this commit, all unittests pass if we move our pants.ini
to a different location.

This commit also ensures that option and contrib/scrooge's
integration tests run if we move pants.ini.

It will take many followups to get all integration tests
to behave similarly.

Testing Done:
CI passes: https://travis-ci.org/pantsbuild/pants/builds/123700526

Moved pants.ini and ran various tests.

Bugs closed: 3195

Reviewed at https://rbcommons.com/s/twitter/r/3709/

.gitignore

@@ -16,6 +16,7 @@
 .buildcache/
 .coverage*
 .idea
+.cache
 .pants.d
 .pants.run
 .pants.workdir.file_lock

contrib/scrooge/tests/python/pants_test/contrib/scrooge/tasks/test_thrift_linter_integration.py

@@ -12,6 +12,25 @@ class ThriftLinterTest(PantsRunIntegrationTest):
 
   lint_error_token = "LINT-ERROR"
 
+  @classmethod
+  def hermetic(cls):
+    return True
+
+  def run_pants(self, command, config=None, stdin_data=None, extra_env=None, **kwargs):
+    full_config = {
+      'GLOBAL': {
+        'pythonpath': ["%(buildroot)s/contrib/scrooge/src/python"],
+        'backend_packages': ["pants.contrib.scrooge"]
+      },
+    }
+    if config:
+      for scope, scoped_cfgs in config.items():
+        updated = full_config.get(scope, {})
+        updated.update(scoped_cfgs)
+        full_config[scope] = updated
+    return super(ThriftLinterTest, self).run_pants(command, full_config, stdin_data, extra_env,
+                                                   **kwargs)
+
   @staticmethod
   def thrift_test_target(name):
     return 'contrib/scrooge/tests/thrift/org/pantsbuild/contrib/scrooge/thrift_linter:' + name
@@ -72,7 +91,7 @@ def test_bad_non_strict_override(self):
     self.assertIn(self.lint_error_token, pants_run.stdout_data)
 
   def test_bad_pants_ini_strict(self):
-    # thrift-linter fails if pants.ini has a thrift-linter:strict=True setting
+    # thrift-linter fails if pants.ini has a thrift-linter:strict=True setting.
     cmd = ['thrift-linter', self.thrift_test_target('bad-thrift-default')]
     pants_ini_config = {'thrift-linter': {'strict': True}}
     pants_run = self.run_pants(cmd, config=pants_ini_config)

src/python/pants/option/config.py

@@ -108,6 +108,10 @@ def _getinstance(self, section, option, type_, default=None):
                             raise_type=self.ConfigError)
 
   # Subclasses must implement.
+  def configs(self):
+    """Returns the underlying single-file configs represented by this object."""
+    raise NotImplementedError()
+
   def sources(self):
     """Returns the sources of this config as a list of filenames."""
     raise NotImplementedError()
@@ -145,6 +149,9 @@ class _EmptyConfig(Config):
   def sources(self):
     return []
 
+  def configs(self):
+    return []
+
   def sections(self):
     return []
 
@@ -169,6 +176,9 @@ def __init__(self, configpath, configparser):
     self.configpath = configpath
     self.configparser = configparser
 
+  def configs(self):
+    return [self]
+
   def sources(self):
     return [self.configpath]
 
@@ -203,31 +213,34 @@ def __init__(self, configs):
                     Later instances take precedence over earlier ones.
     """
     super(_ChainedConfig, self).__init__()
-    self.configs = list(reversed(configs))
+    self._configs = list(reversed(configs))
+
+  def configs(self):
+    return self._configs
 
   def sources(self):
-    return list(itertools.chain.from_iterable(cfg.sources() for cfg in self.configs))
+    return list(itertools.chain.from_iterable(cfg.sources() for cfg in self._configs))
 
   def sections(self):
     ret = OrderedSet()
-    for cfg in self.configs:
+    for cfg in self._configs:
       ret.update(cfg.sections())
     return ret
 
   def has_section(self, section):
-    for cfg in self.configs:
+    for cfg in self._configs:
       if cfg.has_section(section):
         return True
     return False
 
   def has_option(self, section, option):
-    for cfg in self.configs:
+    for cfg in self._configs:
       if cfg.has_option(section, option):
         return True
     return False
 
   def get_value(self, section, option):
-    for cfg in self.configs:
+    for cfg in self._configs:
       try:
         return cfg.get_value(section, option)
       except (configparser.NoSectionError, configparser.NoOptionError):
@@ -237,7 +250,7 @@ def get_value(self, section, option):
     raise configparser.NoOptionError(option, section)
 
   def get_source_for_option(self, section, option):
-    for cfg in self.configs:
+    for cfg in self._configs:
       if cfg.has_option(section, option):
         return cfg.get_source_for_option(section, option)
     return None

src/python/pants/option/options_bootstrapper.py

@@ -162,7 +162,7 @@ def verify_configs_against_options(self, options):
     :return: None.
     """
     has_error = False
-    for config in self._post_bootstrap_config.configs:
+    for config in self._post_bootstrap_config.configs():
       for section in config.sections():
         if section == GLOBAL_SCOPE_CONFIG_SECTION:
           scope = GLOBAL_SCOPE
@@ -175,12 +175,15 @@ def verify_configs_against_options(self, options):
           has_error = True
         else:
           # All the options specified under [`section`] in `config` excluding bootstrap defaults.
-          all_options_under_scope = set(config.configparser.options(section)) - set(config.configparser.defaults())
+          all_options_under_scope = (set(config.configparser.options(section)) -
+                                     set(config.configparser.defaults()))
           for option in all_options_under_scope:
             if option not in valid_options_under_scope:
-              logger.error("Invalid option '{}' under [{}] in {}".format(option, section, config.configpath))
+              logger.error("Invalid option '{}' under [{}] in {}".format(option, section,
+                                                                         config.configpath))
               has_error = True
 
     if has_error:
-      raise OptionsError("Invalid config entries detected. See log for details on which entries to update or remove.\n"
+      raise OptionsError("Invalid config entries detected. See log for details on which entries "
+                         "to update or remove.\n"
                          "(Specify --no-verify-config to disable this check.)")

tests/python/pants_test/option/test_options.py

@@ -605,7 +605,9 @@ def test_file_spec_args(self):
         """
       ))
       tmp.flush()
-      cmdline = './pants --target-spec-file={filename} compile morx:tgt fleem:tgt'.format(
+      # Note that we prevent loading a real pants.ini during get_bootstrap_options().
+      cmdline = './pants --target-spec-file={filename} --pants-config-files="[]" ' \
+                'compile morx:tgt fleem:tgt'.format(
         filename=tmp.name)
       bootstrapper = OptionsBootstrapper(args=shlex.split(cmdline))
       bootstrap_options = bootstrapper.get_bootstrap_options().for_global_scope()

tests/python/pants_test/option/test_options_bootstrapper.py

@@ -24,7 +24,10 @@ def _do_test(self, expected_vals, config, env, args):
                                  pants_distdir=expected_vals[2])
 
   def _config_path(self, path):
-    return ["--pants-config-files=['{}']".format(path)]
+    if path is None:
+      return ["--pants-config-files=[]"]
+    else:
+      return ["--pants-config-files=['{}']".format(path)]
 
   def _test_bootstrap_options(self, config, env, args, **expected_entries):
     with temporary_file() as fp:
@@ -213,7 +216,8 @@ def test_full_options_caching(self):
 
   def test_bootstrap_short_options(self):
     def parse_options(*args):
-      return OptionsBootstrapper(args=list(args)).get_bootstrap_options().for_global_scope()
+      full_args = list(args) + self._config_path(None)
+      return OptionsBootstrapper(args=full_args).get_bootstrap_options().for_global_scope()
 
     # No short options passed - defaults presented.
     vals = parse_options()
@@ -231,7 +235,8 @@ def parse_options(*args):
 
   def test_bootstrap_options_passthrough_dup_ignored(self):
     def parse_options(*args):
-      return OptionsBootstrapper(args=list(args)).get_bootstrap_options().for_global_scope()
+      full_args = list(args) + self._config_path(None)
+      return OptionsBootstrapper(args=full_args).get_bootstrap_options().for_global_scope()
 
     vals = parse_options('main', 'args', '-d/tmp/frogs', '--', '-d/tmp/logs')
     self.assertEqual('/tmp/frogs', vals.logdir)

tests/python/pants_test/option/test_options_integration.py

@@ -14,6 +14,10 @@
 
 class TestOptionsIntegration(PantsRunIntegrationTest):
 
+  @classmethod
+  def hermetic(cls):
+    return True
+
   def test_options_works_at_all(self):
     self.assert_success(self.run_pants(['options']))
 

tests/python/pants_test/pants_run_integration_test.py

@@ -31,7 +31,6 @@
 def ensure_cached(expected_num_artifacts=None):
   """Decorator for asserting cache writes in an integration test.
 
-  :param task_cls: Class of the task to check the artifact cache for. (e.g. JarCreate)
   :param expected_num_artifacts: Expected number of artifacts to be in the task's
                                  cache after running the test. If unspecified, will
                                  assert that the number of artifacts in the cache is
@@ -63,6 +62,14 @@ class PantsRunIntegrationTest(unittest.TestCase):
   PANTS_SUCCESS_CODE = 0
   PANTS_SCRIPT_NAME = 'pants'
 
+  @classmethod
+  def hermetic(cls):
+    """Subclasses may override to acknowledge that they are hermetic.
+
+    That is, that they should run without reading the real pants.ini.
+    """
+    return False
+
   @classmethod
   def has_python_version(cls, version):
     """Returns true if the current system has the specified version of python.
@@ -112,10 +119,21 @@ def source_clone(self, source_dir):
   def run_pants_with_workdir(self, command, workdir, config=None, stdin_data=None, extra_env=None,
                              **kwargs):
 
-    args = ['--no-pantsrc',
-            '--pants-workdir=' + workdir,
-            '--kill-nailguns',
-            '--print-exception-stacktrace']
+    args = [
+      '--no-pantsrc',
+      '--pants-workdir={}'.format(workdir),
+      '--kill-nailguns',
+      '--print-exception-stacktrace',
+    ]
+
+    if self.hermetic():
+      args.extend(['--pants-config-files=[]',
+                   # Turn off cache globally.  A hermetic integration test shouldn't rely on cache,
+                   # or we have no idea if it's actually testing anything.
+                   '--no-cache-read', '--no-cache-write',
+                   # Turn cache on just for tool bootstrapping, for performance.
+                   '--cache-bootstrap-read', '--cache-bootstrap-write'
+                   ])
 
     if config:
       config_data = config.copy()
@@ -132,8 +150,12 @@ def run_pants_with_workdir(self, command, workdir, config=None, stdin_data=None,
     pants_script = os.path.join(get_buildroot(), self.PANTS_SCRIPT_NAME)
     pants_command = [pants_script] + args + command
 
-    env = os.environ.copy()
-    env.update(extra_env or {})
+    if self.hermetic():
+      env = {}
+    else:
+      env = os.environ.copy()
+    if extra_env:
+      env.update(extra_env)
 
     proc = subprocess.Popen(pants_command, env=env, stdin=subprocess.PIPE,
                             stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs)