Use powershell command for download #144

Petr-Hlavenka · 2018-03-05T14:01:28Z

Use powershell command as proposed in JuliaLang/julia#25477.
Later on, I'd prefer only one download function in Base that would work - or would be easy to overwrite in .juliarc for really specific configuration.

Fixes #47.

Use powershell command as proposed in JuliaLang#25477. Later on, I'd prefer only one download function in Base that would work - or would be easy to overwrite in .juliarc for really specific configuration.

nalimilan · 2018-03-05T17:13:15Z

I guess we could simply use Base.download here? Cc: @JeffBezanson

tkelman · 2018-03-05T22:34:35Z

kinda stale, but that's #81. either way, should preserve the retrying

Petr-Hlavenka · 2018-03-06T08:17:00Z

Good point, retying preserved. Good to know that in both JuliaLang/julia#25477. and this PR the TLS1.2 is required - and is only supported by .NET 4.5 and above. So on my win server 2012R2 this is installed by default, but for Win7, wou have .NET 3.5 bundled and you need to update it yourself. Nice summary here.

And I'm very positive to have only one download function in base and use it everywhere - but I'd also like a optional hook-in mechanism such that the user might customize it for some corner cases - on clusters, in corporate environment, etc... The custom routine might ask e.g. a linux machine by ssh to download it for you to a shared storage and than copy it from that place to the destination place.

Petr-Hlavenka · 2018-03-06T08:41:35Z

On Julia 0.7 replace command changed - added compat macro + version bump

nalimilan · 2018-03-06T10:13:03Z

So I guess we can't use Base.download due to the retry feature. AFAICT it's not used anywhere, though, and that argument is only supported on Windows (not sure we really use Unix, but still).

Petr-Hlavenka · 2018-03-07T15:36:03Z

I don't see a reason why we couldn't do the retry feature. Just replace run(....) with Base.download(...) in this PR and you have the retry functionality. Or we might add the retry parameter directly to Base.download.

It's really a huge pain to struggle with firewalls, SSL, TLS etc. to find out that only WinRPM has problem downloading a single "index file" whereas JuliaBase, BinDeps, Conda, pip and shell are OK. Or vice versa. For me, it should be fixable on one central place only. In the best case with some hook-in mechanism that allows the tweaking/hacking without the need to modify the code of Julia Base. Anyhow nobody (including me) would wellcome a PR to Base with a hack for CompanyXY and later another for CompanyYZ.

tkelman · 2018-03-08T21:54:30Z

src/WinRPM.jl

+        client = "New-Object System.Net.Webclient"
+        # in the following we escape ' with '' (see https://ss64.com/ps/syntax-esc.html)
+        filename = joinpath(tempdir(), split(source, "/")[end])        
+        downloadfile = "($client).DownloadFile('$(@compat replace(source, "'" => "''"))', '$(@compat replace(filename, "'" => "''"))')"        


shouldn't need the at-compat macro for these, believe they're method extensions in compat rather than syntax rewrites

Actually, the old method replace(str, pat, r) has been deprecated in 0.7 with replace(s::AbstractString, pat=>r; [count::Integer]). Therefore the @compat for use on both 0.6 and 1.0.

@compat is only needed for syntactic changes. Else you just need using Compat somewhere (either in the file or once for the whole package).

nalimilan · 2018-03-09T08:47:49Z

I don't see a reason why we couldn't do the retry feature. Just replace run(....) with Base.download(...) in this PR and you have the retry functionality.

Good point. Better try that then.

Petr-Hlavenka · 2018-03-29T07:40:54Z

The support for win7 starts to be a mess. For PS download using TLS1.2 PowerShell at least at version 3 is a must (PS 2.0 uses .Net 2.0 that has no chance to get TLS1.2 through). I'd recommend anyone on win7 to install PS 5.1 wmf 5.1.
On the other hand, once present, PS handles all the quirks of Corporate Windows Networking monster - so let's use that for the windows case.

omus · 2018-04-02T12:14:33Z

src/WinRPM.jl

        end
+        warn("""Unknown download failure. WinRPM download function relies on Windows PowerShell functionality.
+            Check that PowerShell 3 or higher is installed and TLS 1.2 protocol support enabled.)""")


Extra trailing ) in message

tkelman · 2018-04-03T04:23:52Z

and for people who don't have admin rights and are unable to update powershell?

Petr-Hlavenka · 2018-04-03T04:48:24Z

One of my motivation for this PR is to re-run discussion that didn't happen prior JuliaLang/julia#25477 rapid merges. Problems there would resurface for win users after 1.0 release - WinRPM might do the burn test in a more flexible way.

The original ccall to URLDownloadToFileW was a total disaster on any of windows servers - in order to allow your "unsigned" software use that API you have to overcome all the traps of IE ESC and various group policies enforced by the domain admin.

And it's only Windows7 that is affected, anything newer works out-of-the-box. And note that even with URLDownloadToFileW to work with the newly enforced TLS1.2 on GitHub you need to apply this Microsoft EasyFix that also requires admin rights - then you might also update the PS instead.

But, as I noted before, anything more elegant that always works would be better - and in that case, I'd like it to be implemented in Base.download and nowhere else.

tkelman · 2018-04-03T04:53:49Z

Changes to Base.download won't happen on 0.6 of course. Win 7 without admin rights is likely more common than any windows server versions, particularly without the admin rights needed there to adjust the security settings. There's no clear best single option here, but trying a few with fallbacks seems better than clearly regressing in a known and previously supported configuration.

Petr-Hlavenka · 2018-04-03T05:28:07Z

I see your point. I also didn't know there is the link to the EasyFix page directly below the 0.6.2 download link.
The problem I'm facing is that some Julia "package providers" have no problem on a "restricted windows platform" whereas some errors (in my case WinRPM). And figuring out which funciton where is failing is not simple.
In fact I'd really like one core function for downloading file that either works, or fails with a huge error message that suggest some helpful hints - or advices to replace it if a better solution for the platform exists. The user on exotic machine would replace it with whatever works for him, and then it would start to work for him everywhere.
What if we modify Base.download in 1.0 to accept keyword argument verbose=true and let it produce the long error message"? And review all WinRPM, Conda, BinDeps, etc.. to use the only one Base.download?

nalimilan · 2018-04-04T07:38:06Z

I'd be inclined to use the same approach as Base.download, whatever it is -- and if it has problems, discuss that in Julia instead of in WinRPM, which very few people follow. So here, that would mean using PowerShell if VERSION > v"0.7.0-DEV.XXX", and using the same method as currently on Julia 0.6.

Petr-Hlavenka · 2018-04-04T08:33:09Z

I'm not familiar with versioning during 0.7-DEV. How can I find the .XXXX number for the PR#25477?

nalimilan · 2018-04-04T08:46:43Z

Using contrib/commit_name.sh in the Julia source tree. That's 0.7.0-DEV.3392. Though we don't really care about the exact commit, the point is just to keep the current behavior on Julia 0.6 to avoid breaking anything.

Petr-Hlavenka · 2018-04-04T09:07:06Z

Let's keep the 0.6+ version untouched and let's just improve things for 0.7 - unification to Base.download regardless of windows/unix/mac. Added retry functionality + better error message on windows (as Base.download does not provide any.

nalimilan · 2018-04-04T09:12:57Z

src/WinRPM.jl

        end
+        warn("""Unknown download failure in `Base.download` function""")
+        if iswindows()


Can we just rethrow the exception raised by Base.download? It needs to print a more helpful error message anyway (e.g. it could check what PowerShell version is installed), so no need to duplicate that logic.

We shouldn't rethrow the error. This is not what the rest of WinRPM code expect - it expects to print the error as a warning and send a errcode !=200. The WinRPM.update function then silently recovers and tries a different provider.

I'd suggest catching an error thrown by Base.download, display it as a warning and continue the same path the old code expects. And we should update the Base.download function to give more info in its error exception.

Or, we can rewrite also the other parts of WinRPM - but that won't bring much functionality either.

Makes sense. Just printing the message should be OK.

Petr-Hlavenka · 2018-04-04T09:23:28Z

So, you think we should rather enhance the Base.download instead? We might add there a verbose keyword and call it here with verbose=true to get a more helpful message from them.
Or just enhance their error message. We might then just rethrow the last error after all attempts in the retry cycle fail.

nalimilan · 2018-04-04T09:51:05Z

Yes, but I don't think we need a verbose argument. Base should always print a detailed error message (ideally detecting cases where the PowerShell error applies). It wouldn't make a lot of sense to expect people to pass verbose=true to get information about the failure: if they knew the problem in the first place they wouldn't need the message.

Petr-Hlavenka · 2018-04-04T14:38:49Z

OK, so let's just do the minimal change, only affecting 0.7. Since 0.7 we'll rely on Base.download functionality for everything, so it will be possible to redefine it in the case a hack is needed.

tkelman · 2018-04-04T18:08:10Z

src/WinRPM.jl

-                resize!(dest, findfirst(iszero, dest) - 1)
-                filename = transcode(String, dest)
+            try
+                filename = joinpath(tempdir(), split(source, "/")[end])


this looks highly likely to have collisions

So you'd rather not enforce the filename?

No, don't drop most of source.

Update the branch

Use powershell command for download

3bda801

Use powershell command as proposed in JuliaLang#25477. Later on, I'd prefer only one download function in Base that would work - or would be easy to overwrite in .juliarc for really specific configuration.

retrying preserved

b830d32

Petr-Hlavenka added 2 commits March 6, 2018 09:38

add @compat to replace

4c5d007

Due @compat replace, update Compat version

3860451

tkelman reviewed Mar 8, 2018

View reviewed changes

nalimilan mentioned this pull request Mar 9, 2018

Download failure #145

Closed

Petr-Hlavenka added 2 commits March 28, 2018 14:32

Add warning for .Net version

c2fe28c

removed @compat and mention PS>=3

fccd061

omus reviewed Apr 2, 2018

View reviewed changes

fix trailing )

f19c019

Petr-Hlavenka mentioned this pull request Apr 3, 2018

support sha check in download, require during Pkg.build JuliaLang/julia#26683

Closed

Use Base.download on 0.7

9b5860c

nalimilan reviewed Apr 4, 2018

View reviewed changes

Petr-Hlavenka and others added 2 commits April 4, 2018 16:33

Just print download error exception

6de4f21

revert require

c5a7269

tkelman reviewed Apr 4, 2018

View reviewed changes

sgryjp and others added 9 commits July 20, 2019 15:48

Merge commit 'b6a1418'

490894e

Merge commit 'c2803c9'

5d71829

Merge commit '0b05b81'

4d7a98c

Merge commit 'f7fb30e'

aa460e8

Merge commit 'aa56cac'

313815c

Merge commit 'cb7a2fa'

8e13c39

Merge commit '5ba232c'

de0bebb

Merge commit '03e3556'

521e0e1

Merge pull request #1 from sgryjp/master

e8bac66

Update the branch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use powershell command for download #144

Use powershell command for download #144

Petr-Hlavenka commented Mar 5, 2018 •

edited by nalimilan

Loading

nalimilan commented Mar 5, 2018

tkelman commented Mar 5, 2018

Petr-Hlavenka commented Mar 6, 2018

Petr-Hlavenka commented Mar 6, 2018

nalimilan commented Mar 6, 2018

Petr-Hlavenka commented Mar 7, 2018

tkelman Mar 8, 2018

Petr-Hlavenka Mar 28, 2018

nalimilan Mar 28, 2018

nalimilan commented Mar 9, 2018

Petr-Hlavenka commented Mar 29, 2018

omus Apr 2, 2018

Petr-Hlavenka Apr 3, 2018

tkelman commented Apr 3, 2018

Petr-Hlavenka commented Apr 3, 2018

tkelman commented Apr 3, 2018

Petr-Hlavenka commented Apr 3, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

nalimilan Apr 4, 2018

Petr-Hlavenka Apr 4, 2018

nalimilan Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

tkelman Apr 4, 2018

Petr-Hlavenka Apr 5, 2018

tkelman Apr 5, 2018

Use powershell command for download #144

Are you sure you want to change the base?

Use powershell command for download #144

Conversation

Petr-Hlavenka commented Mar 5, 2018 • edited by nalimilan Loading

nalimilan commented Mar 5, 2018

tkelman commented Mar 5, 2018

Petr-Hlavenka commented Mar 6, 2018

Petr-Hlavenka commented Mar 6, 2018

nalimilan commented Mar 6, 2018

Petr-Hlavenka commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Mar 9, 2018

Petr-Hlavenka commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 3, 2018

Petr-Hlavenka commented Apr 3, 2018

tkelman commented Apr 3, 2018

Petr-Hlavenka commented Apr 3, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Petr-Hlavenka commented Apr 4, 2018

nalimilan commented Apr 4, 2018

Petr-Hlavenka commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Petr-Hlavenka commented Mar 5, 2018 •

edited by nalimilan

Loading