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

Fix: "pip install ." fails on Python 3.3 (with symlinks in local directory) #1311

Merged
merged 3 commits into from Nov 19, 2013

Conversation

Projects
None yet
6 participants
@mwilliamson
Contributor

mwilliamson commented Nov 10, 2013

This pull request (hopefully) fixes GitHub issue #1006, and includes a separate test case for the issue.

mwilliamson and others added some commits Nov 9, 2013

Keep symlinks as symlinks when installing
Without this, "pip install" crashes when encountering symlinks that
point to external paths. The culprit is "shutil.copytree()", which
throws an error in such cases if the "symlinks" parameter is not set to
"True".

This fixes #1006.
@alquerci

This comment has been minimized.

Show comment
Hide comment
@alquerci

alquerci commented Nov 11, 2013

duplicate #813

@myint

This comment has been minimized.

Show comment
Hide comment
@myint

myint Nov 12, 2013

Contributor

Wow, three pull requests already and still not merged.

Contributor

myint commented Nov 12, 2013

Wow, three pull requests already and still not merged.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Nov 12, 2013

Contributor

ok, marking for 1.5. looking now.

Contributor

qwcode commented Nov 12, 2013

ok, marking for 1.5. looking now.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Nov 12, 2013

Contributor

ok, giving this a tentative merge +1, but want someone else to confirm it .

my admitted paranoia with this, was that it can somehow add a security risk.

this change specifically deals with copying file:// directories, which comes up when doing non-editable installs of local project directories. pip copies the src to a build directory, then installs it from there.

the change here is from:

  • before: symlinks don't make it from src to build; they get hardened during the copy. (atleast that worked in py2, it fails in py3, which is what the fix is about)
  • after: in py2 and py3, symlinks will make it from src to the build dir, but they still get hardened during the install (by setuptools I presume; my testing confirms this)

cc @dstufft @pnasrat @pfmoore

Contributor

qwcode commented Nov 12, 2013

ok, giving this a tentative merge +1, but want someone else to confirm it .

my admitted paranoia with this, was that it can somehow add a security risk.

this change specifically deals with copying file:// directories, which comes up when doing non-editable installs of local project directories. pip copies the src to a build directory, then installs it from there.

the change here is from:

  • before: symlinks don't make it from src to build; they get hardened during the copy. (atleast that worked in py2, it fails in py3, which is what the fix is about)
  • after: in py2 and py3, symlinks will make it from src to the build dir, but they still get hardened during the install (by setuptools I presume; my testing confirms this)

cc @dstufft @pnasrat @pfmoore

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Nov 12, 2013

Contributor

also, I confirmed the test fails without the symlinks=True change, which is what we want.
(the 2nd symlink=True change in the tests lib is needed to copy the symlinks test data around during the setup of the functional tests)

Contributor

qwcode commented Nov 12, 2013

also, I confirmed the test fails without the symlinks=True change, which is what we want.
(the 2nd symlink=True change in the tests lib is needed to copy the symlinks test data around during the setup of the functional tests)

@@ -359,7 +359,7 @@ def unpack_file_url(link, location):
# delete the location since shutil will create it again :(
if os.path.isdir(location):
rmtree(location)
shutil.copytree(source, location)
shutil.copytree(source, location, symlinks=True)

This comment has been minimized.

@pfmoore

pfmoore Nov 12, 2013

Member

Presumably this works on Windows? Is it ignored, or does it attempt to copy symlinks? If the latter, then it may fail because the pip process is not elevated (creating symlinks needs an elevated process on Windows).

@pfmoore

pfmoore Nov 12, 2013

Member

Presumably this works on Windows? Is it ignored, or does it attempt to copy symlinks? If the latter, then it may fail because the pip process is not elevated (creating symlinks needs an elevated process on Windows).

This comment has been minimized.

@qwcode

qwcode Nov 12, 2013

Contributor

not sure. wanna check it? : )

@qwcode

qwcode Nov 12, 2013

Contributor

not sure. wanna check it? : )

This comment has been minimized.

@pfmoore

pfmoore Nov 12, 2013

Member

It'll be a few days before I can, at best. I'm assuming that the test will fail on Windows because setting up a test directory with symlinks needs an elevated process. Running some tests to make sure "normal use" still works is more what matters to me, but I don't know what we'd need to do to make sure this code is exercised in a non-symlink case.

It's probably a non-issue, though, I did a quick test and copytree(..., symlinks=True) works unchanged when the source directory doesn't have symlinks, so there should be no issue.

@pfmoore

pfmoore Nov 12, 2013

Member

It'll be a few days before I can, at best. I'm assuming that the test will fail on Windows because setting up a test directory with symlinks needs an elevated process. Running some tests to make sure "normal use" still works is more what matters to me, but I don't know what we'd need to do to make sure this code is exercised in a non-symlink case.

It's probably a non-issue, though, I did a quick test and copytree(..., symlinks=True) works unchanged when the source directory doesn't have symlinks, so there should be no issue.

This comment has been minimized.

@qwcode

qwcode Nov 13, 2013

Contributor

I don't think this test will fail on windows, but rather, it's not valuable on windows. these are "linux" symlinks (in the "symlinks" test package that was added), not NTFS symlinks, so they're just files to windows. but I don't know windows that well, so I may be wrong.

2 things need to be confirmed on windows. If a project contains true NTFS symlinks:

  1. how does our current code behave in py3? does it fail?
  2. how does it behave after the change? require elevated privileges?

I have to pass on looking into this. Someone who's more comfortable with windows, or more motivated to get this change, needs to confirm it.

@qwcode

qwcode Nov 13, 2013

Contributor

I don't think this test will fail on windows, but rather, it's not valuable on windows. these are "linux" symlinks (in the "symlinks" test package that was added), not NTFS symlinks, so they're just files to windows. but I don't know windows that well, so I may be wrong.

2 things need to be confirmed on windows. If a project contains true NTFS symlinks:

  1. how does our current code behave in py3? does it fail?
  2. how does it behave after the change? require elevated privileges?

I have to pass on looking into this. Someone who's more comfortable with windows, or more motivated to get this change, needs to confirm it.

This comment has been minimized.

@pfmoore

pfmoore Nov 13, 2013

Member

I'm not going to do this, it's not that important to me, and working with symlinks on Windows is a pain. But if someone does want to get this in, I'm not going to block it on the basis of a theoretical risk.

@pfmoore

pfmoore Nov 13, 2013

Member

I'm not going to do this, it's not that important to me, and working with symlinks on Windows is a pain. But if someone does want to get this in, I'm not going to block it on the basis of a theoretical risk.

This comment has been minimized.

@alquerci

alquerci Nov 13, 2013

@qwcode
(1) look at this #813 (comment) obviously need testing
(2) it require elevated privileges indeed (source: @docs.python: os.symlink and @wikipedia: NTFS_symbolic_link)

@alquerci

alquerci Nov 13, 2013

@qwcode
(1) look at this #813 (comment) obviously need testing
(2) it require elevated privileges indeed (source: @docs.python: os.symlink and @wikipedia: NTFS_symbolic_link)

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Nov 19, 2013

Member

This looks fine to me, I'll merge it.

Member

dstufft commented Nov 19, 2013

This looks fine to me, I'll merge it.

dstufft added a commit that referenced this pull request Nov 19, 2013

Merge pull request #1311 from mwilliamson/py3-symlinks
Fix: "pip install ." fails on Python 3.3 (with symlinks in local directory)

@dstufft dstufft merged commit 68eb2c4 into pypa:develop Nov 19, 2013

1 check passed

default The Travis CI build passed
Details
@myint

This comment has been minimized.

Show comment
Hide comment
@myint

myint Nov 19, 2013

Contributor

Thanks.

Contributor

myint commented Nov 19, 2013

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment