Added the possibility to follow symlinks on copytree also for py3 #3707

alquerci · 2016-05-20T23:58:17Z

Hello,

Reply to #1311

It's an answer of https://github.com/pypa/pip/pull/1311/files#r7640350

If a project contains true NTFS symlinks it require elevated privileges indeed (source: @docs.python: os.symlink and @Wikipedia: NTFS_symbolic_link)

Fix shutil.copytree for py3 when a symlink points to a directory

Prevent circular symbolic links

Add test with circular symbolic links

revert f579c17

It should be also update tests.lib.path.Path.copytree method (e.g. git revert 99a4f6f) but it will add a dependency to pip.util module.

All comments are welcome

This was migrated from #1329 to reparent it to the master branch. Please see original pull request for any previous discussion.

rebase it against master

This change is

alquerci · 2016-05-21T03:15:31Z

Seems has been fixed since Python 3.5 travis build and py3.5 source.

dstufft · 2016-05-26T10:46:53Z

You said this has been fixed- Does that mean this PR is no longer needed? Or that it's needed for Python's older then 3.5?

alquerci · 2016-05-28T01:27:29Z

Does that mean this PR is no longer needed?

no, it keep needed, details are follow:

233bbba Fixed shutil.copytree for py3 when a symlink points to a directory
Fix for python version > 2 and < 3.5
145e0b6 Ignored circular symbolic links when copytree
Fix for all Python versions

cc @dstufft

BrownTruck · 2017-04-01T18:00:15Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pradyunsg · 2017-08-21T09:45:36Z

Hi!

@alquerci Could you by any chance fix the conflicts in this PR?

Sidenote: The tests seem to be failing on AppVeyor.

alquerci · 2017-08-25T21:36:52Z

Hello @pradyunsg

I will start to do it by tomorrow.

This reverts commit f579c17.

alquerci · 2017-08-26T09:15:30Z

@pradyunsg All type of fixes are done (conflicts, lints, news).

pradyunsg

Mostly just minor nits and a few misc questions.

I've not checked the functionality of the PR since I haven't read the previous discussions on this PR yet. :)

pradyunsg · 2017-08-26T10:08:51Z

news/3707.bugfix

@@ -0,0 +1,3 @@
+1. Revert previous fix that disable symlinks dereference.


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?

pradyunsg · 2017-08-26T10:09:55Z

pip/compat.py

+                    os.symlink(linkto, dst)
+                    return dst
+                except OSError:
+                    # catch the OSError when the os.symlink function is called


pradyunsg · 2017-08-26T10:10:42Z

pip/exceptions.py

+            'Circular reference detected in "%s" ("%s" > "%s").'
+            '' % (resources[0], '" > "'.join(resources), resources[0]),
+        )
+        PipError.__init__(self, message)


nit: Use super(CircularSymlinkException, self).__init__(message)

pradyunsg · 2017-08-26T10:11:56Z

pip/utils/__init__.py

+def validate_path(path):
+    """Detect circular symbolic link
+
+    @raise CircularSymlinkException: When a symlink loop was found


nit: Use plain English; don't tag with @raise since that isn't something consistent with the rest of the codebase.

pradyunsg · 2017-08-26T10:12:06Z

pip/utils/__init__.py

+def copytree_ignore_callback(src, names):
+    """Ignore circular symbolic links
+
+    @return: set of ignores names


nit: Use plain English; as above.

pradyunsg · 2017-08-26T10:12:19Z

pip/utils/__init__.py

+
+
+def copytree(source, location, symlinks=False):
+    return compat.copytree(


I think it makes sense to have a docstring here describing why we don't simply reuse shutil.copytree.

pradyunsg · 2017-08-26T10:12:37Z

pip/utils/__init__.py

+    )
+
+
+def copytree_ignore_callback(src, names):


Maybe make this _copytree_ignore_callback?

pradyunsg · 2017-08-26T12:32:56Z

tests/unit/test_utils.py

+        finally:
+            os.unlink(symbar_link)
+
+    @pytest.mark.skipif(


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")

pradyunsg · 2017-08-26T13:58:51Z

pip/utils/__init__.py

 import tarfile
 import zipfile

+# For copytree as when using from an import exception is thrown.


I'm curious, what are you seeing?

pradyunsg · 2017-08-26T14:09:49Z

tests/unit/test_utils.py

+            os.symlink(foo_file, symfoo_link)
+            validate_path(symfoo_link)
+        except OSError:
+            return


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.

pradyunsg · 2017-08-26T14:15:33Z

Thanks for updating this @alquerci! ^.^

BrownTruck · 2017-09-01T08:00:10Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pradyunsg · 2017-10-03T20:04:44Z

@alquerci Could you fix the conflicts in this PR; again?

pradyunsg · 2017-11-04T05:01:54Z

Closing due to lack of a response.

lock · 2019-06-02T16:06:18Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

alquerci mentioned this pull request May 20, 2016

Add the possibility to follow symlinks on copytree also for py3 #1329

Closed

4 tasks

alquerci changed the title ~~[WIP] Added the possibility to follow symlinks on copytree also for py3~~ Added the possibility to follow symlinks on copytree also for py3 May 21, 2016

dstufft added the migrated from develop label May 21, 2016

xavfernandez closed this Jan 6, 2017

xavfernandez reopened this Jan 6, 2017

BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 1, 2017

pradyunsg added the S: awaiting response Waiting for a response/more information label Aug 21, 2017

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 26, 2017

alquerci added 3 commits August 26, 2017 09:30

Revert "Keep symlinks as symlinks when installing"

ad178c9

This reverts commit f579c17.

Fixed shutil.copytree for py3 when a symlink points to a directory

dc78f9e

Ignored circular symbolic links when copytree

7d64a09

pradyunsg reviewed Aug 26, 2017

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017

pradyunsg removed the S: awaiting response Waiting for a response/more information label Nov 4, 2017

pradyunsg closed this Nov 4, 2017

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

pradyunsg removed the needs rebase or merge PR has conflicts with current master label Apr 2, 2021

		@@ -0,0 +1,3 @@
		1. Revert previous fix that disable symlinks dereference.



		def copytree(source, location, symlinks=False):
		return compat.copytree(

Uh oh!

Added the possibility to follow symlinks on copytree also for py3 #3707

Added the possibility to follow symlinks on copytree also for py3 #3707

Uh oh!

Conversation

alquerci commented May 20, 2016 • edited by dstufft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alquerci commented May 21, 2016

Uh oh!

dstufft commented May 26, 2016

Uh oh!

alquerci commented May 28, 2016

Uh oh!

BrownTruck commented Apr 1, 2017

Uh oh!

pradyunsg commented Aug 21, 2017

Uh oh!

alquerci commented Aug 25, 2017

Uh oh!

alquerci commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradyunsg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg commented Aug 26, 2017

Uh oh!

BrownTruck commented Sep 1, 2017

Uh oh!

pradyunsg commented Oct 3, 2017

Uh oh!

pradyunsg commented Nov 4, 2017

Uh oh!

lock bot commented Jun 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alquerci commented May 20, 2016 •

edited by dstufft

Loading

alquerci commented Aug 26, 2017 •

edited

Loading

pradyunsg left a comment •

edited

Loading