Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve PEX_PATH handling in pex.py #421

Merged

Conversation

CMLivingston
Copy link
Contributor

@CMLivingston CMLivingston commented Oct 9, 2017

Replace all usages of self._vars.PEX_PATH with a unified pex path composed from merging pex_path in PEX-INFO and PEX_PATH set in the environment.

Problem:
pex_info.pex_path was being overwritten in _activate() in pex.py by the path set in the environment.

Solution:
Implement a util function to merge the paths into a single pex path for usage throughout pex.py, giving priority to the PEX-INFO pex path if both are set.

Chris Livingston added 4 commits October 9, 2017 14:26
…ath property of PEX-INFO metadata. Fallback to PEX_PATH set in the environment if neccesary for compatability.
…ified pex path for PEXEnvironment activation; Add associated integration test.
pex/pex.py Outdated
if pex_info.pex_path:
# set up other environments as specified in PEX_PATH
# set up other environments as specified in pex_path
for pex_path in filter(None, pex_info.pex_path.split(os.pathsep)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about instead of assigning to temp vars and then joining + mutating pex_info.pex_path here, just combining these into a new temporary list and then handling that directly?

also, would it make more sense to use self._vars.PEX_PATH as the source of the env var vs self._pex_info_overrides.pex_path? that's how it worked prior to your last change.

e.g.

# N.B. `PEX_PATH` entries written into `PEX-INFO` take precedence over those set in the environment.
combined_pex_path = self._merge_split(pex_info.pex_path, self._vars.PEX_PATH)
if combined_pex_path:
  for pex_path in combined_pex_path:
    ...

pex/pex.py Outdated
def _merge_split(self, *paths):
merged_paths = ":".join(filter(None, paths))
return filter(None, merged_paths.split(':'))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could reduce this to a one liner list comprehension:

def _merge_split(self, *paths):
  return [p for p in ':'.join(paths).split(':') if p]

with the if p bit doing the same as the filter(None, ...) above.

pex/pex.py Outdated
@@ -68,11 +68,15 @@ def _activate(self):

# set up the local .pex environment
pex_info = self._pex_info.copy()
pex_info_pex_path = pex_info.pex_path
overrides_pex_path = self._pex_info_overrides.pex_path
pex_info.update(self._pex_info_overrides)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that you found that this pex_info.update() call does not actually combine env vars + PEX-INFO, but instead clobbers one or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Env vars clobber the existing vars set by pex info.


# create a pex for environment PEX_PATH
pex3_path = os.path.join(output_dir, 'pex3.pex')
res3 = run_pex_command(['--disable-cache', 'sqlalchemy', '-o', pex3_path])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, iirc sqlalchemy is a pretty heavy dep. how about something lighter, like bitarray, enum34, mox etc?

@kwlzn kwlzn mentioned this pull request Oct 11, 2017
pex/pex.py Outdated
pex_info.update(self._pex_info_overrides)
pex_info.pex_path = ':'.join(filter(None, combined_pex_paths))
Copy link
Contributor Author

@CMLivingston CMLivingston Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figure it makes more sense to use a temp var here rather than construct a new pex info API to preserve the overrides where we want pex_info vars overwritten. Doing something like pex_info.merge_pex_paths_from_env(self._vars.PEX_PATH) wouldn't be possible within this context as it would require more temp vars/code to ensure that pex_info.pex_path comes first in the unified path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pex_info.merge_pex_paths_from_env(self._vars.PEX_PATH) should totally work, and it would be a cleaner API here in particular because it doesn't involve you as a caller having to remember to put the content of pex_info.pex_path at the head before setting it - it would just merge it for you.

the implementation would look approximately like this:

from pex.util import merge_split

class PexInfo(...):
  ...
  def merge_pex_path(self, pex_path):
    """Merges a new PEX_PATH definition into the existing one (if any).
    :param string pex_path: The PEX_PATH to merge.
    """
    if not pex_path:
      return

    self.pex_path = ':'.join(merge_split(self.pex_path, pex_path))

bonus points if you provide a flag on merge_split to enable inline deduping.

pex/pex.py Outdated
@@ -132,7 +137,7 @@ def _get_site_packages():
def site_libs(cls):
site_libs = cls._get_site_packages()
site_libs.update([sysconfig.get_python_lib(plat_specific=False),
sysconfig.get_python_lib(plat_specific=True)])
sysconfig.get_python_lib(plat_specific=True)])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unintentional re-indent?

pex/pex.py Outdated
@@ -276,10 +281,10 @@ def patch_all(path, path_importer_cache, modules):
patch_dict(sys.modules, modules)

old_sys_path, old_sys_path_importer_cache, old_sys_modules = (
sys.path[:], sys.path_importer_cache.copy(), sys.modules.copy())
sys.path[:], sys.path_importer_cache.copy(), sys.modules.copy())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unintentional re-indent?

pex/pex.py Outdated
pex_info.update(self._pex_info_overrides)
pex_info.pex_path = ':'.join(filter(None, combined_pex_paths))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pex_info.merge_pex_paths_from_env(self._vars.PEX_PATH) should totally work, and it would be a cleaner API here in particular because it doesn't involve you as a caller having to remember to put the content of pex_info.pex_path at the head before setting it - it would just merge it for you.

the implementation would look approximately like this:

from pex.util import merge_split

class PexInfo(...):
  ...
  def merge_pex_path(self, pex_path):
    """Merges a new PEX_PATH definition into the existing one (if any).
    :param string pex_path: The PEX_PATH to merge.
    """
    if not pex_path:
      return

    self.pex_path = ':'.join(merge_split(self.pex_path, pex_path))

bonus points if you provide a flag on merge_split to enable inline deduping.

Copy link
Contributor

@kwlzn kwlzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! two tiny straggler requests.

pex/util.py Outdated

def merge_split(*paths):
"""
Merge paths into a single path delimited by colons and split on colons to return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docstring should start on the opening line like:

"""Docstring starts here.

...
"""

otherwise it won't appear correctly in IDEs/help()/etc.

pex/util.py Outdated


def merge_split(*paths):
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a quick test for this in test_util.py would probably be good.

@CMLivingston CMLivingston changed the title Patch sys module using pex_path from PEX-INFO metadata Improve PEX_PATH handling in pex.py Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants