Skip to content

Add DownloadFile task#3297

Merged
AndyGerlicher merged 4 commits intodotnet:masterfrom
jeffkl:downloadfile
May 12, 2018
Merged

Add DownloadFile task#3297
AndyGerlicher merged 4 commits intodotnet:masterfrom
jeffkl:downloadfile

Conversation

@jeffkl
Copy link
Copy Markdown
Contributor

@jeffkl jeffkl commented May 11, 2018

  • Supports cancellation
  • Supports retries (off by default)
  • Supports skipping download if file is up-to-date (on by default)
  • Supports getting file name from response headers or URL if possible
  • Has output parameter with downloaded path

Fixes #3219

Follow-up items:

  • Documentation for DownloadFile task

* Supports cancellation
* Supports retries (off by default)
* Supports skipping download if file is up-to-date (on by default)
* Supports getting file name from URL if possible
* Has output parameter with downloaded path
@rainersigwald
Copy link
Copy Markdown
Member

Mention "closes #3219" please. fyi @kasper3

Comment thread src/Tasks/DownloadFile.cs
// Not all URIs contain a file name so users will have to specify one
// Example: http://www.download.com/file/1/

filename = !String.IsNullOrWhiteSpace(DestinationFileName?.ItemSpec)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we call TrygetFileName after we have recieved the headers?

Precedence (descending order):

  • DestinationFileName (user-specified)
  • client.ResponseHeaders["content-disposition"], if DestinationFileName is empty
  • url.LocalPath, if DestinationFileName is empty and client.ResponseHeaders["content-disposition"] is non-empty and meaningful (<file-name>.<extension> format)

so we can call urls like https://lorempizza.com/380/240, we get this kind of filename from response header: imagedoc-darknoise.png (instead of 240)

@ghost
Copy link
Copy Markdown

ghost commented May 11, 2018

@jeffkl, first of all kudos on the good HttpClient synchronous implementation with the disposables in correct place! 👍

I have left one comment about content disposition, which @CZEMacLeod mentioned in #3220.

Secondly, this utility is good as it is, we can safely say that it addresses #3219.

#3219 sounds like it wants to have a way to download a file before evaluation. In my opinion we shouldn't do that so I just said this PR is related.

True, that is #3220, we can discuss it separately to make DependsOnTask semantics for design-time evaluation friendlier. In VS's Solution Explorer, the delayed inclusions (<ItemGroup> <Compile Include="/some/other/root/**/*.cs" /> </ItemGroup> inside the target, does not get showed up, but MSBuild does compile), so the issue is to have some friendly/implicit semantics for consumer to reevaluate the design time target, that aids the VS Solution Explorer. Hence the idea of DependsOnTask to prevent consumer from entering the domain of "doing too much" in their project files. If that is technically impossible, then I accept that. :)

@AndyGerlicher
Copy link
Copy Markdown
Contributor

Only concern I had here is the SkipUnchangedFiles flag just checking the size and defaulting to true. I could easily see this getting used to download a version file and ending up in scenarios that are hard to debug when the file doesn't always change lengths. For binaries it's probably ok though?

One thought that's not a great solution but could help is always download when under a certain size or content type?

@CZEMacLeod
Copy link
Copy Markdown

@AndyGerlicher I don't know how you would store the information, but similarly to the Content-Disposition, perhaps the caching headers could get used, and/or store the ETag? Could they get stored in alternate streams on the file, or is that too platform specific? Some kind of metadata cache file would allow for this. If the file exists, you could use the last updated date and add the If-Modified-Since header to prevent downloading the file again... Or use the info about when it last downloaded from the aforementioned metadata.

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented May 11, 2018

I found a LastModified property (here) that I'm going to use.

If the property is not set, then it will download the file again.

@magol
Copy link
Copy Markdown

magol commented May 11, 2018

Does it support download via other protocols than http(s), such as FTP or SMB/CIFS?

@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented May 11, 2018

@magol

Does it support download via other protocols than http(s), such as FTP or SMB/CIFS?

Not at the moment, it only supports HTTP endpoints

@magol
Copy link
Copy Markdown

magol commented May 11, 2018

@jeffkl

Not at the moment, it only supports HTTP endpoints

But it is possible that it is added later?

@AndyGerlicher AndyGerlicher merged commit d1998aa into dotnet:master May 12, 2018
@jeffkl
Copy link
Copy Markdown
Contributor Author

jeffkl commented May 14, 2018

@magol possibly. This currently does a simple non-authenticated HTTP GET. If you need more functionality, please feel free to contribute.

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.

5 participants