Add `--extra-index-url` from all extra indexes #1980

techalchemy · 2018-04-13T05:18:44Z

Handle indexes, extra indexes, uncached sources

Handle extra-index-urls when resolving
Handle extra-index-url when using --skip-lock
Parse index arguments when installing individual packages
Translate index aliases to urls
Always include extra indexes when installing a packages
get_source() falls back to parsed_pipfile['source'] for sources when
not present in the lockfile (SourceNotFound for internal source on 11.10.0 #1994)
Include index and extra-index-url arguments in pipenv lock -r output
Fixes pipenv install --skip-lock broken with multiple sources #1973, pipenv install --skip-lock produces error and fails to install dependencies "You likely have a mismatch in your sub-dependencies" #1974, Feature request: pipenv install --index myindex mypackage #1852, pipenv sync unexpected behavior when Pipfile missing #1977, SourceNotFound for internal source on 11.10.0 #1994

Signed-off-by: Dan Ryan dan@danryan.co

techalchemy · 2018-04-13T14:29:16Z

@uranusjr I want to favor this over #1950 but the get_source method in pipenv.Project only checks the cached lockfile, which doesn't work if you add a new source to the pipfile that hasn't been locked yet (i.e. you use --skip-lock) -- thoughts?

techalchemy · 2018-04-17T00:31:04Z

@uranusjr @jtratner @ncoghlan @frostming would appreciate if at least one of you can look this over before it gets merged so I can go ahead and cut a bugfix release

- Always add extra indexes when installing - Look up indexes by key if key is given instead of url - Fixes #1973, #1974, #1852 Signed-off-by: Dan Ryan <dan@danryan.co>

Signed-off-by: Dan Ryan <dan@danryan.co>

- Handle extra-index-urls when resolving - Handle extra-index-url when using `--skip-lock` - Parse index arguments when installing individual packages - Translate index aliases to urls - Always include extra indexes when installing a packages - `get_source()` falls back to `parsed_pipfile['source']` for sources when not present in the lockfile (#1994) - Include index and extra-index-url arguments in `pipenv lock -r` output - Fixes #1973, #1974, #1852, #1977, #1994 Signed-off-by: Dan Ryan <dan@danryan.co>

Signed-off-by: Dan Ryan <dan@danryan.co>

uranusjr

Made some comments. The most problematic to me is test_install_named_index_alias. My other comments are mostly petty.

uranusjr · 2018-04-17T03:00:40Z

pipenv/project.py

            elif url:
-                if source.get('url') in url:
-                    return source
+                source = [s for s in sources if s.get('url') in url]


I know this was the case before, but is it a good idea to use in here, or should we use startswith instead?

probably, it seemed odd when I was reimplementing it but I kind of copied what was there... I can't really imagine why we'd use in vs startswith

git blame shows @kennethreitz and it dates all the way back to the original implementation (v2.8.7!). Maybe no particular reason, just to hammer out something working first.

uranusjr · 2018-04-17T03:02:27Z

pipenv/project.py

-                    return source
+                source = [s for s in sources if s.get('url') in url]
+            if source:
+                return source[0]


Isn’t this essentially what first does? I don’t dislike this (I never liked the first library, in fact), but maybe we should be consistent here.

if it's imported I'll swap it over, in the spirit of speeding up startup time (which primarily was coming from indirect loading of the project module) I was trying to avoid extra imports

uranusjr · 2018-04-17T03:31:38Z

tests/integration/test_install_uri.py

+            """.format(os.environ.get('PIPENV_TEST_INDEX')).strip()
+            f.write(contents)
+        c = p.pipenv('install pytz --index pypi')
+        assert c.return_code == 0


A few problems:

Comment seems wrong. A lot of tests below have the wrong comment, it seems? None of them are using a Git repo afaict.

This is not future-proof. If we add pytz to the mock PyPI (we might) this cannot detect an incorrect --index implementation.

Probably don’t want to use dict.get. If PIPENV_TEST_INDEX is not set this needs to fail loudly.

oh yeah I think I've been copy pasting a bunch, I can do some comment cleanup

I agree about the package, we can use something that's only available on the test index like the one I am using in the other test

uranusjr · 2018-04-17T03:40:04Z

Forgot to mention in the review—split_index and split_extra_index seem a bit too similar to me. Is it possible to refactor the logic out?

Signed-off-by: Dan Ryan <dan@danryan.co>

jtratner · 2018-04-17T04:06:59Z

pipenv/core.py

@@ -1367,6 +1367,7 @@ def pip_install(
    pre=False,
    selective_upgrade=False,
    requirements_dir=None,
+    extra_indexes=None,


wdyt about, rather than making this type polymorphic, enforcing that extra_indexes must always be a list and when calling this function on line 841, ensuring that it's always a list (or having split_extra_index always return a list of elements)

I think that makes sense but I want to stay out of the weeds on that logic until I get this bugfix released, so lets plan for 11.11.0

jtratner · 2018-04-17T04:10:42Z

pipenv/core.py

+                extra_indexes = [extra_indexes,]
+            for idx in extra_indexes:
+                try:
+                    extra_src = project.find_source(idx).get('url')


what exactly is going on here? how would you hit this case?

If you have multiple indexes in your pipfile but you don't specify anything special when you run pipenv install, this makes sure we use all of the indexes you specify as extra index urls

jtratner · 2018-04-17T04:18:24Z

tests/integration/test_project.py

+            source = [s for s in project.pipfile_sources if s.get('name') == name]
+            assert source[0]['name'] == name
+            assert source[0]['url'] == url
+            source = project.get_source(name=name)


why can't you just do one big equality check here?

source = [s for s in project.pipfile_sources if s.get('name') == name] assert source source = source[0] assert source['name'] == name assert source['url'] == url assert source == project.get_source(name=name) assert source == project.get_source(url=url) assert source == project.find_source(name) assert source == project.find_source(url)

additionally where are there get and find methods that do the same thing?

so the pipfile_sources property hits the pipfile directly, exclusively. The get_source method (which I didn't write and wasn't sure about) appears to consult the lockfile, and when I happened upon it it only consulted the lockfile. find_source is a helper method to deal with #1852 so it simply saves reimplementing the same calls/checks a bunch in order to figure out if we have a named source

However yes this can probably be done the way you described

turns out equality checks don't work but I did fix this

jtratner · 2018-04-17T04:21:35Z

tests/integration/test_project.py

+
+@pytest.mark.project
+@pytest.mark.sources
+def test_get_cached_source(PipenvInstance, pypi):


what do you think about combining these two tests) which look nearly the same except for lock step into one test that looks like:

@pytest.mark.parameterized('lock_first', [True, False]) def test_get_source(PipenvInstnace, pypi, lock_first): with PipenvInstance(...) with open(p.pi...): ... if lock_first: # force source to be cached c = p.pipenv('lock') assert c.return_code == 0

that way it's clearer how these two checks differ

jtratner · 2018-04-17T04:25:03Z

tests/integration/test_lock.py

+    # dependency anyway.
+    with PipenvInstance() as p:
+        with open(p.pipfile_path, 'w') as f:
+            contents = """


since these two are the same, how do you feel about using a constant and/or a fixture:

@pytest.mark.install # private indexes need to be uncached for resolution @pytest.fixture def PrivateIndexPipenvInstance(PipenvInstance): # This uses the real PyPI since we need Internet to access the Git # dependency anyway. with PipenvInstance() as p: with open(p.pipfile_path, 'w') as f: f.write(contents) yield p

that way you don't have to repeat the @pytest.mark.install as much and you can slim down the test to make it easier to read.

def test_private_index_skip_lock(PrivateIndexPipenvInstance): p = PrivateIndexPipenvInstance c = p.pipenv('install --skip-lock') assert p.pipenv('install --skip-lock')

or if you want to keep the convention that you call PipenvInstances in a with statement, can do:

def PrivatePipenvIndexInstance(PipenvInstance): @contextlib.contextmanager def inner(): with PipenvInstance() as p: # write stuff yield p return inner

I'm not that great with pytest so I don't want to hold the release up while I tinker with this but I hear the comments and agree broadly speaking that we can afford to clean this up some more

- Interpolate environment vars into pipfile sources - Use first to more efficiently handle list comprehensions Signed-off-by: Dan Ryan <dan@danryan.co>

Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy · 2018-04-17T05:36:11Z

Ok mostly changed stuff, didn't rewrite all of the testing w/ the fixtures and refactor points per @jtratner but did refactor them a bit to be more concise & got them passing again

Signed-off-by: Dan Ryan <dan@danryan.co>

jakul mentioned this pull request Apr 13, 2018

"pipenv lock -r " doesn't output extra index information #1987

Closed

techalchemy force-pushed the 1973-extra-index-urls branch 2 times, most recently from 4714b50 to 53fafef Compare April 13, 2018 14:27

techalchemy force-pushed the 1973-extra-index-urls branch 5 times, most recently from 205edcf to 7cc1d08 Compare April 14, 2018 00:47

techalchemy added the Category: Private PyPIs 😎 Problem relates to private PyPI usage. label Apr 14, 2018

techalchemy added this to the 11.10.1 milestone Apr 14, 2018

techalchemy force-pushed the 1973-extra-index-urls branch 4 times, most recently from b3512ed to 2315eab Compare April 17, 2018 00:27

techalchemy and others added 12 commits April 16, 2018 23:14

Add --extra-index-url from all extra indexes

e054d77

- Always add extra indexes when installing - Look up indexes by key if key is given instead of url - Fixes #1973, #1974, #1852 Signed-off-by: Dan Ryan <dan@danryan.co>

Check for sources in pipfile before validating

2000f47

Signed-off-by: Dan Ryan <dan@danryan.co>

add PIPENV_INSTALL_TIMEOUT to envar index

8401acc

Signed-off-by: Dan Ryan <dan@danryan.co>

add more envars to docs

fa299db

add section explaining PIPENV_VENV_IN_PROJECT

f91facd

clarify wording on some envars

1a64a8d

Add encoding to jenkins test runner

e8efe3f

Signed-off-by: Dan Ryan <dan@danryan.co>

Add tests for project source changes

f93b9f2

Signed-off-by: Dan Ryan <dan@danryan.co>

Add test coverage for all fixes

c78628b

Signed-off-by: Dan Ryan <dan@danryan.co>

Update test pypi index

8d20caf

Signed-off-by: Dan Ryan <dan@danryan.co>

Run private indexes in uncached tests

81a2367

Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy force-pushed the 1973-extra-index-urls branch from 2e403e3 to 81a2367 Compare April 17, 2018 03:15

uranusjr requested changes Apr 17, 2018

View reviewed changes

Update tests

b89a73c

Signed-off-by: Dan Ryan <dan@danryan.co>

jtratner reviewed Apr 17, 2018

View reviewed changes

techalchemy force-pushed the 1973-extra-index-urls branch from f747090 to fe3a3fe Compare April 17, 2018 04:27

Refactor split_index and split_extra_index

c2462e3

- Interpolate environment vars into pipfile sources - Use first to more efficiently handle list comprehensions Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy force-pushed the 1973-extra-index-urls branch from fe3a3fe to c2462e3 Compare April 17, 2018 04:34

Fix tests and index url fetching code per requests

7a8d01e

Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy force-pushed the 1973-extra-index-urls branch from bb612ab to 45d6355 Compare April 17, 2018 05:33

Sort items before comparison

31a4a75

Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy force-pushed the 1973-extra-index-urls branch from 45d6355 to 31a4a75 Compare April 17, 2018 05:53

Use startswith for url comparison in project

88db0fe

Signed-off-by: Dan Ryan <dan@danryan.co>

techalchemy merged commit 3c675bd into master Apr 17, 2018

techalchemy deleted the 1973-extra-index-urls branch April 20, 2018 03:42

techalchemy mentioned this pull request Apr 20, 2018

Running with --skip-lock fails if an extra index is defined in the Pipfile #2029

Closed

uranusjr mentioned this pull request Apr 25, 2018

pipenv install is missing an equivalent to pip's --index-url= #2051

Closed

This was referenced Apr 25, 2018

generate pip {dev-}requirements.txt from Pipfile.lock #89

Closed

extra index url not stripped out of requirements explodes #2070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--extra-index-url` from all extra indexes #1980

Add `--extra-index-url` from all extra indexes #1980

techalchemy commented Apr 13, 2018 •

edited

Loading

techalchemy commented Apr 13, 2018

techalchemy commented Apr 17, 2018

uranusjr left a comment

uranusjr Apr 17, 2018

techalchemy Apr 17, 2018

uranusjr Apr 17, 2018

uranusjr Apr 17, 2018

techalchemy Apr 17, 2018

uranusjr Apr 17, 2018

techalchemy Apr 17, 2018

uranusjr commented Apr 17, 2018

jtratner Apr 17, 2018

techalchemy Apr 17, 2018

jtratner Apr 17, 2018

techalchemy Apr 17, 2018

jtratner Apr 17, 2018

techalchemy Apr 17, 2018

techalchemy Apr 17, 2018

techalchemy Apr 17, 2018

jtratner Apr 17, 2018

jtratner Apr 17, 2018

jtratner Apr 17, 2018

techalchemy Apr 17, 2018

techalchemy commented Apr 17, 2018

Add --extra-index-url from all extra indexes #1980

Add --extra-index-url from all extra indexes #1980

Conversation

techalchemy commented Apr 13, 2018 • edited Loading

techalchemy commented Apr 13, 2018

techalchemy commented Apr 17, 2018

uranusjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uranusjr commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techalchemy commented Apr 17, 2018

Add `--extra-index-url` from all extra indexes #1980

Add `--extra-index-url` from all extra indexes #1980

techalchemy commented Apr 13, 2018 •

edited

Loading