test: switch from curl to urllib for HTTP requests #29970
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
test/get_previous_releases.py
Outdated
| response = requests.get(tarballUrl) | ||
| with open(tarball, 'wb') as file: | ||
| file.write(response.content) |
There was a problem hiding this comment.
This is not a refactor. It is a behavior change when an error occurs, for example a network error, or a file error
There was a problem hiding this comment.
If you're going to read the entire data into memory, you might as well do the hashing at the same time? Currently, it reads back the same tarball it wrote and passes it to the hasher, which seems kind of a waste.
There was a problem hiding this comment.
You're right about behavior change. I am still not sure about using script or util tag.
There was a problem hiding this comment.
It's important to be careful about behavior changes but i do think this is an improvement, using requests.head is better practice than calling curl then looking for a string in the output.
|
This is a script used for the tests, so adding |
iw4p
changed the title
|
The title's tag also changed from |
iw4p
force-pushed
the
feature/requests-library-usage
branch
from
38176a7
to
ddd27cb
Compare
|
@laanwj I investigated and found that you modified edit: It seems bitcoin/ci/test/00_setup_env_native_previous_releases.sh is responsible to provides everything that the environment's needed. Shall I add |
|
Yes, that's usually how it'd be done. But gah, |
iw4p
force-pushed
the
feature/requests-library-usage
branch
from
ddd27cb
to
29e9377
Compare
iw4p
changed the title
iw4p
force-pushed
the
feature/requests-library-usage
branch
from
29e9377
to
7fe94f7
Compare
|
I replaced the |
| ['curl', '--remote-name', tarballUrl] | ||
| ] | ||
| try: | ||
| response = urlopen(Request(tarballUrl, method='HEAD')) |
There was a problem hiding this comment.
Is it required to check for 404 in a separate, additional request?
There was a problem hiding this comment.
That's how the current code works too - it first does a HEAD request to check that the file is there, then goes to downloading. i don't know the original reasoning behind this, though.
There was a problem hiding this comment.
No, I could write more pythonically and with one request, but the reason I wrote it this way was to commit to the previous code. If you agree with one request, I can change it.
Should this change be in a new commit or should I force it on the previous commit?
There was a problem hiding this comment.
Should this change be in a new commit or should I force it on the previous commit?
Squashing seems fine?
There was a problem hiding this comment.
To rewrite the git history for having the last change on the last commit and ignore old ones, I forced it. I used squashing only for merging before that.
| if ret: | ||
| return ret | ||
| try: | ||
| response = urlopen(tarballUrl) |
There was a problem hiding this comment.
AFAIK there's a minor behaviour change here, where urlopen will follow redirects whereas curl won't usually
$ curl -I https://httpbin.org/absolute-redirect/3
HTTP/2 302
# ...>>> from urllib.request import urlopen
>>> response = urlopen("https://httpbin.org/absolute-redirect/3")
>>> response.code
200 # Not 302 because redirects were followedThis should be fine, but worth a mention.
| @@ -5,7 +5,7 @@ | |||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | |||
| # | |||
| # Download or build previous releases. | |||
| # Needs curl and tar to download a release, or the build dependencies when | |||
| # Needs urllib built-in python library and tar to download a release, or the build dependencies when | |||
There was a problem hiding this comment.
If this is part of the standard library, I don't think you need to list it as a requirement.
Switched from using subprocess to make HTTP requests (curl) to using the Python requests library, improving cleanliness and maintainability.