Skip to content

Avoid redundant downloads when bootstrapping #34644

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 6, 2016
Merged

Conversation

infinity0
Copy link
Contributor

If the local file is available, then verify it against the hash we just
downloaded, and if it matches then we don't need to download it again.

If the local file is available, then verify it against the hash we just
downloaded, and if it matches then we don't need to download it again.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

@alexcrichton
Copy link
Member

Thanks for the PR! This was actually a bad approximation to fix another issue which we may want to take into account as well. If you're bootstrapping Rust today you'll have the snapshot compiler downloaded, but then once it updates and you do a git pull this'll cause futures builds to fail, right?

Basically we just need to handle that case where if src/stage0.txt changed then it's ok to re-download.

@infinity0
Copy link
Contributor Author

I think my patch here doesn't break that - it still always downloads the hash, even if src/stage0.txt changes (and changes the hash url).

@alexcrichton
Copy link
Member

Oh! I failed to notice the fall through from a failed verification to just doing the download anyway, sorry about that! Then in that case this looks good to me, thanks!

Perhaps verify could return a boolean, however, instead of throwing an exception to indicate failure? That way it could avoid the try/catch and accidentally catching other exceptions.

@infinity0
Copy link
Contributor Author

I've changed verify as you suggested. It turned out well that you suggested this - I missed the fact that the previous version had a sys.exit so my fall-through wouldn't actually work properly. Now it does, though.

download(temp_path, url, verbose)
verify(temp_path, sha_path, verbose)
if not verify(temp_path, sha_path, verbose):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps this should pass verbose as True unconditionally to ensure we always print out the mismatching hashes?

Similarly, the verbose above seems like it could always be False as we don't want to print out mis-verifications there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, see the next commit.

@alexcrichton
Copy link
Member

@bors: r+ 933a103

@bors
Copy link
Collaborator

bors commented Jul 6, 2016

⌛ Testing commit 933a103 with merge a120ae7...

bors added a commit that referenced this pull request Jul 6, 2016
Avoid redundant downloads when bootstrapping

If the local file is available, then verify it against the hash we just
downloaded, and if it matches then we don't need to download it again.
@bors bors merged commit 933a103 into rust-lang:master Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants