download with powershell 2.0 for windows #21968
Conversation
base/interactiveutil.jl
Outdated
| error("automatic download failed (error: $res): $url") | ||
| end | ||
| ps = "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" | ||
| res = run(`$ps -NoProfile -Command "(New-Object System.Net.Webclient).DownloadFile(\"$url\", \"$filename\")"`) |
There was a problem hiding this comment.
DownloadFile('$(replace(url, "'", "''"))', '$(replace(filename, "'", "''"))'
There was a problem hiding this comment.
I admit I don't know what that does here, seem more complicated but does the same job?
There was a problem hiding this comment.
it ensures that meta characters (`, $, ') pass through unscathed
There was a problem hiding this comment.
should have some tests for this then? likewise with the unix version
There was a problem hiding this comment.
what is a counterexample and how does this fix it?
There was a problem hiding this comment.
@vtjnash (mind providing some test cases for this since I don't understand why it's needed?)
|
Also, do we need to enable secure HTTPS support explicitly (JuliaPackaging/WinRPM.jl#57 (comment))? |
|
I enabled https support. Yes it looks like we need to do it explicitly. |
|
bump? |
| @@ -571,11 +571,10 @@ end | |||
| downloadcmd = nothing | |||
There was a problem hiding this comment.
may as well set this, if a command is being used
There was a problem hiding this comment.
I think downloadcmd can be removed and just be a local variable.
However this is unrelated to the changes in this PR. Let's save this for a separate PR
There was a problem hiding this comment.
Not sure on a good way to avoid the global here?
There was a problem hiding this comment.
you're changing the download function to shell out to something, the value of what you're shelling out to should be saved in the global that exists for that purpose
base/interactiveutil.jl
Outdated
| end | ||
| ps = "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" | ||
| enable_tls12 = "[System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls12" | ||
| download_cmd = "(New-Object System.Net.Webclient).DownloadFile('$(replace(url, "'", "''"))', '$(replace(filename, "'", "''"))')" |
There was a problem hiding this comment.
this replacement doesn't make a lot of sense, could use comments and tests to show what it's accomplishing and why it's necessary
There was a problem hiding this comment.
See comment at #21968 (comment) . Not really sure why it's needed so someoneelse would need to add tests and comments for this
There was a problem hiding this comment.
try fetching a URL stuffed with metacharacters like https://httpbin.org/get?test='^"
There was a problem hiding this comment.
I think I see your point, but don't you want
replace(filename, "'", "\'")
instead of
replace(filename, "'", "''")
or are those the same
and is this only needed here since we first interpolate the string into another string before interpolating into the cmd run( ) ? I'm just trying to understand why the non windows download cmd doesn't need this.
There was a problem hiding this comment.
No, the escape sequence for ' is '', not \'
Non-windows can call the download program directly, and doesn't need to involve a shell.
There was a problem hiding this comment.
no, the escape-character is context sensitive. I used single quotes because the rules are simplest (https://ss64.com/ps/syntax-esc.html)
|
test added |
base/interactiveutil.jl
Outdated
| @@ -598,7 +598,7 @@ else | |||
| rethrow() | |||
| end | |||
| elseif downloadcmd == :curl | |||
| run(`curl -L -f -o $filename $url`) | |||
| run(`curl -L -f -s -o $filename $url`) | |||
There was a problem hiding this comment.
you've seen JuliaPackaging/BinDeps.jl#135, but this function could probably use a more serious refactoring - not everyone will want all downloads to be silent. you can use curlrc to control some of these globally if you know you're using curl
There was a problem hiding this comment.
Yeah I think that will require more serious thinking about the api etc. I think for now we should at least try to make the experience consistent; dumping out the progress bar from curl directly doesn't seem very Julian. I agree it would be useful to have some control over the progress etc.
|
anyone mind retriggering travis ? |
|
Done. |
|
I plan on merging this tomorrow unless someone has an objection |
|
This should set the downloadcmd global. |
|
|
|
Now that the download function shells out on windows, that variable is relevant on windows |
|
How so? Looks to me that |
|
set it to powershell on windows. |
|
Do we know what using powershell2 instead of |
|
The belief is that this will have better support for redirects, cookies, authentication, and file size among other issues |
base/interactiveutil.jl
Outdated
| @@ -591,17 +591,19 @@ end | |||
|
|
|||
| # file downloading | |||
|
|
|||
| downloadcmd = nothing | |||
There was a problem hiding this comment.
don't think this should be moved
|
The only thing that is disappointing with shelling is that it is slower than the call to |
|
powershell has a kind of annoying first-time startup delay, hopefully isn't as bad if you download multiple things in a julia session |
Fair enough. Should we merge this then? |
|
bumping for decision |
|
It may be worth adding this as a non-functional change to |
|
BinDeps already has been using powershell in its download_cmd |
|
Right, but at least then it would be using the same implementation (e.g. it doesn't look like it correctly escapes urls). |
|
@tkelman Would you be on board with that? I.e. replace all uses of download with the definitions in base? |
|
The BinDeps version would have to be deprecated, as it's used by other packages |
|
Should we merge this or leave it as is ? |
|
I don't think this is important and causing issues |
|
What kind of issues? I thought it was supposed to reduce download failures, which would be a major improvement. The PR has been approved for weeks, so why close it now? |
|
Rebased: #25477 |
Supported on all versions starting from windows 7