Removing .partial that causes repeated failure to download a component #1889
Conversation
|
@Glamhoth Are you in a position to respond to Robert's comment? |
|
Sorry for the delay. I pushed a commit that uses existing |
There was a problem hiding this comment.
@rbtcollins Could you comment on what further change it was you referred to in your previous comment? I think something about handling partials elsewhere?
Otherwise, from my perspective, this change looks useful, though I'd like to see a test to demonstrate the codepaths firing.
|
In that case there is only a test left to be written, but for now I have absolutely no idea how to prepare damaged partial file for it. Quote from @kinnison (so it won't be lost):
|
|
So while I don't have the details to hand... The test case will have created a set of channel data somewhere because that's used to provide a dist server for the test to update from. The trick will be to find that, get the SHA of the channel manifest, and then use that to create a partial file for the channel manifest (since that'd be named after the SHA I think). I will see if I can catch you on Discord to discuss further. |
|
☔ The latest upstream changes (presumably #2077) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I raised a PR against @Glamhoth's branch that I believe adds a test case for the mentioned scenario. |
|
I merged @c3st7n PR and rebased my branch onto upstream master to resolve a conflict. |
There was a problem hiding this comment.
The addition of the test is excellent. I've put a couple of comments in-line. If you could please
address those, and then rebase your branch so it doesn't carry fixup commits, this looks very close to mergeable. Once you've done that I'll try and test against the various issues we have raised which might be mitigated by this, before I merge.
tests/dist.rs
Outdated
| "target hash", | ||
| &path.join("dist/2016-02-02/rustc-nightly-x86_64-apple-darwin.tar.gz.sha256"), | ||
| ) | ||
| .unwrap()[..64] |
There was a problem hiding this comment.
This is unusual, I think I'd prefer:
| .unwrap()[..64] | |
| .unwrap().trim() |
Assuming I've correctly understood the purpose of that [..64]
There was a problem hiding this comment.
Turns out the hash file has more just the sha in it:
66d22b7dea5bf8af3295ef13a2ac71abe85e9ad2616eeb22623ca85e6f613ccf *rustc-nightly-x86_64-apple-darwin.tar.gz
We could split the string on whitespace but at the same time we know it is a fixed 64 character string so I'm tempted to leave it as is?
Also, in the actual code where it reads a hash file (src/dist/download.rs - fn download_hash) it just uses a slice too.
There was a problem hiding this comment.
Raised MR to glamhoth (https://github.com/Glamhoth/rustup.rs/pull/3) to use a contstant for the slice.
tests/dist.rs
Outdated
|
|
||
| match *err.kind() { | ||
| ErrorKind::ComponentDownloadFailed(_) => (), | ||
| _ => panic!(), |
There was a problem hiding this comment.
This could at least be:
| _ => panic!(), | |
| k @ _ => panic!("Unexpected error: {}", k), |
There was a problem hiding this comment.
| _ => panic!(), | |
| e => panic!("{:?}", e), |
There was a problem hiding this comment.
This was based on how it is done elsewhere in the same file, happy to change but wonder what you think about keeping it consistent with existing tests. Same goes for the other comment regarding use of [..64] instead of .trim().
There was a problem hiding this comment.
I think I'd rather see newer tests looking nicer than the older ones. A separate change to make the tests more obvious throughout could be done, though it doesn't belong here.
|
Also, welcome back @Glamhoth :-D |
|
Merged https://github.com/Glamhoth/rustup.rs/pull/3 and rebased fixup commits |
|
Because the server is full of memory, I solve this problem by cleaning up some temporary files to free up some memory. |
#1854