url_exists related improvements#34095
Merged
tgamblin merged 4 commits intospack:developfrom Nov 23, 2022
Merged
Conversation
76aaa85 to
a1a2f98
Compare
For reasons beyond me Python thinks it's a great idea to upgrade HEAD requests to GET requests when following redirects. So, this PR adds a better `HTTPRedirectHandler`, and also moves some ad-hoc logic around for dealing with disable ssl cert verification. Also, I'm stumped by the fact that Spack's `url_exists` does not use HEAD requests at all, so in certain cases Spack awkwardly downloads something first to see if it can download it, and then downloads it because it know it can download it. So, this PR ensures that both urllib and botocore use HEAD requests.
a1a2f98 to
5e637d0
Compare
Member
Author
|
@spackbot fix style |
|
Let me see if I can fix that for you! |
|
I was able to run spack style --fix==> Running style checks on spack
selected: isort, mypy, black, flake8
==> Modified files
lib/spack/spack/test/web.py
lib/spack/spack/util/web.py
==> Running isort checks
Fixing /tmp/tmp3yze3ko_/spack/lib/spack/spack/util/web.py
isort checks were clean
==> Running mypy checks
lib/spack/llnl/util/lang.py:1064: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
lib/spack/llnl/util/lock.py:84: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
lib/spack/spack/util/timer.py:68: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
lib/spack/spack/bootstrap.py:87: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
Success: no issues found in 565 source files
mypy checks were clean
==> Running black checks
All done! ✨ 🍰 ✨
2 files left unchanged.
black checks were clean
==> Running flake8 checks
flake8 checks were clean
==> spack style checks were clean
I've updated the branch with style fixes. |
tgamblin
approved these changes
Nov 23, 2022
haampie
pushed a commit
that referenced
this pull request
Nov 26, 2022
This reverts commit d06fd26. The problem is that Bitbucket's API forwards download requests to an S3 bucket using a temporary URL. This URL includes a signature for the request, which embeds the HTTP verb. That means only GET requests are allowed, and HEAD requests would fail verification, leading to 403 erros. The same is observed when using `curl -LI ...`
amd-toolchain-support
pushed a commit
to amd-toolchain-support/spack
that referenced
this pull request
Feb 16, 2023
For reasons beyond me Python thinks it's a great idea to upgrade HEAD requests to GET requests when following redirects. So, this PR adds a better `HTTPRedirectHandler`, and also moves some ad-hoc logic around for dealing with disabling SSL certs verification. Also, I'm stumped by the fact that Spack's `url_exists` does not use HEAD requests at all, so in certain cases Spack awkwardly downloads something first to see if it can download it, and then downloads it again because it knows it can download it. So, this PR ensures that both urllib and botocore use HEAD requests. Finally, it also removes some things that were there to support currently unsupported Python versions. Notice that the HTTP spec [section 10.3.2](https://datatracker.ietf.org/doc/html/rfc2616.html#section-10.3.2) just talks about how to deal with POST request on redirect (whether to follow or not): > If the 301 status code is received in response to a request other > than GET or HEAD, the user agent MUST NOT automatically redirect the > request unless it can be confirmed by the user, since this might > change the conditions under which the request was issued. > Note: When automatically redirecting a POST request after > receiving a 301 status code, some existing HTTP/1.0 user agents > will erroneously change it into a GET request. Python has a comment about this, they choose to go with the "erroneous change". But they then mess up the HEAD request while following the redirect, probably because they were too busy discussing how to deal with POST. See python/cpython#99731
amd-toolchain-support
pushed a commit
to amd-toolchain-support/spack
that referenced
this pull request
Feb 16, 2023
This reverts commit d06fd26. The problem is that Bitbucket's API forwards download requests to an S3 bucket using a temporary URL. This URL includes a signature for the request, which embeds the HTTP verb. That means only GET requests are allowed, and HEAD requests would fail verification, leading to 403 erros. The same is observed when using `curl -LI ...`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For reasons beyond me Python thinks it's a great idea to upgrade HEAD
requests to GET requests when following redirects. So, this PR adds a
better
HTTPRedirectHandler, and also moves some ad-hoc logic aroundfor dealing with disabling SSL certs verification.
Also, I'm stumped by the fact that Spack's
url_existsdoes not useHEAD requests at all, so in certain cases Spack awkwardly downloads
something first to see if it can download it, and then downloads it
again because it knows it can download it. So, this PR ensures that both
urllib and botocore use HEAD requests.
Finally, it also removes some things that were there to support currently
unsupported Python versions.
Notice that the HTTP spec section 10.3.2 just talks about how to deal
with POST request on redirect (whether to follow or not):
Python has a comment about this, they choose to go with the "erroneous change".
But they then mess up the HEAD request while following the redirect, probably
because they were too busy discussing how to deal with POST.
See python/cpython#99731