This repository has been archived by the owner. It is now read-only.

Make `CurlDownloadStrategy` resume aborted downloads #13953

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

urdh commented Aug 4, 2012

Having curl resume aborted downloads has been discussed earlier in #10978, #6194 and other issues, but never resolved. This is an attempt to introduce a way of making curl resume downloads. Comments and suggestions are welcome.

  • CurlDownloadStrategy#_fetch (and the same methods in its subclasses) now fetches the file to a temporary path, and CurlDownloadStrategy#fetch moves it to the correct location.
  • Homebrew#cleanup cleans the temporary files CurlDownloadStrategy creates if they're left in the cache.
Contributor

urdh commented Aug 4, 2012

@adamv: Fixed. (If I understood your comment correctly.)

Owner

MikeMcQuaid commented Aug 5, 2012

Not tested it yet but looks good to me. Nice work.

Member

mxcl commented Aug 6, 2012

Why temporary path? I'd rather the patch was minimal as possible in case of bugs.

Contributor

urdh commented Aug 7, 2012

@mxcl: Leaving half-completed files in tarball_path makes brew think the file is already downloaded. Changing that check would probably mean having to send a HEAD request to the server to see if the file is complete, which seems much more fragile.

Member

mxcl commented Aug 7, 2012

Yep, makes sense.

Member

mxcl commented Aug 7, 2012

I propose that if the user does CTRL-C we should delete the incomplete download.

Owner

MikeMcQuaid commented Aug 7, 2012

I don't know, when I was in crapternet in Australia I'd abort downloads with Ctrl-C to resume them later (because I had to e.g. leave the coffee shop). If they are cleaned up on successful download (and probably on the newer version being installed like brew cleanup does) then I think that would be good.

Contributor

tyd commented Aug 19, 2012

@MikeMcQuaid I agree with your comment about wanting to resume later. How about prompting the user to remove or keep partially downloaded files on cntrl-c? or add a switch to change whatever the default behavior ends up being.

Contributor

urdh commented Aug 19, 2012

Personally I'd be against asking the user things on Ctrl+C (I would expect the program to exit as cleanly as possible without asking me stuff). I'd also advocate saving unfinished downloads on Ctrl+C as default behaviour, along the same lines as @MikeMcQuaid. As this patch is written now, brew cleanup removes all unfinished downloads anyway, but some kind of flag to make brew not save unfinished downloads might make sense. I'll wait and see what the maintainers think about this before changing the pull request.

Member

mxcl commented Aug 21, 2012

CLI tools should never prompt, they should do a reversible, sensible, default action.

Anyway, I agree, CTRL-C should not delete.

Member

mxcl commented Aug 21, 2012

Well, I think we should pull and test. Good work @urdh.

Contributor

urdh commented Oct 20, 2012

I have rebased this on top of master if there's still interest in pulling this.

Owner

MikeMcQuaid commented Feb 1, 2013

@urdb Sorry to be a pain. Any chance of rebasing again and I'll test and merge?

Make `CurlDownloadStrategy` resume aborted downloads
* `CurlDownloadStrategy#_fetch` (and the same methods in its subclasses) now fetches the file to a temporary path, and `CurlDownloadStrategy#fetch` moves it to the correct location.
* `Homebrew#cleanup` cleans the temporary files `CurlDownloadStrategy` creates if they're left in the cache.
Contributor

urdh commented Feb 1, 2013

@MikeMcQuaid Done.

Owner

MikeMcQuaid commented Feb 2, 2013

Really sorry it took so long. This is a great feature.

Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Feb 19, 2013

Make `CurlDownloadStrategy` resume aborted downloads
* `CurlDownloadStrategy#_fetch` (and the same methods in its
  subclasses) now fetches the file to a temporary path, and
  `CurlDownloadStrategy#fetch` moves it to the correct location.
* `Homebrew#cleanup` cleans the temporary files `CurlDownloadStrategy`
  creates if they're left in the cache.

Closes #13953.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.