Fix from_env to capture only explicitly set values.

Previously from_env also captured Variables defaults which had the
effect of defaulted Variables always over-riding user supplied PexInfo
values.

Testing Done:
CI went green here:
  https://travis-ci.org/pantsbuild/pex/builds/72339390

Bugs closed: 135, 136

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

CHANGES.rst

@@ -2,6 +2,14 @@
 CHANGES
 =======
 
+----------
+1.0.2.dev0
+----------
+
+* Bug fix: PEX-INFO values were overridden by environment `Variables` with default values that were
+  not explicitly set in the environment.
+  Fixes `#135 <https://github.com/pantsbuild/pex/issues/135>`_.
+
 -----
 1.0.1
 -----

pex/pex_info.py

@@ -88,16 +88,18 @@ def from_json(cls, content):
 
   @classmethod
   def from_env(cls, env=ENV):
+    supplied_env = env.strip_defaults()
+    zip_safe = None if supplied_env.PEX_FORCE_LOCAL is None else not supplied_env.PEX_FORCE_LOCAL
     pex_info = {
-      'pex_root': env.PEX_ROOT,
-      'entry_point': env.PEX_MODULE,
-      'script': env.PEX_SCRIPT,
-      'zip_safe': not env.PEX_FORCE_LOCAL,
-      'inherit_path': env.PEX_INHERIT_PATH,
-      'ignore_errors': env.PEX_IGNORE_ERRORS,
-      'always_write_cache': env.PEX_ALWAYS_CACHE,
+      'pex_root': supplied_env.PEX_ROOT,
+      'entry_point': supplied_env.PEX_MODULE,
+      'script': supplied_env.PEX_SCRIPT,
+      'zip_safe': zip_safe,
+      'inherit_path': supplied_env.PEX_INHERIT_PATH,
+      'ignore_errors': supplied_env.PEX_IGNORE_ERRORS,
+      'always_write_cache': supplied_env.PEX_ALWAYS_CACHE,
     }
-    # Filter out empty entries.
+    # Filter out empty entries not explicitly set in the environment.
     return cls(info=dict((k, v) for (k, v) in pex_info.items() if v is not None))
 
   @classmethod
@@ -260,10 +262,10 @@ def update(self, other):
     self._distributions.update(other.distributions)
     self._requirements.update(other.requirements)
 
-  def dump(self):
+  def dump(self, **kwargs):
     pex_info_copy = self._pex_info.copy()
     pex_info_copy['requirements'] = list(self._requirements)
-    return json.dumps(pex_info_copy)
+    return json.dumps(pex_info_copy, **kwargs)
 
   def copy(self):
     return PexInfo(info=self._pex_info.copy())

pex/variables.py

@@ -32,8 +32,9 @@ def iter_help(cls):
       variable_type, variable_text = cls.process_pydoc(getattr(value, '__doc__'))
       yield variable_name, variable_type, variable_text
 
-  def __init__(self, environ=None):
+  def __init__(self, environ=None, use_defaults=True):
     self._environ = environ.copy() if environ is not None else os.environ
+    self._use_defaults = use_defaults
 
   def copy(self):
     return self._environ.copy()
@@ -44,6 +45,9 @@ def delete(self, variable):
   def set(self, variable, value):
     self._environ[variable] = str(value)
 
+  def _defaulted(self, default):
+    return default if self._use_defaults else None
+
   def _get_bool(self, variable, default=False):
     value = self._environ.get(variable)
     if value is not None:
@@ -54,10 +58,10 @@ def _get_bool(self, variable, default=False):
       else:
         die('Invalid value for %s, must be 0/1/false/true, got %r' % (variable, value))
     else:
-      return default
+      return self._defaulted(default)
 
   def _get_string(self, variable, default=None):
-    return self._environ.get(variable, default)
+    return self._environ.get(variable, self._defaulted(default))
 
   def _get_path(self, variable, default=None):
     value = self._get_string(variable, default=default)
@@ -70,7 +74,14 @@ def _get_int(self, variable, default=None):
     except ValueError:
       die('Invalid value for %s, must be an integer, got %r' % (variable, self._environ[variable]))
     except KeyError:
-      return default
+      return self._defaulted(default)
+
+  def strip_defaults(self):
+    """Returns a copy of these variables but with defaults stripped.
+
+    Any variables not explicitly set in the environment will have a value of `None`.
+    """
+    return Variables(environ=self.copy(), use_defaults=False)
 
   @contextmanager
   def patch(self, **kw):

tests/test_pex_info.py

@@ -5,6 +5,7 @@
 
 from pex.orderedset import OrderedSet
 from pex.pex_info import PexInfo
+from pex.variables import Variables
 
 
 def make_pex_info(requirements):
@@ -32,3 +33,33 @@ def test_backwards_incompatible_pex_info():
       ['world==0.2', False, None],
   ])
   assert pi.requirements == OrderedSet(['hello==0.1', 'world==0.2'])
+
+
+def assert_same_info(expected, actual):
+  assert expected.dump(sort_keys=True) == actual.dump(sort_keys=True)
+
+
+def test_from_empty_env():
+  environ = Variables(environ={})
+  info = {}
+  assert_same_info(PexInfo(info=info), PexInfo.from_env(env=environ))
+
+
+def test_from_env():
+  environ = dict(PEX_ROOT='/pex_root',
+                 PEX_MODULE='entry:point',
+                 PEX_SCRIPT='script.sh',
+                 PEX_FORCE_LOCAL='true',
+                 PEX_INHERIT_PATH='true',
+                 PEX_IGNORE_ERRORS='true',
+                 PEX_ALWAYS_CACHE='true')
+
+  info = dict(pex_root='/pex_root',
+              entry_point='entry:point',
+              script='script.sh',
+              zip_safe=False,
+              inherit_path=True,
+              ignore_errors=True,
+              always_write_cache=True)
+
+  assert_same_info(PexInfo(info=info), PexInfo.from_env(env=Variables(environ=environ)))

tests/test_variables.py

@@ -72,3 +72,20 @@ def test_pex_vars_set():
   assert v._get_int('HELLO') == 42
   v.delete('HELLO')
   assert v._get_int('HELLO') is None
+
+
+def test_pex_vars_defaults_stripped():
+  v = Variables(environ={})
+  stripped = v.strip_defaults()
+
+  # bool
+  assert v.PEX_ALWAYS_CACHE is not None
+  assert stripped.PEX_ALWAYS_CACHE is None
+
+  # string
+  assert v.PEX_PATH is not None
+  assert stripped.PEX_PATH is None
+
+  # int
+  assert v.PEX_VERBOSE is not None
+  assert stripped.PEX_VERBOSE is None