show deprecation warning for options given in env and config

Currently, if a deprecating option is provided through environment variable or config file, no warnings will be printed.
It will result in errors when that option is eventually deprecated.

This change makes Pants spit out a warning in those cases and adds test cases.

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/164089994

Bugs closed: 3896, 3918

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

src/python/pants/option/parser.py

@@ -144,10 +144,6 @@ def parse_args(self, flags, namespace):
       self._validate(args, kwargs)
       dest = kwargs.get('dest') or self._select_dest(args)
 
-      def consume_flag(flag):
-        self._check_deprecated(dest, kwargs)
-        del flag_value_map[flag]
-
       # Compute the values provided on the command line for this option.  Note that there may be
       # multiple values, for any combination of the following reasons:
       #   - The user used the same flag multiple times.
@@ -186,7 +182,7 @@ def add_flag_val(v):
         if arg in flag_value_map:
           for v in flag_value_map[arg]:
             add_flag_val(v)
-          consume_flag(arg)
+          del flag_value_map[arg]
 
       # Get the value for this option, falling back to defaults as needed.
       try:
@@ -200,6 +196,9 @@ def add_flag_val(v):
           '\nCaused by:\n{}'.format(', '.join(args), self._scope_str(), traceback.format_exc())
         )
 
+      # If the option is explicitly given, check deprecation.
+      if val.rank > RankedValue.HARDCODED:
+        self._check_deprecated(dest, kwargs)
       setattr(namespace, dest, val)
 
     # See if there are any unconsumed flags remaining.

tests/python/pants_test/option/test_options.py

@@ -756,69 +756,100 @@ def test_drop_flag_values(self):
 
   def test_deprecated_option_past_removal(self):
     """Ensure that expired options raise CodeRemovedError on attempted use."""
+    # Test option past removal from flag
     with self.assertRaises(CodeRemovedError):
       self._parse('./pants --global-crufty-expired=way2crufty').for_global_scope()
 
+    # Test option past removal from env
+    with self.assertRaises(CodeRemovedError):
+      self._parse('./pants', env={'PANTS_GLOBAL_CRUFTY_EXPIRED':'way2crufty'}).for_global_scope()
+
+    #Test option past removal from config
+    with self.assertRaises(CodeRemovedError):
+      self._parse('./pants', config={'GLOBAL':{'global_crufty_expired':'way2crufty'}}).for_global_scope()
+
   @contextmanager
   def warnings_catcher(self):
     with warnings.catch_warnings(record=True) as w:
       warnings.simplefilter('always')
       yield w
 
-  def test_deprecated_options(self):
-    def assertWarning(w, option_string):
-      self.assertEquals(1, len(w))
-      self.assertTrue(issubclass(w[-1].category, DeprecationWarning))
-      warning_message = str(w[-1].message)
-      self.assertIn("will be removed in version",
-                    warning_message)
-      self.assertIn(option_string, warning_message)
+  def assertWarning(self, w, option_string):
+    self.assertEquals(1, len(w))
+    self.assertTrue(issubclass(w[-1].category, DeprecationWarning))
+    warning_message = str(w[-1].message)
+    self.assertIn("will be removed in version",
+                  warning_message)
+    self.assertIn(option_string, warning_message)
 
+  def test_deprecated_options_flag(self):
     with self.warnings_catcher() as w:
       options = self._parse('./pants --global-crufty=crufty1')
       self.assertEquals('crufty1', options.for_global_scope().global_crufty)
-      assertWarning(w, 'global_crufty')
+      self.assertWarning(w, 'global_crufty')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants --global-crufty-boolean')
       self.assertTrue(options.for_global_scope().global_crufty_boolean)
-      assertWarning(w, 'global_crufty_boolean')
+      self.assertWarning(w, 'global_crufty_boolean')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants --no-global-crufty-boolean')
       self.assertFalse(options.for_global_scope().global_crufty_boolean)
-      assertWarning(w, 'global_crufty_boolean')
+      self.assertWarning(w, 'global_crufty_boolean')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants stale --crufty=stale_and_crufty')
       self.assertEquals('stale_and_crufty', options.for_scope('stale').crufty)
-      assertWarning(w, 'crufty')
+      self.assertWarning(w, 'crufty')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants stale --crufty-boolean')
       self.assertTrue(options.for_scope('stale').crufty_boolean)
-      assertWarning(w, 'crufty_boolean')
+      self.assertWarning(w, 'crufty_boolean')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants stale --no-crufty-boolean')
       self.assertFalse(options.for_scope('stale').crufty_boolean)
-      assertWarning(w, 'crufty_boolean')
+      self.assertWarning(w, 'crufty_boolean')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants --no-stale-crufty-boolean')
       self.assertFalse(options.for_scope('stale').crufty_boolean)
-      assertWarning(w, 'crufty_boolean')
+      self.assertWarning(w, 'crufty_boolean')
 
     with self.warnings_catcher() as w:
       options = self._parse('./pants --stale-crufty-boolean')
       self.assertTrue(options.for_scope('stale').crufty_boolean)
-      assertWarning(w, 'crufty_boolean')
+      self.assertWarning(w, 'crufty_boolean')
 
     # Make sure the warnings don't come out for regular options.
     with self.warnings_catcher() as w:
       self._parse('./pants stale --pants-foo stale --still-good')
       self.assertEquals(0, len(w))
 
+  def test_deprecated_options_env(self):
+    with self.warnings_catcher() as w:
+      options = self._parse('./pants', env={'PANTS_GLOBAL_CRUFTY':'crufty1'})
+      self.assertEquals('crufty1', options.for_global_scope().global_crufty)
+      self.assertWarning(w, 'global_crufty')
+
+    with self.warnings_catcher() as w:
+      options = self._parse('./pants', env={'PANTS_STALE_CRUFTY':'stale_and_crufty'})
+      self.assertEquals('stale_and_crufty', options.for_scope('stale').crufty)
+      self.assertWarning(w, 'crufty')
+
+  def test_deprecated_options_config(self):
+    with self.warnings_catcher() as w:
+      options = self._parse('./pants', config={'GLOBAL':{'global_crufty':'crufty1'}})
+      self.assertEquals('crufty1', options.for_global_scope().global_crufty)
+      self.assertWarning(w, 'global_crufty')
+
+    with self.warnings_catcher() as w:
+      options = self._parse('./pants', config={'stale':{'crufty':'stale_and_crufty'}})
+      self.assertEquals('stale_and_crufty', options.for_scope('stale').crufty)
+      self.assertWarning(w, 'crufty')
+
   def test_middle_scoped_options(self):
     """Make sure the rules for inheriting from a hierarchy of scopes.