Skip to content
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

Add automatic resume flag when retrying download with curl #3089

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

darkkeh
Copy link
Contributor

@darkkeh darkkeh commented Oct 12, 2022

Should fix the resume issue that caused #2853 to be reopened.

Simply adds the resume flag -C and tells curl to automatically figure out where to resume the download with -.

Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!
How can I test it? Did you do some tests for this patch?

@darkkeh
Copy link
Contributor Author

darkkeh commented Oct 14, 2022

Sorry for being a bit slow to respond yesterday ended up being busy!

I did some manual testing by kill switching my network with my VPN (adding a video below to show the behavior).
unknown.webm

I could look into adding an actual test case for this but I would probably need some guidance as I'm fairly new to rust and I wouldn't exactly be sure how to go about testing this properly

rustup-init.sh Outdated
@@ -610,7 +610,8 @@ check_curl_for_retry_support() {
local _retry_supported=""
# "unspecified" is for arch, allows for possibility old OS using macports, homebrew, etc.
if check_help_for "notspecified" "curl" "--retry"; then
_retry_supported="--retry 3"
# "-C -" tells curl to automatically find where to resume the download when retrying.
_retry_supported="--retry 3 -C -"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check the --continue-at flag support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take a look at adding that tomorrow just to make sure, I did check when it was added when I was adding it and it seems to have been supported since 1998 I think it was? But probably better safe than sorry so I'll try to add a check for the support tomorrow

@Rustin170506
Copy link
Member

I did some manual testing by kill switching my network with my VPN (adding a video below to show the behavior).
unknown.webm

Looks good. Thank you!

I could look into adding an actual test case for this but I would probably need some guidance as I'm fairly new to rust and I wouldn't exactly be sure how to go about testing this properly

I don't think there's a way to test it in code, so manual testing is good enough.

@darkkeh
Copy link
Contributor Author

darkkeh commented Oct 24, 2022

Sorry that it took a while again ended up being more busy than expected.

Added the check to verify that --continue-at is available and cleaned up some indentation

Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you please squash your PR and make it just one commit?

Thanks!

@darkkeh
Copy link
Contributor Author

darkkeh commented Oct 26, 2022

There we go, commits have been squashed!

@Rustin170506 Rustin170506 merged commit b3d5325 into rust-lang:master Oct 27, 2022
@Rustin170506
Copy link
Member

Thanks again! 💚 💙 💜 💛 ❤️

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.

2 participants