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 Downloads stdlib #37340

Merged
merged 5 commits into from
Sep 16, 2020
Merged

add Downloads stdlib #37340

merged 5 commits into from
Sep 16, 2020

Conversation

StefanKarpinski
Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski commented Sep 2, 2020

This PR adds a Downloads stdlib and replaces the Base.download function which relies on a miscellany of external programs (curl, wget, PowerShell) which may or may not happen to be present in order to download files, with Downloads.download which uses libcurl to download files in the same way everywhere. In the process, we also gain support for passing headers and more (to be exposed and used in Pkg in future PRs).

@KristofferC KristofferC added the needs docs Documentation for this change is required label Sep 2, 2020
@StefanKarpinski StefanKarpinski force-pushed the sk/downloader branch 2 times, most recently from ce8ebdf to c7c4d35 Compare September 2, 2020 21:44
stdlib/LibCURL.version Outdated Show resolved Hide resolved
@StefanKarpinski StefanKarpinski marked this pull request as draft September 5, 2020 23:03
@StefanKarpinski StefanKarpinski force-pushed the sk/downloader branch 4 times, most recently from 78619e9 to 1957fed Compare September 10, 2020 19:41
@StefanKarpinski StefanKarpinski changed the title add Downloader stdlib add Downloads stdlib Sep 10, 2020
@StefanKarpinski StefanKarpinski force-pushed the sk/downloader branch 2 times, most recently from be36c1f to 70652d7 Compare September 11, 2020 00:22
@StefanKarpinski StefanKarpinski marked this pull request as ready for review September 11, 2020 00:24
@StefanKarpinski
Copy link
Sponsor Member Author

Aside from passing CI, I think this PR is mergeable. @KristofferC, you had marked it as "needs docs" but I'm not entirely sure why—was that because the Downloads stdlib doesn't have docs? Currently, I'm just viewing this as a replacement for existing functionality and I've updated the Base.download docs in this PR.

@StefanKarpinski
Copy link
Sponsor Member Author

The Win64 tester keeps hanging, I have no idea why. It was passing previously, so I'm not sure if this is related to this PR or just because there's something up with the infrastructure. @staticfloat, any idea what's up?

@KristofferC KristofferC removed the needs docs Documentation for this change is required label Sep 11, 2020
@musm
Copy link
Contributor

musm commented Sep 11, 2020

This is awesome, and closes many long standing issues.

If this is now a stdlib, shouldn't there now be a depwarn for Base.download?

@StefanKarpinski
Copy link
Sponsor Member Author

Yes, I could add one since deprecation warnings default to off these days, right?

@StefanKarpinski
Copy link
Sponsor Member Author

This is awesome, and closes many long standing issues.

Yep, and the next step is to use it for downloads in Pkg as well, so even more problems solved there. Figuring out how to use libcurl correctly from Julia with libuv was pretty tricky, but now that it works, libcurl is great: it's very portable and has every feature we might ever want for downloading/uploading stuff—all the protocols, progress support, authentication, etc. Since I have a good understanding of the API now, if we need any additional features in the future, I'm pretty confident that libcurl has them and that we can add them easily.

@KristofferC
Copy link
Sponsor Member

Regarding the docs question, I just meant an entry in the manual so it is listed as an stdlib with the API it has, like the other stdlibs.

@StefanKarpinski
Copy link
Sponsor Member Author

Right, I can add that once I've got it working.

@StefanKarpinski StefanKarpinski added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Sep 13, 2020
@StefanKarpinski
Copy link
Sponsor Member Author

Ok, well, this finally works. Turns out the Win64 hang was a libcurl bug. Upgrading libcurl fixes it, but requires adding nghttp2 as a dependency. That's fine, since we want to support HTTP/2 anyway. For the record fde145d was the commit that worked.

@StefanKarpinski
Copy link
Sponsor Member Author

StefanKarpinski commented Sep 16, 2020

Anything else needed here besides news and a manual section?

@StefanKarpinski StefanKarpinski marked this pull request as ready for review September 16, 2020 07:24
@StefanKarpinski StefanKarpinski added domain:io Involving the I/O subsystem: libuv, read, write, etc. and removed needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Sep 16, 2020
- Base.download is deprecated in favor of Downloads.download
- the former is now implemented by calling the latter
@StefanKarpinski
Copy link
Sponsor Member Author

StefanKarpinski commented Sep 16, 2020

Deprecating Base.download is non-trivial since we run tests with depwarn=error. As these things so often seem to be, it's more involved than you'd imagine. Having Base.download be an error during testing can be worked around and I have a pull-request that does that, but LibCURL uses Base.download in its tests, which presents a particular challenge. This is fine when it's running its own tests, since those are run with depwarn=no, but when we run the LibCURL tests, we're stricter, so it fails.

The best fix for that is to just change it to use Downloads.download instead of Base.download, but in order for that to work on older Julia's, it requires Downloads to be registered as an official package, a process that I've started but which won't go through for another three days. Once that happens, I can fix LibCURL's tests to use Downloads as a normal package on 1.3-1.5 and as a stdlib on 1.6+, after which it will pass tests as a stdlib with depwarn=error and we can actually deprecate Base.download without breaking CI.

@ViralBShah
Copy link
Member

This is pretty amazing!

Off-topic, we really need to move away from having a file for each download hash.

@haampie
Copy link
Contributor

haampie commented Sep 16, 2020

@StefanKarpinski can you try ] add JuMP on this PR, seems like it's broken:

(Pkg) pkg> add JuMP
   Updating registry at `/dev/shm/.julia/registries/General`
  Resolving package versions...
ERROR: Dependency `Test` in target `test` not listed in `deps` or `extras` section.

[deps]
Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb"

[extra]
Copy link
Member

Choose a reason for hiding this comment

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

s

name = "MozillaCACerts_jll"
uuid = "14a3606d-f60d-562e-9121-12d972cd8159"

[extra]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@fredrikekre
Copy link
Member

ERROR: Dependency Test in target test not listed in deps or extras section.

Where is this thrown from?

@haampie
Copy link
Contributor

haampie commented Sep 16, 2020

I've already compiled an older commit, so I cannot easily go back to see :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants