Get a file during bootstrap to a temp location first. #33288
Conversation
When downloading a file in the bootstrap phase - get it to a temp location first. Verify it there and only if downloaded properly move it to the `cache` directory. This should prevent `make` being stuck if the download was interrupted or otherwise corrupted. The temporary files are deleted in case of an exception.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| temp_path = temp_file.name | ||
| sha_file = tempfile.NamedTemporaryFile(suffix=".sha256", delete=True) | ||
| sha_path = sha_file.name | ||
| download(sha_path, sha_url, temp_path, url, verbose) |
There was a problem hiding this comment.
I think it might be worth reducing number of parameters here and invoking download twice instead, to show that we indeed download 2 files here ?
Make download() receive less parameters and use it explicitly 2 times in get().
cyplo
changed the title
| run(["curl", "-o", _path, _url], verbose=verbose) | ||
| print("verifying " + path) | ||
| with open(path, "rb") as f: | ||
| temp_file = tempfile.NamedTemporaryFile(delete=False) |
There was a problem hiding this comment.
Should be using with ... as temp_file:.
|
Very cool, thanks a lot @rkruppe, will look into fixing the things you've mentioned :) |
| run(["curl", "-o", path, url], verbose=verbose) | ||
|
|
||
|
|
||
| def verify(sha_path, temp_path, verbose): |
There was a problem hiding this comment.
The second parameter here isn't required to be a temporary file, so I think just path is a better name.
Personally, I think ordering the parameters as def verify(path, sha_path, verbose) would feel more natural, but that's not a big deal.
Remove the temp files if something goes wrong.
|
LGTM 👍 |
|
Cool, thanks ! @alexcrichton could you take a look ? |
Get a file during bootstrap to a temp location first. When downloading a file in the bootstrap phase - get it to a temp location first. Verify it there and only if downloaded properly move it to the `cache` directory. This should prevent `make` being stuck if the download was interrupted or otherwise corrupted, as per discussion in #32834 The temporary files are deleted in case of an exception. I was looking for some unit/integration tests around this and couldn't find any - presumably because this is being tested by just Travis launching it ? Let me know if it would be good to try to write tests around this. Thanks !
When downloading a file in the bootstrap phase - get it to a temp
location first. Verify it there and only if downloaded properly move it
to the
cachedirectory.This should prevent
makebeing stuck if the download was interruptedor otherwise corrupted, as per discussion in #32834
The temporary files are deleted in case of an exception.
I was looking for some unit/integration tests around this and couldn't find any - presumably because this is being tested by just Travis launching it ? Let me know if it would be good to try to write tests around this. Thanks !