Fix PEXBuilder.clone. (pex-tool#575)

Previously clone copied distributions twice. One copy was made by the
underlying `Chroot`, and then another was manually performed. The
manual copy was only needed to update the internal `_distributions`
set, so just do that.

A test that failed before the fix is added to ensure distributions with
unwriteable files can be cloned.

Fixes pex-tool#570

pex/pex_builder.py

@@ -74,15 +74,16 @@ def __init__(self, path=None, interpreter=None, chroot=None, pex_info=None, prea
       The temporary directory created when ``path`` is not specified is now garbage collected on
       interpreter exit.
     """
-    self._chroot = chroot or Chroot(path or safe_mkdtemp())
-    self._frozen = False
     self._interpreter = interpreter or PythonInterpreter.get()
-    self._shebang = self._interpreter.identity.hashbang()
-    self._logger = logging.getLogger(__name__)
+    self._chroot = chroot or Chroot(path or safe_mkdtemp())
+    self._pex_info = pex_info or PexInfo.default(self._interpreter)
     self._preamble = to_bytes(preamble or '')
     self._copy = copy
+
+    self._shebang = self._interpreter.identity.hashbang()
+    self._logger = logging.getLogger(__name__)
+    self._frozen = False
     self._distributions = set()
-    self._pex_info = pex_info or PexInfo.default(interpreter)
 
   def _ensure_unfrozen(self, name='Operation'):
     if self._frozen:
@@ -110,14 +111,13 @@ def clone(self, into=None):
     """
     chroot_clone = self._chroot.clone(into=into)
     clone = self.__class__(
-        chroot=chroot_clone,
-        interpreter=self._interpreter,
-        pex_info=self._pex_info.copy(),
-        preamble=self._preamble,
-        copy=self._copy)
+      chroot=chroot_clone,
+      interpreter=self._interpreter,
+      pex_info=self._pex_info.copy(),
+      preamble=self._preamble,
+      copy=self._copy)
     clone.set_shebang(self._shebang)
-    for dist in self._distributions:
-      clone.add_distribution(dist)
+    clone._distributions = self._distributions.copy()
     return clone
 
   def path(self):
@@ -286,14 +286,12 @@ def _add_dist_zip(self, path, dist_name):
       os.mkdir(whltmp)
       wf = WheelFile(path)
       wf.install(overrides=self._get_installer_paths(whltmp), force=True)
-      for (root, _, files) in os.walk(whltmp):
+      for root, _, files in os.walk(whltmp):
         pruned_dir = os.path.relpath(root, tmp)
         for f in files:
           fullpath = os.path.join(root, f)
-          if os.path.isdir(fullpath):
-            continue
           target = os.path.join(self._pex_info.internal_cache, pruned_dir, f)
-          self._chroot.copy(fullpath, target)
+          self._copy_or_link(fullpath, target)
       return CacheHelper.dir_hash(whltmp)
 
     with open_zip(path) as zf:

pex/testing.py

@@ -66,20 +66,22 @@ def get_dep_dist_names_from_pex(pex_path, match_prefix=''):
 
 
 @contextlib.contextmanager
-def temporary_content(content_map, interp=None, seed=31337):
+def temporary_content(content_map, interp=None, seed=31337, perms=0o644):
   """Write content to disk where content is map from string => (int, string).
 
      If target is int, write int random bytes.  Otherwise write contents of string."""
   random.seed(seed)
   interp = interp or {}
   with temporary_dir() as td:
     for filename, size_or_content in content_map.items():
-      safe_mkdir(os.path.dirname(os.path.join(td, filename)))
-      with open(os.path.join(td, filename), 'wb') as fp:
+      dest = os.path.join(td, filename)
+      safe_mkdir(os.path.dirname(dest))
+      with open(dest, 'wb') as fp:
         if isinstance(size_or_content, int):
           fp.write(random_bytes(size_or_content))
         else:
           fp.write((size_or_content % interp).encode('utf-8'))
+      os.chmod(dest, perms)
     yield td
 
 

tests/test_bdist_pex.py

@@ -2,12 +2,14 @@
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 import os
+import stat
 import subprocess
 import sys
 from textwrap import dedent
 
 from twitter.common.contextutil import pushd
 
+from pex.common import open_zip
 from pex.testing import temporary_content
 
 
@@ -81,3 +83,52 @@ def test_pex_args_shebang_with_spaces():
 
 def test_pex_args_shebang_without_spaces():
   assert_pex_args_shebang('#!/usr/bin/python')
+
+
+def test_unwriteable_contents():
+  my_app_setup_py = dedent("""
+      from setuptools import setup
+
+      setup(
+        name='my_app',
+        version='0.0.0',
+        zip_safe=True,
+        packages=['my_app'],
+        include_package_data=True,
+        package_data={'my_app': ['unwriteable.so']},
+      )
+    """)
+
+  UNWRITEABLE_PERMS = 0o400
+  with temporary_content({'setup.py': my_app_setup_py,
+                          'my_app/__init__.py': '',
+                          'my_app/unwriteable.so': ''},
+                         perms=UNWRITEABLE_PERMS) as my_app_project_dir:
+    with pushd(my_app_project_dir):
+      subprocess.check_call([sys.executable, 'setup.py', 'bdist_wheel'])
+
+    uses_my_app_setup_py = dedent("""
+      from setuptools import setup
+
+      setup(
+        name='uses_my_app',
+        version='0.0.0',
+        zip_safe=True,
+        install_requires=['my_app'],
+      )
+    """)
+    with temporary_content({'setup.py': uses_my_app_setup_py}) as uses_my_app_project_dir:
+      with pushd(uses_my_app_project_dir):
+        subprocess.check_call([sys.executable,
+                               'setup.py',
+                               'bdist_pex',
+                               '--pex-args=--disable-cache --no-pypi -f {}'
+                              .format(os.path.join(my_app_project_dir, 'dist'))])
+
+        with open_zip('dist/uses_my_app-0.0.0.pex') as zf:
+          unwriteable_sos = [path for path in zf.namelist()
+                             if path.endswith('my_app/unwriteable.so')]
+          assert 1 == len(unwriteable_sos)
+          unwriteable_so = unwriteable_sos.pop()
+          zf.extract(unwriteable_so)
+          assert UNWRITEABLE_PERMS == stat.S_IMODE(os.stat(unwriteable_so).st_mode)