install: Update extra-index-url in session from requirements file

[NoahGorny]

Resolves #8103
@pfmoore you are welcome to take a look 😄

Not sure how to test this though, maybe just check that session.auth.index_urls is updated as expected in a test?

[uranusjr]

Where were --extra-index-url from command line and the --index-url from requirements file populated into the session? It seems weird we need to update this separately.

[ivanst0]

URLs from both --index-url and --extra-index-url are treated equally and stored in session.auth.index_urls and finder.index_urls. They are not stored in session itself, just passed to auth (session.py:252). When the session is initialized (install.py:278) only options from command line are available. The options from the requirements file are populated into finder.index_urls only later by get_requirements() (install.py:300). At that point session.auth.index_urls can be updated to include URLs from the requirements file.

[uranusjr]

Thanks for the explanation! The patch makes sense to me now. It seems like the whole requirements file parsing logic needs some serious revamping 🙁 But this will have to do for now.

[NoahGorny]

@uranusjr @ivanst0 I am pinging this as this seems to be ready and can be merged (pip just released a version, so merging should be okay now)

[sbidoul]

Would it be better to update session.index_urls in req_file.handle_option_line()? It is the place where the finder search scope is updated with index urls found in requirement files, so it could make sense to put the update of the session at the same place so it will be easier to spot when refactoring the requirements parser.

[NoahGorny]

Would it be better to update session.index_urls in req_file.handle_option_line()? It is the place where the finder search scope is updated with index urls found in requirement files, so it could make sense to put the update of the session at the same place so it will be easier to spot when refactoring the requirements parser.

This is a very good idea, take a look now @sbidoul, moved to here and pushed a change

[sbidoul]

@NoahGorny thanks for the update. It would nice to add a test in test_req_file.py, for which you can find inspiration here, using the session fixture.

[NoahGorny]

@NoahGorny thanks for the update. It would nice to add a test in test_req_file.py, for which you can find inspiration here, using the session fixture.

Updated the relevant tests, take a look and thanks a lot for your feedback! 😄

[sbidoul]

Thanks!

[NoahGorny]

@pradyunsg thanks for the additional review 😄
I think the PR is ready now 😃

[ivanst0]

This PR has two approvals and all tests are passing, is there anything else blocking the merge?

Could you please link it to #8640?

[NoahGorny]

@pradyunsg @uranusjr could you give us a hand here?

[pradyunsg]

Merged, not because I reviewed this but since there's 2 other approvals already. Thanks for the PR @NoahGorny and for the nudge here @ivanst0! :)

[mitar]

Thanks for fixing #8103! Could I suggest that documentation is updated as well? Currently it says:

pip install keyring
echo your-password | keyring set pypi.company.com your-username
pip install your-package --extra-index-url https://pypi.company.com/

But with this fix it should be:

pip install keyring
echo your-password | keyring set https://pypi.company.com/ your-username
pip install your-package --extra-index-url https://pypi.company.com/