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

Allow users to override default PyPI index URLs with PyPI mirror URLs #2281

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
4 participants
@JacobHenner
Contributor

JacobHenner commented May 30, 2018

In response to #2075, allow users to override the default PyPI indices (pypi.org, pypi.python.org) with a mirror specified via CLI parameter or environment variable.

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented May 30, 2018

WIP - Still need to write the test cases and update the docs, but figured I'd start the PR to encourage review.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 7475070 to eb55b19 May 30, 2018

nargs=1,
callback=validate_pypi_mirror,
help="Specify a PyPI mirror.",
)

This comment has been minimized.

@uranusjr

uranusjr May 31, 2018

Member

Is the option needed here? I’d be more inclined to put it only on install, lock, update, and sync. I don’t quite see the need of passing this for project initialisation.

This comment has been minimized.

@JacobHenner

JacobHenner Jun 6, 2018

Contributor

It looks like this one is indeed for the install operation.

@uranusjr

This comment has been minimized.

Member

uranusjr commented May 31, 2018

The implementation looks reasonable with a brief read (I didn’t actually run it, and yes tests are needed). I’m not sure if the flag is needed for bare pipenv calls.

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented May 31, 2018

I've been working on adding tests for install, but I'm running into an issue testing the --pypi-mirror parameter. It looks like the test suite overrides the default sources to use a locally-run web server, but I've written a test to ensure setting --pypi-mirror doesn't modify the Pipfile's sources. I haven't been able to narrow down where this is occurring, or how to prevent it, however, so the test fails when it reads a source url "http://localhost...".

Also, right now this just covers install. I hope to test and address all functionality currently dependent on the two primary PyPI urls before proceeding.

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 31, 2018

Thanks for the PR! The test fixtures and the project.py itself are both configured to use an enviroment variable to configure the test source to always use a local pypi cache for testing. Your test just needs to verify that the real url, when you place it in the sources somewhere, gets overridden during the install process. So you can write a Pipfile with an extra source to actual pypi and use p.pipenv('install --pypi-mirror') and use the verbose flag and just verify that the actual url is not present in the output or something, since it should include source urls

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented May 31, 2018

@techalchemy Thanks, I'll take a look at that.

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented May 31, 2018

@techalchemy If I go that route, where would you suggest I store that Pipfile?

For the local pypi cache, I can't seem to see where it's set. During each test run, I see the server uses 127.0.0.1 with a random port, but I'm still looking (without much luck) for where this is set, or where PIPENV_TEST_INDEX is set by the test suite.

@uranusjr

This comment has been minimized.

Member

uranusjr commented May 31, 2018

@JacobNenner PIPENV_TEST_INDEX is set in conftest.py (test configuration file used by Pytest), when a PipenvInstance is initiated. You can disable the PyPI mocking by passing pypi=False when you initiate the PipenvInstance.

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented May 31, 2018

@pytest.mark.install
@flaky
def test_mirror_install(PipenvInstance, pypi):
    with PipenvInstance(pypi=False) as p:
        # This should sufficiently demonstrate the mirror functionality
        # since pypi.org is the default.
        c = p.pipenv('install requests --pypi-mirror https://pypi.python.org/simple')
        assert c.return_code == 0
        # Ensure the --pypi-mirror parameter hasn't altered the Pipfile or Pipfile.lock sources
        assert len(p.pipfile['source']) == 1
        assert len(p.lockfile["_meta"]["sources"]) == 1
        assert 'https://pypi.org/simple' == p.pipfile['source'][0]['url']
        assert 'https://pypi.org/simple' == p.lockfile['_meta']['sources'][0]['url']
        
        assert 'requests' in p.pipfile['packages']
        assert 'requests' in p.lockfile['default']
        assert 'chardet' in p.lockfile['default']
        assert 'idna' in p.lockfile['default']
        assert 'urllib3' in p.lockfile['default']
        assert 'certifi' in p.lockfile['default']

still returns

test_mirror_install failed; it passed 0 out of the required 1 times.
	<type 'exceptions.AssertionError'>
	assert 'https://pypi.org/simple' == 'http://127.0.0.1:44423/simple'
  - https://pypi.org/simple
  + http://127.0.0.1:44423/simple
@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 3, 2018

Hey thanks for all your work on this front, so here's what you actually need to do to get your tests to work (almost there!)-- not sure you need to pass chdir=True here but it's possible:

@pytest.mark.install
@flaky
def test_mirror_install(PipenvInstance):
    with PipenvInstance(chdir=True) as p:
        with open(p.pipfile_path, 'w') as f:
            f.write("""
[[source]]
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"
            """.strip())
        # This should sufficiently demonstrate the mirror functionality
        # since pypi.org is the default.
        c = p.pipenv('install requests --pypi-mirror https://pypi.python.org/simple')
        assert c.return_code == 0
        # Ensure the --pypi-mirror parameter hasn't altered the Pipfile or Pipfile.lock sources
        assert len(p.pipfile['source']) == 1
        assert len(p.lockfile["_meta"]["sources"]) == 1
        assert 'https://pypi.org/simple' == p.pipfile['source'][0]['url']
        assert 'https://pypi.org/simple' == p.lockfile['_meta']['sources'][0]['url']

        assert 'requests' in p.pipfile['packages']
        assert 'requests' in p.lockfile['default']
        assert 'chardet' in p.lockfile['default']
        assert 'idna' in p.lockfile['default']
        assert 'urllib3' in p.lockfile['default']
        assert 'certifi' in p.lockfile['default']

And FYI, I grabbed a copy of your branch and ran this test on it and it passes, so nice work! :)

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch 2 times, most recently from 7b63a5e to 299baa7 Jun 4, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 4, 2018

Thanks @techalchemy. I've gone ahead and added that.

I'm now going to work on the other functions which depend on connectivity to PyPI, such as updates.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from e92f856 to f3d00fb Jun 5, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 5, 2018

This is almost finished, I have two things left to address:

  1. There's currently an issue with setting an environment variable on Windows running Python 2, according to appveyor.
  2. I need to write additional integration tests for update, sync, lock, and uninstall. The challenge will be figuring out which mirror can be used for testing, and how to ensure that mirror is being used (and ensure that PyPI isn't being used silently in the background).

Hopefully I'll have time to work on this tomorrow. If not, I'll try to get to it by Wednesday.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 606d6b0 to 787a466 Jun 5, 2018

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jun 5, 2018

Maybe you can use the actual PyPI, and override it with the local (mocked) PyPI as mirror? I guess you could peek into the logs or peek into it directly to see if it gets used as expected.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 787a466 to 9154f88 Jun 5, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 5, 2018

Looks like all that's left are the additional tests. Thanks for the suggestions @uranusjr and @techalchemy, I'll investigate them soon (hopefully tomorrow).

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 5, 2018

That's roughly what i've been attempting to suggest on IRC, but it might not be clear :p

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 5, 2018

Looks like that worked.

def test_mirror_install(PipenvInstance, pypi):
    with temp_environ(), PipenvInstance(chdir=True) as p:
        mirror_url = os.environ['PIPENV_TEST_INDEX']
        del os.environ['PIPENV_TEST_INDEX']
        # This should sufficiently demonstrate the mirror functionality
        # since pypi.org is the default when PIPENV_TEST_INDEX is unset.
        c = p.pipenv('install requests --pypi-mirror {0}'.format(mirror_url))
        assert c.return_code == 0
        # Ensure the --pypi-mirror parameter hasn't altered the Pipfile or Pipfile.lock sources
        assert len(p.pipfile['source']) == 1
        assert len(p.lockfile["_meta"]["sources"]) == 1
        assert 'https://pypi.org/simple' == p.pipfile['source'][0]['url']
        assert 'https://pypi.org/simple' == p.lockfile['_meta']['sources'][0]['url']

        assert 'requests' in p.pipfile['packages']
        assert 'requests' in p.lockfile['default']
        assert 'chardet' in p.lockfile['default']
        assert 'idna' in p.lockfile['default']
        assert 'urllib3' in p.lockfile['default']
        assert 'certifi' in p.lockfile['default']

I'll go ahead and use this pattern for the rest of my tests. So far, I intend to add tests for:

  1. update
  2. uninstall
  3. lock
  4. sync

This should cover all of the functions that now have the extra pypi_mirror parameter. Not sure if I'll get to this today. If not, I'll have a look tomorrow.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 9154f88 to 3eabbbe Jun 6, 2018

@JacobHenner JacobHenner changed the title from WIP: #2075 Allow users to override default PyPI index URLs with PyPI mirror URLs to Allow users to override default PyPI index URLs with PyPI mirror URLs Jun 6, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 6, 2018

I've written tests that should cover installs, uninstalls, locks, and syncs using the --pypi-mirror parameter. Please let me know if you think there are additional tests I should add.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 3eabbbe to 69b3e43 Jun 6, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 6, 2018

@uranusjr @techalchemy I performed additional manual tests in a restrictive environment, with no access to PyPI. All tested operations worked as expected. Looks like this one is ready for review, unless there are additional integration tests you'd like me to include.

@@ -277,6 +277,7 @@ class PipCommand(basecommand.Command):
pypi = PyPIRepository(
pip_options=pip_options, use_json=True, session=session
)
print(pip_options)

This comment has been minimized.

@uranusjr

uranusjr Jun 7, 2018

Member

stray debug?

This comment has been minimized.

@JacobHenner

JacobHenner Jun 7, 2018

Contributor

Yep, missed in my pre-commit diff grep. Removed.

@@ -442,6 +445,7 @@ def resolve_deps(
collected_hashes = []
if any('python.org' in source['url'] or 'pypi.org' in source['url']
for source in sources):
#FIXME Add support for mirrors.

This comment has been minimized.

@uranusjr

uranusjr Jun 7, 2018

Member

Is this still relevant?

This comment has been minimized.

@JacobHenner

JacobHenner Jun 7, 2018

Contributor

Nope, this was a note-to-self that was missed in my pre-commit diff grep. Removed.

This comment has been minimized.

@JacobHenner

JacobHenner Jun 7, 2018

Contributor

That being said, it looks like something might have been missed with venv_resolve_deps, I'll take a look now.

This comment has been minimized.

@JacobHenner

JacobHenner Jun 7, 2018

Contributor

Fixed the missing bit, will push shortly.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch 2 times, most recently from ba6ad56 to 625d739 Jun 7, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 7, 2018

Removed some accidentally included debugging artifacts, added pypi_mirror support to venv_resolve_deps in get_vcs_deps. Should be good to continue review.

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 625d739 to 18f8a69 Jun 7, 2018

@brettdh

This comment has been minimized.

brettdh commented Jun 7, 2018

One note: starting from scratch, I had expected the first pipenv install --pypi-mirror <package> to set url = <mirror url> in the Pipfile's [[source]] section, but it didn't. Either I have to manually correct this (even though I've already typed the URL once), or someone else coming along and running pipenv install would then unintentionally install the packages from PyPI.

Is there a more ergonomic way to support the use case of initializing a project to use a mirror for all the packages? Or configuring an existing project to use a mirror from now on? I'd expect that this is more common than selectively installing some packages from PyPI and some from a mirror.

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jun 7, 2018

I think the behaviour is intended. The flag is used to override the index during lock and install, not replace it in the declaration. The “right” way to do what you want is to write the mirror into pypirc, or produce your own Pipfile by hand. This really goes back to #2188 IMO—we can’t have an ergonic (typo, ergonomic) way to do this unless we have an actual init command.

@brettdh

This comment has been minimized.

brettdh commented Jun 7, 2018

Ah! Yes, I absolutely agree - a hypothetical pipenv init would be the right place for this. Thanks.

@brettdh

This comment has been minimized.

brettdh commented Jun 7, 2018

Side note: I'm not sure how to do this with .pypirc (does pipenv use settings from there?), but editing the Pipfile by hand is fine for now.

@uranusjr

This comment has been minimized.

Member

uranusjr commented Jun 7, 2018

@brettdh Sorry, I meant pip.conf :p I believe (didn’t check) we do extract sources from it when initialising the project.

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 7, 2018

Add support for PyPI mirrors
Adds support for the --pypi-mirror command line parameter and the
PIPENV_PYPI_MIRROR environment variable for most pipenv operations.
This permits pipenv to function without pypi.org, which is necessary for
users:

    1. behind restrictive networks
    2. facing strict artifact sourcing policies
    3. experiencing poor performance connecting to pypi.org
    4. who've configured a local cache for performance reasons

When specified, the value of this parameter replaces all instances of
pypi.org and pypi.python.org within pipenv operations without modifying
or requring the modification of Pipfiles.

- Resolves #2075

@JacobHenner JacobHenner force-pushed the JacobHenner:add_pypi_mirrors branch from 18f8a69 to af91eb6 Jun 7, 2018

@JacobHenner

This comment has been minimized.

Contributor

JacobHenner commented Jun 7, 2018

Rebased and (seemingly) ready to go.

@@ -1131,7 +1138,7 @@ def do_lock(
lockfile['default'][dep['name']]['markers'] = dep['markers']
# Add refs for VCS installs.
# TODO: be smarter about this.
_vcs_deps, vcs_lockfiles = get_vcs_deps(project, pip_freeze, which=which, verbose=verbose, clear=clear, pre=pre, allow_global=system, dev=False)
_vcs_deps, vcs_lockfiles = get_vcs_deps(project, pip_freeze, which=which, verbose=verbose, clear=clear, pre=pre, allow_global=system, dev=False, pypi_mirror=pypi_mirror)

This comment has been minimized.

@techalchemy

techalchemy Jun 8, 2018

Member

FYI I did realize how ugly and horrible this is, there is a minor refactor PR over in #2315 that will change this a bit

@techalchemy

This comment has been minimized.

Member

techalchemy commented Jun 8, 2018

Merging, thanks for your hard work 🍰

@techalchemy techalchemy merged commit 0fa7bd8 into pypa:master Jun 8, 2018

2 checks passed

buildkite/pipenv Build #301 passed (10 minutes, 26 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@techalchemy techalchemy moved this from Done to Needs Changelog in 2018.06.x Release Jun 16, 2018

@techalchemy techalchemy moved this from Needs Changelog to Done in 2018.06.x Release Jun 17, 2018

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