Centralize options tracking in the Parser. (#4832)

This also fixes a bug where details provenance of a merged value would
be lost.

src/python/pants/option/options.py

@@ -307,11 +307,6 @@ def for_scope(self, scope, inherit_from_enclosing_scope=True):
         # This makes the code a bit neater.
         values.update(self.for_scope(deprecated_scope))
 
-    # Record the value derivation.
-    for option in values:
-      self._option_tracker.record_option(scope=scope, option=option, value=values[option],
-                                         rank=values.get_rank(option))
-
     # Cache the values.
     self._values_by_scope[scope] = values
 

src/python/pants/option/parser.py

@@ -520,20 +520,27 @@ def expand(val_str):
     # instances of RankedValue (so none will be None, although they may wrap a None value).
     ranked_vals = list(reversed(list(RankedValue.prioritized_iter(*values_to_rank))))
 
-    # Record info about the derivation of each of the values.
+    def record_option(value, rank, option_details=None):
+      deprecation_version = kwargs.get('removal_version')
+      self._option_tracker.record_option(scope=self._scope,
+                                         option=dest,
+                                         value=value,
+                                         rank=rank,
+                                         deprecation_version=deprecation_version,
+                                         details=option_details)
+
+    # Record info about the derivation of each of the contributing values.
+    detail_history = []
     for ranked_val in ranked_vals:
       if ranked_val.rank in (RankedValue.CONFIG, RankedValue.CONFIG_DEFAULT):
         details = config_details
       elif ranked_val.rank == RankedValue.ENVIRONMENT:
         details = env_details
       else:
         details = None
-      self._option_tracker.record_option(scope=self._scope,
-                                         option=dest,
-                                         value=ranked_val.value,
-                                         rank=ranked_val.rank,
-                                         deprecation_version=kwargs.get('removal_version'),
-                                         details=details)
+      if details:
+        detail_history.append(details)
+      record_option(value=ranked_val.value, rank=ranked_val.rank, option_details=details)
 
     # Helper function to check various validity constraints on final option values.
     def check(val):
@@ -569,6 +576,10 @@ def check(val):
       ret = ranked_vals[-1]
       check(ret.value)
 
+    # Record info about the derivation of the final value.
+    merged_details = ', '.join(detail_history) if detail_history else None
+    record_option(value=ret.value, rank=ret.rank, option_details=merged_details)
+
     # All done!
     return ret
 

tests/python/pants_test/option/test_options_integration.py

@@ -111,7 +111,7 @@ def test_from_config(self):
           only_overridden: True
           show_history: True
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path), 'options'])
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path), 'options'])
       self.assert_success(pants_run)
       self.assertIn('options.only_overridden = True', pants_run.stdout_data)
       self.assertIn('(from CONFIG in {})'.format(config_path), pants_run.stdout_data)
@@ -135,7 +135,7 @@ def test_options_deprecation_from_config(self):
           [options]
           colors: False
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path), 'options'])
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path), 'options'])
       self.assert_success(pants_run)
 
 
@@ -161,7 +161,7 @@ def test_from_config_invalid_section(self):
           colors: False
           scope: options
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   'goals'])
       self.assert_failure(pants_run)
       self.assertIn('ERROR] Invalid scope [invalid_scope]', pants_run.stderr_data)
@@ -180,7 +180,7 @@ def test_from_config_invalid_option(self):
           fail_fast: True
           invalid_option: True
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   'goals'])
       self.assert_failure(pants_run)
       self.assertIn("ERROR] Invalid option 'invalid_option' under [test.junit]",
@@ -207,7 +207,7 @@ def test_from_config_invalid_global_option(self):
           [test.junit]
           fail_fast: True
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   'goals'])
       self.assert_failure(pants_run)
       self.assertIn("ERROR] Invalid option 'invalid_global' under [GLOBAL]", pants_run.stderr_data)
@@ -232,16 +232,16 @@ def test_invalid_command_line_option_and_invalid_config(self):
 
       # Run with invalid config and invalid command line option.
       # Should error out with invalid command line option only.
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   '--test-junit-invalid=ALL',
                                   'goals'])
       self.assert_failure(pants_run)
-      self.assertIn("Exception message: Unrecognized command line flags on scope 'test.junit': --invalid",
-                    pants_run.stderr_data)
+      self.assertIn("Exception message: Unrecognized command line flags on scope 'test.junit': "
+                    "--invalid", pants_run.stderr_data)
 
       # Run with invalid config only.
       # Should error out with `bad_option` and `invalid_scope` in config.
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   'goals'])
       self.assert_failure(pants_run)
       self.assertIn("ERROR] Invalid option 'bad_option' under [test.junit]", pants_run.stderr_data)
@@ -284,11 +284,12 @@ def test_pants_ignore_option(self):
           [GLOBAL]
           pants_ignore: +['some/random/dir']
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   '--no-colors',
                                   'options'])
       self.assert_success(pants_run)
-      self.assertIn("pants_ignore = ['.*/', '/dist/', 'some/random/dir'] (from CONFIG)",
+      self.assertIn("pants_ignore = ['.*/', '/dist/', 'some/random/dir'] (from CONFIG in {})"
+                    .format(config_path),
                     pants_run.stdout_data)
 
   @ensure_engine
@@ -301,9 +302,10 @@ def test_pants_ignore_option_non_default_dist_dir(self):
           pants_ignore: +['some/random/dir']
           pants_distdir: some/other/dist/dir
         """))
-      pants_run = self.run_pants(['--config-override={}'.format(config_path),
+      pants_run = self.run_pants(['--pants-config-files={}'.format(config_path),
                                   '--no-colors',
                                   'options'])
       self.assert_success(pants_run)
-      self.assertIn("pants_ignore = ['.*/', '/some/other/dist/dir/', 'some/random/dir'] (from CONFIG)",
+      self.assertIn("pants_ignore = ['.*/', '/some/other/dist/dir/', 'some/random/dir'] "
+                    "(from CONFIG in {})".format(config_path),
                     pants_run.stdout_data)