Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplify setupext.download_or_cache.#15430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| with open(cache_dir / sha, "xb") as fout: | ||
| fout.write(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| withopen(cache_dir/sha,"xb")asfout: | |
| fout.write(data) | |
| (cache_dir/sha).write_bytes(data) |
would match reading, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but then this would not fail if a file already exists with the wrong hash (well, I dunno what we want to do in that case, which is mostly theoretical, but I guess "don't overwrite a file we didn't expect to be there" is a reasonable approach?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If there is an unexpected file in the cache, that could cause trouble later when reading from the cache. I think it's a valid position to claim that under this hash there should be exactly data. We can gracefully fix that. But both ways are ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At worst what will happen is that the "wrong" file will stay stuck in the cache (we didn't create it, so we can't afford to overwrite it) and later builds will redownload the file and fail to cache it but will otherwise proceed correctly. I think this is the same behavior as before this patch?
| with open(cache_dir / sha, "xb") as fout: | ||
| fout.write(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If there is an unexpected file in the cache, that could cause trouble later when reading from the cache. I think it's a valid position to claim that under this hash there should be exactly data. We can gracefully fix that. But both ways are ok.
Instead of repeatedly unwrapping and rewrapping the file contents in aBytesIO that needs to be seek()ed, just pass around the bytes objectitself.
@QuLogic do you want to re-review? :) |
Instead of repeatedly unwrapping and rewrapping the file contents in a
BytesIO that needs to be seek()ed, just pass around the bytes object
itself.
PR Summary
PR Checklist