-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Mutually exclusive --target, --user and --prefix #4557
Changes from 18 commits
5232fbd
0b1d546
8c01518
5cba275
cf8e1a1
6c30bdd
fd8aa04
73df058
c3bd8ac
a6c1638
5333b78
4d970ab
fefa917
8bf1276
94a234f
c446591
91c835a
b395355
6e64754
8e58aca
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 @@ | ||
pip install ``--target``, ``--user``, and ``--prefix`` are now mutually exclusive. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,12 @@ | |
from tests.lib.local_repos import local_checkout | ||
|
||
|
||
if os.name == 'posix': | ||
distutils_cfg = os.path.expanduser('~/.pydistutils.cfg') | ||
else: | ||
distutils_cfg = os.path.expanduser('~\\.pydistutils.cfg') | ||
|
||
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. This isn't necessary. 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. Done. |
||
|
||
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.
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. Changing was probably a copy-paste error. Why it doesn't fail is another question. From https://github.com/python/cpython/blob/6f0eb93183519024cb360162bdd81b9faec97ba6/Lib/distutils/dist.py#L348 configs on Linux and Windows files are different, so 4 tests together that use this setting including https://github.com/techtonik/pip/blob/b395355b0188e64d0224a744b632333a5e3144d7/tests/functional/test_install_reqs.py#L232 are probably indifferent to those settings. 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 can't look into this, because I ditched Windows from my life. 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. On the second thought at least it should be possible to make them fail on both platforms by renaming the file to some nonsense. 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. You should be able to write a test that fails on Appveyor, then modify the code to resolve the test failure. Agreed, it's more painful if you don't have a local Windows system, but it's still doable. On the other hand, this all seems unrelated to the original point of the PR, which is simply about making the options mutually exclusive. Can't the original requirement be satisfied without changing this area of code at all? Sorry if I missed something, but there's no obvious comment in the thread explaining why we need to modify this. 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 can't run the single test due to #4878. |
||
@pytest.mark.network | ||
def test_requirements_file(script): | ||
""" | ||
|
@@ -229,13 +235,8 @@ def test_wheel_user_with_prefix_in_pydistutils_cfg( | |
# Make sure wheel is available in the virtualenv | ||
script.pip('install', 'wheel', '--no-index', '-f', common_wheels) | ||
virtualenv.system_site_packages = True | ||
if os.name == 'posix': | ||
user_filename = ".pydistutils.cfg" | ||
else: | ||
user_filename = "pydistutils.cfg" | ||
user_cfg = os.path.join(os.path.expanduser('~'), user_filename) | ||
script.scratch_path.join("bin").mkdir() | ||
with open(user_cfg, "w") as cfg: | ||
with open(distutils_cfg, "w") as cfg: | ||
cfg.write(textwrap.dedent(""" | ||
[install] | ||
prefix=%s""" % script.scratch_path)) | ||
|
@@ -249,6 +250,53 @@ def test_wheel_user_with_prefix_in_pydistutils_cfg( | |
assert 'installed requiresupper' in result.stdout | ||
|
||
|
||
def test_nowheel_user_with_prefix_in_pydistutils_cfg(script, data, virtualenv): | ||
virtualenv.system_site_packages = True | ||
with open(distutils_cfg, "w") as cfg: | ||
cfg.write(textwrap.dedent(""" | ||
[install] | ||
prefix=%s""" % script.scratch_path)) | ||
|
||
result = script.pip('install', '--no-binary=:all:', '--user', '--no-index', | ||
'-f', data.find_links, 'requiresupper', | ||
expect_stderr=True) | ||
assert 'installed requiresupper' in result.stdout | ||
|
||
|
||
@pytest.mark.network | ||
def test_wheel_target_with_prefix_in_pydistutils_cfg( | ||
script, data, virtualenv, common_wheels): | ||
# pip needs `wheel` to build `requiresupper` wheel before installing | ||
script.pip('install', 'wheel', '--no-index', '-f', common_wheels) | ||
with open(distutils_cfg, "w") as cfg: | ||
cfg.write(textwrap.dedent(""" | ||
[install] | ||
prefix=%s""" % script.scratch_path)) | ||
|
||
target_path = script.scratch_path / 'target' | ||
result = script.pip('install', '--target', target_path, '--no-index', | ||
'-f', data.find_links, '-f', common_wheels, | ||
'requiresupper') | ||
# Check that we are really installing a wheel | ||
assert 'Running setup.py install for requiresupper' not in result.stdout | ||
assert 'installed requiresupper' in result.stdout | ||
|
||
|
||
def test_nowheel_target_with_prefix_in_pydistutils_cfg(script, data, | ||
virtualenv): | ||
with open(distutils_cfg, "w") as cfg: | ||
cfg.write(textwrap.dedent(""" | ||
[install] | ||
prefix=%s""" % script.scratch_path)) | ||
|
||
target_path = script.scratch_path / 'target' | ||
result = script.pip('install', '--no-binary=:all:', | ||
'--target', target_path, | ||
'--no-index', '-f', data.find_links, 'requiresupper', | ||
expect_stderr=True) | ||
assert 'installed requiresupper' in result.stdout | ||
|
||
|
||
def test_install_option_in_requirements_file(script, data, virtualenv): | ||
""" | ||
Test --install-option in requirements file overrides same option in cli | ||
|
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.
this is a toxic destructive test - if ever any user with a preconfigured configfile runs it, they will face a lost configuration, and potentially hours or days of figuring why something they knew worked is suddenly broken
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.
I just copied that from the test below to avoid code duplication in many places. I have no idea how this config file is supposed to be used.
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.
The code in current
master
does the same.pip/tests/functional/test_install_reqs.py
Lines 232 to 236 in 404838a
So yes, it points to user HOME, but I thought that test framework should mock that, no?
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.
@RonnyPfannschmidt so any ideas how to change that?
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.
This is fine since we have an
isolate
fixture to isolate the tests from the actual system, which is used whenvirtualenv
fixture is used.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.
AFAIK, by the time we isolate, it's too late, since this is done during collection.
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.
Ah right. The execution during collection can be deferred using a function then, with the appropriate comment on why it's a function.