download with powershell 2.0 for windows #21968

musm · 2017-05-19T17:17:50Z

Supported on all versions starting from windows 7

vtjnash · 2017-05-19T18:14:13Z

base/interactiveutil.jl

-            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\")"`)


DownloadFile('$(replace(url, "'", "''"))', '$(replace(filename, "'", "''"))'

I admit I don't know what that does here, seem more complicated but does the same job?

it ensures that meta characters (`, $, ') pass through unscathed

should have some tests for this then? likewise with the unix version

what is a counterexample and how does this fix it?

@vtjnash (mind providing some test cases for this since I don't understand why it's needed?)

vtjnash · 2017-05-19T18:39:21Z

Also, do we need to enable secure HTTPS support explicitly (JuliaPackaging/WinRPM.jl#57 (comment))?

musm · 2017-05-19T20:44:08Z

I enabled https support. Yes it looks like we need to do it explicitly.
I updated the code to always enable https tls1.2 support.

musm · 2017-05-20T01:39:21Z

refs #19526
JuliaPackaging/BinDeps.jl#135

musm · 2017-05-25T22:05:02Z

bump?

tkelman · 2017-05-25T22:22:29Z

base/interactiveutil.jl

@@ -571,11 +571,10 @@ end
 downloadcmd = nothing


may as well set this, if a command is being used

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

Not sure on a good way to avoid the global here?

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

tkelman · 2017-05-25T22:36:09Z

base/interactiveutil.jl

-        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, "'", "''"))')"


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

See comment at #21968 (comment) . Not really sure why it's needed so someoneelse would need to add tests and comments for this

try fetching a URL stuffed with metacharacters like https://httpbin.org/get?test='^"

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.

No, the escape sequence for ' is '', not \'

Non-windows can call the download program directly, and doesn't need to involve a shell.

you mean `' ?

no, the escape-character is context sensitive. I used single quotes because the rules are simplest (https://ss64.com/ps/syntax-esc.html)

musm · 2017-06-02T15:30:10Z

test added

tkelman · 2017-06-02T16:00:11Z

base/interactiveutil.jl

@@ -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`)


separate PR please

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

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.

musm · 2017-06-02T21:23:57Z

anyone mind retriggering travis ?

nalimilan · 2017-06-05T19:05:53Z

Done.

KristofferC · 2017-06-26T01:34:17Z

I plan on merging this tomorrow unless someone has an objection

tkelman · 2017-06-26T01:48:35Z

This should set the downloadcmd global.

musm · 2017-06-26T02:26:36Z

downloadcmd is only relevant for non windows systems. I have made no modifications to this variable in this PR, aside from placing it in the non windows branch, since it does not affect windows systems.

tkelman · 2017-06-26T03:03:05Z

Now that the download function shells out on windows, that variable is relevant on windows

musm · 2017-06-26T03:11:06Z

How so? Looks to me that downloadcmd is used on linux to test which download shell function to use. On windows we only have powershell so I am a bit confused as to your suggestion?

tkelman · 2017-06-26T03:25:48Z

set it to powershell on windows.

musm · 2017-07-08T22:00:35Z

Do we know what using powershell2 instead of URLDownloadToFileW solves? Is it possible proxy issues or something else. The docs mention that URLDownloadToFileW is limited to downloading 4GB files.

vtjnash · 2017-07-09T00:57:13Z

The belief is that this will have better support for redirects, cookies, authentication, and file size among other issues

tkelman

needs rebase

tkelman · 2017-07-15T18:08:02Z

base/interactiveutil.jl

@@ -591,17 +591,19 @@ end

 # file downloading

-downloadcmd = nothing


don't think this should be moved

musm · 2017-07-15T18:20:07Z

The only thing that is disappointing with shelling is that it is slower than the call to URLDownloadToFileW ....

tkelman · 2017-07-16T14:47:31Z

powershell has a kind of annoying first-time startup delay, hopefully isn't as bad if you download multiple things in a julia session

musm · 2017-07-17T23:02:58Z

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?

musm · 2017-07-24T18:30:16Z

bumping for decision

musm · 2017-07-28T03:40:43Z

It may be worth adding this as a non-functional change to Compat and using that as Compat.download in BinDeps and WinRPM , thoughts on this ?

tkelman · 2017-07-28T03:46:15Z

BinDeps already has been using powershell in its download_cmd

musm · 2017-07-28T03:47:40Z

Right, but at least then it would be using the same implementation (e.g. it doesn't look like it correctly escapes urls).

musm · 2017-07-28T21:31:13Z

@tkelman Would you be on board with that? I.e. replace all uses of download with the definitions in base?

tkelman · 2017-07-28T21:35:55Z

The BinDeps version would have to be deprecated, as it's used by other packages

musm · 2017-08-05T21:38:14Z

Should we merge this or leave it as is ?

musm · 2017-09-17T20:19:29Z

I don't think this is important and causing issues

nalimilan · 2017-09-17T21:08:00Z

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?

JeffBezanson · 2018-01-09T21:24:22Z

Rebased: #25477

vtjnash reviewed May 19, 2017

View reviewed changes

ararslan added the system:windows Affects only Windows label May 19, 2017

tkelman reviewed May 25, 2017

View reviewed changes

vtjnash approved these changes May 25, 2017

View reviewed changes

tkelman reviewed May 25, 2017

View reviewed changes

musm force-pushed the patch-6 branch from 34936f4 to 18de40e Compare June 2, 2017 15:29

Use powershell for download function for Windows

a909b64

musm force-pushed the patch-6 branch from 18de40e to a909b64 Compare June 2, 2017 15:39

Move global downloadcmd to non windows branch

64cb9ae

tkelman reviewed Jun 2, 2017

View reviewed changes

musm force-pushed the patch-6 branch from 97e8bb4 to 64cb9ae Compare June 2, 2017 16:01

musm added 2 commits June 25, 2017 23:38

set downloadcmd to powershell

6f215f7

whites fix

d5616eb

tkelman approved these changes Jul 15, 2017

View reviewed changes

Merge branch 'master' into patch-6

2976fb7

tkelman reviewed Jul 15, 2017

View reviewed changes

Update interactiveutil.jl

33b582b

Update download.jl

569d69a

musm closed this Sep 17, 2017

tkelman mentioned this pull request Jan 9, 2018

download function not working on Windows #19526

Closed

JeffBezanson reopened this Jan 9, 2018

JeffBezanson closed this Jan 9, 2018

musm deleted the patch-6 branch May 15, 2019 16:59

		@@ -591,17 +591,19 @@ end

		# file downloading

		downloadcmd = nothing

download with powershell 2.0 for windows #21968

download with powershell 2.0 for windows #21968

Conversation

musm commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented May 19, 2017

musm commented May 19, 2017

musm commented May 20, 2017

musm commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Jun 2, 2017

nalimilan commented Jun 5, 2017

KristofferC commented Jun 26, 2017

tkelman commented Jun 26, 2017

musm commented Jun 26, 2017 • edited Loading

tkelman commented Jun 26, 2017

musm commented Jun 26, 2017

tkelman commented Jun 26, 2017

musm commented Jul 8, 2017 • edited Loading

vtjnash commented Jul 9, 2017

tkelman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Jul 15, 2017

tkelman commented Jul 16, 2017

musm commented Jul 17, 2017

musm commented Jul 24, 2017

musm commented Jul 28, 2017

tkelman commented Jul 28, 2017

musm commented Jul 28, 2017

musm commented Jul 28, 2017

tkelman commented Jul 28, 2017

musm commented Aug 5, 2017

musm commented Sep 17, 2017

nalimilan commented Sep 17, 2017

JeffBezanson commented Jan 9, 2018

musm commented Jun 26, 2017 •

edited

Loading

musm commented Jul 8, 2017 •

edited

Loading

tkelman left a comment •

edited

Loading