From 7654a4c762247a94760b1d593e84b1ca710a3c34 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Tue, 4 Aug 2015 16:00:28 -0600 Subject: [PATCH] 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 | 8 ++++++++ pex/pex_info.py | 22 ++++++++++++---------- pex/variables.py | 19 +++++++++++++++---- tests/test_pex_info.py | 31 +++++++++++++++++++++++++++++++ tests/test_variables.py | 17 +++++++++++++++++ 5 files changed, 83 insertions(+), 14 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index b6f904677..6d6e0dda5 100644 --- a/CHANGES.rst +++ b/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 `_. + ----- 1.0.1 ----- diff --git a/pex/pex_info.py b/pex/pex_info.py index 17ba97792..d4e88242e 100644 --- a/pex/pex_info.py +++ b/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()) diff --git a/pex/variables.py b/pex/variables.py index 2923a62e9..059459751 100644 --- a/pex/variables.py +++ b/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): diff --git a/tests/test_pex_info.py b/tests/test_pex_info.py index dd7f2e664..032b221da 100644 --- a/tests/test_pex_info.py +++ b/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))) diff --git a/tests/test_variables.py b/tests/test_variables.py index 2852678ca..e97698567 100644 --- a/tests/test_variables.py +++ b/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