Consistently catch exceptions in index.py

[cboylan]

This resolves issue #4195.

In index.py's index retrieval routine we were catching
requests.HTTPError to log and ignore 404s and other similar HTTP server
errors when pulling from (extra-)index-urls. Unfortunately, the actual
path to that exception is requests.exceptions.HTTPError and the alias we
were using does not work when pip is installed with unvendored libs as
with the debian packaged pip.

Thankfully the fix is simple. Import and use
requests.exceptions.HTTPError. This comes with the added bonus of
fitting in with the existing handling for RetryError and SSLError. With
this change in place upstream pip and downstream packaged pip should
both catch this exception properly.

Note: I've not added any tests cases as I'm unsure how to test the
distro packaging case within pip's testsuite. However, the existing test
suite should hopefully cover that this isn't a regression and I've
manually confirmed that this works with a hacked up debian package
install. Also this is how we handle RetryError and SSLError.

[cboylan]

Rebased to rerun tests with the rerunfailures plugin fix.

[cjerdonek]

In index.py's index retrieval routine we were catching requests.HTTPError to log and ignore 404s

Are you saying this is something Debian is unable to fix on their end? I'd just like to understand why it's better to fix here. Also, if things change in pip again, it might break Debian again. So it doesn't seem like it's a real fix.

[yan12125]

the alias we were using does not work when pip is installed with unvendored libs

That's because a missing entry in _vendor/__init__.py. I have a proof-of-concept patch at https://github.com/yan12125/pip/commit/7a29806a6b8eca0a1ec4e265d7b20610d5609742. Note that this patch works only if 8ef3283 is applied first.

I tested my patch on Arch Linux.

[cboylan]

In index.py's index retrieval routine we were catching requests.HTTPError to log and ignore 404s

Are you saying this is something Debian is unable to fix on their end? I'd just like to understand why it's better to fix here. Also, if things change in pip again, it might break Debian again. So it doesn't seem like it's a real fix.

To be clear the patch that Debian carries is simple and basically all it does is set the DEBUNDLED flag in pip. This DEBUNDLED flag then conditionalled called the vendored() function on vendored paths to have them get loaded out of the distro packaging instead. Whatever the fix is for this is likely to be in the pip code base as a result.

As for the problem in this specific issue (#4195) this change fixes that and catches the HTTPError exception when DEBUNDLED is set. I didn't think this was unreasonable considering that this is how the requests defines, throws, and tests HTTPError, but they do document the exception as requests.HTTPError.

All that said, as mentioned the underlying issue has to do with requests.HTTPError != requests.exceptions.HTTPError when debundled. Sorting this out is a bit more involved and likely has a much larger surface area to consider as it will involve changes to the vendored() utility function (which @yan12125 seems to have gotten in 8ef3283) and how we call the vendored() utility.

Even then these are fixes to pip's tree and not distro packaging. It would be good if we can reopen #4195 at the very least and work to fix it in pip.

[cboylan]

the alias we were using does not work when pip is installed with unvendored libs

That's because a missing entry in _vendor/__init__.py. I have a proof-of-concept patch at yan12125@7a29806. Note that this patch works only if 8ef3283 is applied first.

I tested my patch on Arch Linux.

Are there reasons for not making yan12125@7a29806 a PR against pip if it fixes the larger problem? I imagine that there may be concerns that this might have broader fallout? Say in the case where a DEBUNDLED package doesn't provide all the necessary vendored bits themselves?

[cjerdonek]

@cboylan @yan12125 Is @cboylan’s fix still needed with @yan12125’s suggested change? Also, assuming the answer is no, would @cboylan’s change hurt anything if @yan12125’s change is first applied?

[cjerdonek]

PS - thanks for the explanation of what Debian does. My only concern is fixing this in a way that is less likely to get accidentally “unfixed” by random changes later on down the road (so ultimately trying to help you). @yan12125’s fix does that IMO, assuming it’s enough.

[cboylan]

@cboylan @yan12125 Is @cboylan’s fix still needed with @yan12125’s suggested change? Also, assuming the answer is no, would @cboylan’s change hurt anything if @yan12125’s change is first applied?

If I understand correctly this PR is not needed if we get @yan12125's suggested change in. However, because that change fixes the relationship between requests.HTTPError and requests.exceptions.HTTPError so that they are the same object this PR should be safe to merge as well should that be desirable (perhaps the other change needs more testing, not sure why it hasn't been proposed as a PR yet, or maybe it has?)

[cjerdonek]

Okay, great. Can either you or @yan12125 propose that as a separate PR that fixes #4195? After merging that, this can be reclassified as a cosmetic, style change.

[yan12125]

Are there reasons for not making yan12125@7a29806 a PR against pip if it fixes the larger problem?

When I was working on the prerequisite patch 8ef3283 (#6113), there are no Debian developers reporting testing results with my patch. As a result I hesitated to propose more patches depending on the first one. Now that the situation has changed, so I've opened #6373. @cboylan As you're working with the same topic here, could you

• Check if pip after 8ef3283 still works fine with Debian's debundling approach?
• Check if 8ef3283 + Fix handling of requests exceptions when dependencies are debundled #6373 fixes Changed behavior with --extra-index-url with pip 9  #4195 on Debian?

[thomasgoirand]

Hi,

I'm the person who attempted to fix things with the python-pip package in both Stretch and Buster/Sid.

I have tested these:
8ef3283 and https://github.com/yan12125/pip/commit/7a29806a6b8eca0a1ec4e265d7b20610d5609742

on top of the Debian pip package, and I can confirm that this fixes the issue of --extra-index-url (ie: no 404 anymore).

Please do go ahead and approve the patches.

[cjerdonek]

Okay, thanks, all. @yan12125's PR #6373 is now merged.

@cboylan, could you update this PR to be a style-only change -- for the purposes of making the exception handling have a consistent form? (No news entry would be needed.)

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[cboylan]

Okay, thanks, all. @yan12125's PR #6373 is now merged.

@cboylan, could you update this PR to be a style-only change -- for the purposes of making the exception handling have a consistent form? (No news entry would be needed.)

Done, though I added a trivial news entry as documented in the contributing docs. Let me know if that should be removed.

[cjerdonek]

Thanks, @cboylan!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.