Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 19, 2013
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
048c976
Add failing test case for installing when there are symlinks to direc…
mwilliamson Nov 9, 2013
f579c17
Keep symlinks as symlinks when installing
myint Jun 26, 2013
99a4f6f
Add symlinks=True to copytree invocation so that tests pass in Python 3
mwilliamson Nov 10, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pip/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure. wanna check it? : )

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

else:
unpack_file(source, location, content_type, link)

Expand Down
Empty file added tests/data/packages/symlinks/doc/intro
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions tests/data/packages/symlinks/docs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
doc
3 changes: 3 additions & 0 deletions tests/data/packages/symlinks/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[egg_info]
tag_build = dev
tag_svn_revision = true
8 changes: 8 additions & 0 deletions tests/data/packages/symlinks/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from setuptools import setup

version = '0.1'

setup(name='symlinks',
version=version,
packages=["symlinks"],
)
1 change: 1 addition & 0 deletions tests/data/packages/symlinks/symlinks/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#
12 changes: 12 additions & 0 deletions tests/functional/test_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,18 @@ def test_install_from_local_directory(script, data):
assert egg_info_folder in result.files_created, str(result)


def test_install_from_local_directory_with_symlinks_to_directories(script, data):
"""
Test installing from a local directory containing symlinks to directories.
"""
to_install = data.packages.join("symlinks")
result = script.pip('install', to_install, expect_error=False)
pkg_folder = script.site_packages/'symlinks'
egg_info_folder = script.site_packages/'symlinks-0.1dev-py%s.egg-info' % pyversion
assert pkg_folder in result.files_created, str(result.stdout)
assert egg_info_folder in result.files_created, str(result)


def test_install_from_local_directory_with_no_setup_py(script, data):
"""
Test installing from a local directory with no 'setup.py'.
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ def copytree(self, to):
"""
Copies a directory tree to another path.
"""
return shutil.copytree(self, to)
return shutil.copytree(self, to, symlinks=True)

def move(self, to):
"""
Expand Down