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

use Base.download to get archives #280

Closed
wants to merge 1 commit into from
Closed

use Base.download to get archives #280

wants to merge 1 commit into from

Conversation

simonbyrne
Copy link
Contributor

Note: I haven't actually tested this (see #279).

@KristofferC
Copy link
Sponsor Member

What the reason for BinaryProvider not using Base.download @staticfloat

@KristofferC
Copy link
Sponsor Member

This now prints

 Resolving package versions...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   128    0   128    0     0   1809      0 --:--:-- --:--:-- --:--:--  1828
  0     0    0  2132    0     0  17149      0 --:--:-- --:--:-- --:--:-- 2082k

and we spawn multiple downloads at once so it will look pretty bad I think.

@codecov-io
Copy link

Codecov Report

Merging #280 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   88.39%   88.38%   -0.01%     
==========================================
  Files          16       16              
  Lines        3464     3462       -2     
==========================================
- Hits         3062     3060       -2     
  Misses        402      402
Impacted Files Coverage Δ
src/Operations.jl 92.63% <100%> (-0.02%) ⬇️
src/PlatformEngines.jl 59.78% <0%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 053a1fb...3921c51. Read the comment docs.

@staticfloat
Copy link
Sponsor Member

Back in the day, it was because the powershell implementation was broken, and we couldn't update people's Base as easily as we could update BinaryProvider. Only three reasons nowadays:

(1) More flexible output handling

(2) Resume capabilities (e.g. for large downloads on flaky connections)

(3) Doesn't require the usage of the which utility. (This will be fixed by JuliaLang/julia#26559)

@simonbyrne
Copy link
Contributor Author

My opinion is that we should either be using the one in Base, or don't have it in Base at all. It is kind of absurd to have different implementations.

@KristofferC
Copy link
Sponsor Member

Well, the docs of download says

For production use or situations in which more options are needed, please use a package that provides the desired functionality instead.

I wanted to easily redirect the output, so that's what I did.

@simonbyrne
Copy link
Contributor Author

We should probably add a "suppress output" option to Base.download then.

@staticfloat
Copy link
Sponsor Member

In Pkg3, I tend to agree, but BinaryProvider has pretty specific requirements in that we want to use the OutputCollector functionality which does things like logs to files, interleaves stdout and stderr on a per-line basis, colorizes output, etc..... We want all of that, and so we can't just use a download() function, we need something that provides a Cmd that we can then pass to OutputCollector, because the only way to get the stdout and stderr streams directly is to call spawn() directly. So I don't think BinaryProvider will be able to use Base.download().

@simonbyrne
Copy link
Contributor Author

simonbyrne commented May 9, 2018 via email

@staticfloat
Copy link
Sponsor Member

I don't see how that changes my argument

If by that you mean "this change should still be merged regardless of what BinaryProvider does" then yes, I agree and that's what I was trying to communicate.

If by that you mean "My point still stands about only having one download() function, and all implementations should use the one implementation in Base, I believe my point above is addressing that pretty well; I don't want a download() function, I want a gen_download_cmd() function, and that doesn't seem like something that would be exported from Base, so we resort to writing our own.

@simonbyrne
Copy link
Contributor Author

My understanding (which might be wrong) is that BinaryProvider.jl is intended to become part of the stdlib, in which case we will end up with 2 "download" implementations in the standard Julia distribution.

I don't see why your requests (which are entirely reasonable) can't be accommodated in the standard Julia download function, wherever it might live.

That said, this might better be discussed as an issue in the julia repo itself.

@simonbyrne
Copy link
Contributor Author

re: silent download, there is an old PR JuliaLang/julia#15930

@KristofferC
Copy link
Sponsor Member

Base.download doesn't provide the flexibility we need right now. We can revisit when it does.

@KristofferC KristofferC deleted the sb/download branch August 12, 2018 15:17
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.

4 participants