-
Couldn't load subscription status.
- Fork 3.2k
Added the possibility to follow symlinks on copytree also for py3 #3707
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| 1. Revert previous fix that disable symlinks dereference. | ||
| 2. Fixed shutil.copytree for py3 when a symlink points to a directory (python version > 2 and < 3.5). | ||
| 3. Ignored circular symbolic links for copytree execution. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import locale | ||
| import logging | ||
| import os | ||
| import shutil | ||
| import sys | ||
|
|
||
| from pip._vendor.six import text_type | ||
|
|
@@ -124,6 +125,63 @@ def native_str(s, replace=False): | |
| return s.encode('utf-8') | ||
| return s | ||
|
|
||
| if sys.version_info >= (3,) and sys.version_info < (3, 5): | ||
| def copytree(source, location, symlinks=False, ignore=None): | ||
| # The py3k version of `shutil.copytree` fails when symlinks point on | ||
| # directories. | ||
| follow_symlinks = not symlinks | ||
| copying = [] | ||
|
|
||
| def copy_callback(src, dst, follow_symlinks=follow_symlinks): | ||
| if not follow_symlinks and os.path.islink(src): | ||
| linkto = os.readlink(src) | ||
| if not os.path.isabs(linkto): | ||
| linkto = os.path.join(os.path.dirname(src), linkto) | ||
| try: | ||
| os.symlink(linkto, dst) | ||
| return dst | ||
| except OSError: | ||
| # catch the OSError when the os.symlink function is called | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Catch |
||
| # on Windows by an unprivileged user. In that case we pass | ||
| # follow_symlinks to True | ||
| follow_symlinks = True | ||
| src = os.path.normcase(os.path.realpath(src)) | ||
| if src in copying: | ||
| # Already seen this path, so we must have a symlink loop | ||
| raise Exception( | ||
| 'Circular reference detected in "%s" ("%s" > "%s").' | ||
| '' % (copying[0], '" > "'.join(copying), copying[0]) | ||
| ) | ||
| copying.append(src) | ||
| if os.path.isdir(src): | ||
| shutil.copytree( | ||
| src, | ||
| dst, | ||
| symlinks=not follow_symlinks, | ||
| ignore=ignore, | ||
| copy_function=copy_callback, | ||
| ) | ||
| else: | ||
| shutil.copy2(src, dst) | ||
| copying.remove(src) | ||
| return dst | ||
| return shutil.copytree( | ||
| source, | ||
| location, | ||
| symlinks=symlinks, | ||
| ignore=ignore, | ||
| copy_function=copy_callback, | ||
| ) | ||
|
|
||
| else: | ||
| def copytree(source, location, symlinks=False, ignore=None): | ||
| return shutil.copytree( | ||
| source, | ||
| location, | ||
| symlinks=symlinks, | ||
| ignore=ignore, | ||
| ) | ||
|
|
||
|
|
||
| def get_path_uid(path): | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -246,3 +246,13 @@ def hash_then_or(hash_name): | |
| class UnsupportedPythonVersion(InstallationError): | ||
| """Unsupported python version according to Requires-Python package | ||
| metadata.""" | ||
|
|
||
|
|
||
| class CircularSymlinkException(PipError): | ||
| """When a circular symbolic link is detected.""" | ||
| def __init__(self, resources): | ||
| message = ( | ||
| 'Circular reference detected in "%s" ("%s" > "%s").' | ||
| '' % (resources[0], '" > "'.join(resources), resources[0]), | ||
| ) | ||
| PipError.__init__(self, message) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Use |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ | |
| import tarfile | ||
| import zipfile | ||
|
|
||
| # For copytree as when using from an import exception is thrown. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious, what are you seeing? |
||
| import pip.compat as compat | ||
| from pip.exceptions import InstallationError | ||
| from pip.compat import console_to_str, expanduser, stdlib_pkgs | ||
| from pip.locations import ( | ||
|
|
@@ -28,6 +30,7 @@ | |
| from pip._vendor.six.moves import input | ||
| from pip._vendor.six import PY2 | ||
| from pip._vendor.retrying import retry | ||
| from pip.exceptions import CircularSymlinkException | ||
|
|
||
| if PY2: | ||
| from io import BytesIO as StringIO | ||
|
|
@@ -862,3 +865,48 @@ def enum(*sequential, **named): | |
| reverse = dict((value, key) for key, value in enums.items()) | ||
| enums['reverse_mapping'] = reverse | ||
| return type('Enum', (), enums) | ||
|
|
||
|
|
||
| def validate_path(path): | ||
| """Detect circular symbolic link | ||
|
|
||
| @raise CircularSymlinkException: When a symlink loop was found | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Use plain English; don't tag with |
||
| """ | ||
| paths_seen = [] | ||
| if not os.path.isabs(path): | ||
| path = os.path.abspath(path) | ||
| while os.path.islink(path): | ||
| path = os.path.normcase(os.path.normpath(path)) | ||
| if path in paths_seen: | ||
| # Already seen this path, so we must have a symlink loop | ||
| raise CircularSymlinkException(paths_seen) | ||
| paths_seen.append(path) | ||
| # Resolve where the link points to | ||
| resolved = os.readlink(path) | ||
| if not os.path.isabs(resolved): | ||
| resolved = os.path.join(os.path.dirname(path), resolved) | ||
| path = resolved | ||
|
|
||
|
|
||
| def copytree(source, location, symlinks=False): | ||
| return compat.copytree( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to have a docstring here describing why we don't simply reuse |
||
| source, | ||
| location, | ||
| symlinks=symlinks, | ||
| ignore=copytree_ignore_callback, | ||
| ) | ||
|
|
||
|
|
||
| def copytree_ignore_callback(src, names): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe make this |
||
| """Ignore circular symbolic links | ||
|
|
||
| @return: set of ignores names | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Use plain English; as above. |
||
| """ | ||
| ignores = set() | ||
| for name in names: | ||
| path = os.path.join(src, name) | ||
| try: | ||
| validate_path(path) | ||
| except CircularSymlinkException: | ||
| ignores.add(name) | ||
| return ignores | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,17 +17,19 @@ | |
| from pip._vendor.six import BytesIO | ||
|
|
||
| from pip.exceptions import ( | ||
| HashMismatch, HashMissing, InstallationError, UnsupportedPythonVersion | ||
| CircularSymlinkException, HashMismatch, HashMissing, InstallationError, | ||
| UnsupportedPythonVersion | ||
| ) | ||
| from pip.utils import ( | ||
| egg_link_path, ensure_dir, get_installed_distributions, normalize_path, | ||
| rmtree, untar_file, unzip_file | ||
| rmtree, untar_file, unzip_file, validate_path | ||
| ) | ||
| from pip.utils.encoding import auto_decode | ||
| from pip.utils.glibc import check_glibc_version | ||
| from pip.utils.hashes import Hashes, MissingHashes | ||
| from pip.utils.packaging import check_dist_requires_python | ||
| from pip.utils.temp_dir import TempDirectory | ||
| from tests.lib import DATA_DIR, Path | ||
|
|
||
|
|
||
| class Tests_EgglinkPath: | ||
|
|
@@ -592,3 +594,74 @@ def test_check_requires(self, metadata, should_raise): | |
| check_dist_requires_python(fake_dist) | ||
| else: | ||
| check_dist_requires_python(fake_dist) | ||
|
|
||
|
|
||
| class TestValidatePath(object): | ||
| def setup(self): | ||
| self.tempdir = tempfile.mkdtemp() | ||
| self.old_mask = os.umask(0o022) | ||
| self.dir = DATA_DIR.join('util').join('bar') | ||
| self.file = self.dir.join('foo') | ||
| self.symlinksDir = Path(self.tempdir) | ||
|
|
||
| def teardown(self): | ||
| os.umask(self.old_mask) | ||
| shutil.rmtree(self.tempdir, ignore_errors=True) | ||
|
|
||
| def testRegularFile(self): | ||
| foo_file = self.file.join('foo') | ||
| validate_path(foo_file) | ||
|
|
||
| @pytest.mark.skipif( | ||
| not hasattr(os, 'symlink'), | ||
| reason="requires os.symlink", | ||
| ) | ||
| def testRegularSymlinkToFile(self): | ||
| foo_file = self.file | ||
| symfoo_link = self.symlinksDir.join('symfoo') | ||
| try: | ||
| os.symlink(foo_file, symfoo_link) | ||
| validate_path(symfoo_link) | ||
| except OSError: | ||
| return | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip the test or make it fail if you come inside the except; maybe don't actually catch it? Same for later cases as well. |
||
| finally: | ||
| os.unlink(symfoo_link) | ||
|
|
||
| @pytest.mark.skipif( | ||
| not hasattr(os, 'symlink'), | ||
| reason="requires os.symlink", | ||
| ) | ||
| def testRegularSymlinkToDirectory(self): | ||
| bar_dir = self.dir | ||
| symbar_link = self.symlinksDir.join('symbar') | ||
| try: | ||
| os.symlink(bar_dir, symbar_link) | ||
| validate_path(symbar_link) | ||
| except OSError: | ||
| return | ||
| finally: | ||
| os.unlink(symbar_link) | ||
|
|
||
| @pytest.mark.skipif( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: You're using the same decorator in multiple places; you can simply make it into a module level underscored variable and decorate functions with it. needs_os_symlink = pytest.mark.skipif(not hasattr(os, "symlink"), reason="requires os.symlink") |
||
| not hasattr(os, 'symlink'), | ||
| reason="requires os.symlink", | ||
| ) | ||
| def testCircularSymlink(self): | ||
| foo_file = self.file.join('foo') | ||
| symfoo_link = self.symlinksDir.join('symfoo') | ||
| to_symfoo_link = self.symlinksDir.join('to_symfoo') | ||
| to_to_symfoo_link = self.symlinksDir.join('to_to_symfoo') | ||
| try: | ||
| os.symlink(foo_file, symfoo_link) | ||
| os.symlink(symfoo_link, to_symfoo_link) | ||
| os.symlink(to_symfoo_link, to_to_symfoo_link) | ||
| os.unlink(symfoo_link) | ||
| os.symlink(to_to_symfoo_link, symfoo_link) | ||
| with pytest.raises(CircularSymlinkException): | ||
| validate_path(symfoo_link) | ||
| except OSError: | ||
| return | ||
| finally: | ||
| os.unlink(symfoo_link) | ||
| os.unlink(to_symfoo_link) | ||
| os.unlink(to_to_symfoo_link) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't have a numbered list here. The news fragments get aggregated into a list. This would result in a list containing a list - nobody wants that in NEWS. :P
Having a commit message style summary line followed by a small passage describing the change would probably be a better format. :)
This suggests to me that this PR is doing multiple things at the same time; if so, is it possible to split them?