Skip to content
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

Prevent distutils option error target prefix conflict #6008

Conversation

@jaraco
Copy link
Member

commented Nov 12, 2018

Fixes #4106

  • Add news fragment
@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Here is a test:

 tests/functional/test_install.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git i/tests/functional/test_install.py w/tests/functional/test_install.py
index 40504a18..d93d68c7 100644
--- i/tests/functional/test_install.py
+++ w/tests/functional/test_install.py
@@ -1444,3 +1444,19 @@ def test_install_conflict_warning_can_be_suppressed(script, data):
         'install', '--no-index', pkgB_path, '--no-warn-conflicts'
     )
     assert "Successfully installed pkgB-2.0" in result2.stdout, str(result2)
+
+
+def test_target_install_ignores_distutils_config_install_prefix(script):
+    prefix = script.scratch_path / 'prefix'
+    Path(os.environ['HOME'], '.pydistutils.cfg').write(textwrap.dedent(
+        '''
+        [install]
+        prefix=%s
+        ''' % str(prefix)))
+    target = script.scratch_path / 'target'
+    result = script.pip_install_local('simplewheel', '-t', target)
+    assert (
+        "Successfully installed simplewheel" in result.stdout and
+        (target - script.base_path) in result.files_created and
+        (prefix - script.base_path) not in result.files_created
+    ), str(result)
@jaraco

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

I've added the test to the PR. The main reason I didn't add the test is that it's much more difficult to test the reported failure, which is that prefix is configured globally in distutils.cfg, and I found that simply creating a virtualenv works around the issue, so it may prove very difficult to create a suitable environment for replicating that failure mode.

But if this test does at least capture one failure mode, that's much better than no test. Thanks!

@jaraco jaraco force-pushed the jaraco:bugfix/4106-distutils-option-error-target-prefix-conflict branch to 3f01334 Nov 12, 2018

@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Damn it! The test is failing on Windows, should be using this instead for the distutils config location:

    Path(os.path.expanduser('~'),
         'pydistutils.cfg' if sys.platform == 'win32' else '.pydistutils.cfg'
        ).write(textwrap.dedent(
        '''
        [install]
        prefix=%s
        ''' % str(prefix)))
@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Which of course the linter is going to complain about... Stupid mindless PEP 8 enforcement.

def test_target_install_ignores_distutils_config_install_prefix(script):
    prefix = script.scratch_path / 'prefix'
    distutils_config = Path(os.path.expanduser('~'),
                            'pydistutils.cfg' if sys.platform == 'win32'
                            else '.pydistutils.cfg')
    distutils_config.write(textwrap.dedent(
        '''
        [install]
        prefix=%s
        ''' % str(prefix)))
    target = script.scratch_path / 'target'
    result = script.pip_install_local('simplewheel', '-t', target)
    assert (
        "Successfully installed simplewheel" in result.stdout and
        (target - script.base_path) in result.files_created and
        (prefix - script.base_path) not in result.files_created
    ), str(result)

@jaraco jaraco force-pushed the jaraco:bugfix/4106-distutils-option-error-target-prefix-conflict branch to da9caa4 Nov 12, 2018

@jaraco jaraco closed this Nov 12, 2018

@jaraco jaraco reopened this Nov 12, 2018

@rouge8

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I merged this with the latest master and tested it locally and it worked! 🎉 Is there any way to get this reviewed and accepted?

rouge8 added a commit to rouge8/dotfiles that referenced this pull request May 14, 2019

@jaraco jaraco requested a review from pradyunsg May 23, 2019

@pradyunsg pradyunsg merged commit 287aa4b into pypa:master May 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
news-file/pr News files updated and/or change is trivial.
Details
@pradyunsg

This comment has been minimized.

Copy link
Member

commented May 25, 2019

Thanks @benoit-pierre and @jaraco for figuring this out!

I'm not super familiar with these details though, so maybe it'd break something else somewhere in an esoteric manner. I don't think that's a likely outcome here though.

rouge8 added a commit to rouge8/dotfiles that referenced this pull request Jun 5, 2019
Use shiv to build shiv with fixed pip
Now that pypa/pip#6008 is fixed, shiv works with
a Homebrew Python without any dumb hacks with `~/.pydistutils.cfg`.

@lock lock bot added the S: auto-locked label Jun 24, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.