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

Julia v0.7 support #143

Merged
merged 46 commits into from
Aug 21, 2018
Merged

Julia v0.7 support #143

merged 46 commits into from
Aug 21, 2018

Conversation

rdeits
Copy link
Collaborator

@rdeits rdeits commented Aug 17, 2018

Includes #139 plus a few more deprecation fixes. Also now checks out WebIO.jl master on 0.7+

Fixes #132
Fixes #145

@piever
Copy link
Collaborator

piever commented Aug 17, 2018

I'm trying to make Interact functional again and am looking for an environment where things are supposed to work (among Blink, Mux, jupyter notebook). Is this PR + Blink the best bet?

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 17, 2018

Yup, I have the tests passing locally for Blink with WebIO master and this PR. You'll also need JuliaGizmos/JSExpr.jl#16 on Julia 1.0

@piever
Copy link
Collaborator

piever commented Aug 17, 2018

Cool, on my machine I have the first Interact slider on Julia 1.0! Thank you so much for all the amazing updating work!

@sglyon
Copy link
Contributor

sglyon commented Aug 18, 2018

Thank you @rdeits for all the hard work on getting this and related packages working! We owe you

@pfitzseb pfitzseb mentioned this pull request Aug 18, 2018
@piever
Copy link
Collaborator

piever commented Aug 19, 2018

Now that WebIO and JSExpr PRs have been merged, I guess this one can also be merged and we can tag Blink 0.8.0 ?

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 19, 2018

I'll update this PR to check out WebIO and JSExpr master on travis, just to make sure everything is actually working first.

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 19, 2018

Huh, appveyor is consistently failing when trying to uninstall AtomShell:

ERROR: LoadError: IOError: unlink: operation not permitted (EPERM)
Stacktrace:
 [1] uv_error at .\libuv.jl:85 [inlined]
 [2] unlink(::String) at .\file.jl:743
 [3] #rm#9(::Bool, ::Bool, ::Function, ::String) at .\file.jl:253
 [4] #rm at .\none:0 [inlined]
 [5] #rm#9(::Bool, ::Bool, ::Function, ::String) at .\file.jl:263
 [6] #rm at .\none:0 [inlined]
 [7] rm′ at C:\projects\blink-jl\src\AtomShell\install.jl:3 [inlined]
 [8] uninstall() at C:\projects\blink-jl\src\AtomShell\install.jl:12

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 19, 2018

Are we perhaps trying to delete a folder instead of a file now? https://stackoverflow.com/a/10979105

@pfitzseb
Copy link
Member

We are, but rm′ handles that just fine. I suspect there's still an electron process running when we try to delete the dir. Will look into it.

@pfitzseb
Copy link
Member

pfitzseb commented Aug 19, 2018

Ok, so the issue is that Window() creates three electron subprocesses, of which only one is closed by destroy(::Window) (which isn't that surprising). Force closing the others via quit(window.shell) throws an error in JSON.jl, while killing them with kill(window.shell.proc) throws an error somewhere in HTTP.jl

Imho we need a way to gracefully close a Shell and should do that after the tests run, which will fix the test issue on Windows as well. Unfortunately I know too little about error handling in HTTP.jl (or JSON.jl for that matter) to fix this anytime soon.

@piever
Copy link
Collaborator

piever commented Aug 19, 2018

Unrelated: we can stop checking out master of WebIO and JSExpr as they now have releases that support Julia 0.7

@pfitzseb
Copy link
Member

Oh, and one slightly annoying thing is that HTTP.jl's info messages are printed when creating a new Window:

julia> w = Window()
Window(1, Electron(Process(`'C:\Users\Basti\.julia\dev\Blink\deps\atom\electron.exe' 'C:\Users\Basti\.julia\dev\Blink\src\AtomShell\main.js' port 5403`, ProcessRunning), Sockets.TCPSocket(Base.Libc.WindowsRawSocket(0x00000000000007f0) active, 0 bytes waiting), Dict{String,Any}("callback"=>##1#2())), Page(3, #undef, Dict{String,Any}("callback"=>##1#2()), Distributed.Future(1, 1, 3, nothing)))

julia> [ Info: Accept:  �    0↑     0↓    0s 127.0.0.1:4714:4714 ≣16
[ Info: Accept:  �    0↑     0↓    0s 127.0.0.1:4714:4714 ≣16
[ Info: Accept:  �    0↑     0↓    0s 127.0.0.1:4714:4714 ≣16
[ Info: Accept:  �    0↑     0↓    0s 127.0.0.1:4714:4714 ≣16
[ Info: Accept:  �    0↑     0↓    0s 127.0.0.1:4714:4714 ≣16
[ Info: Closed:  �    1↑     1↓�   4s 127.0.0.1:4714:4714 ≣16

Is there some way to suppress that?

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 20, 2018

Can we wrap the serve() call in Logging.with_logger() and pass the logs to a NullLogger ?

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 20, 2018

Another suggestion from Slack: JuliaLang/julia#28229 (comment)

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 21, 2018

I implemented the suggestion from JuliaLang/julia#28229 (comment) , and it seems to work fine.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@3eff499). Click here to learn what that means.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #143   +/-   ##
=========================================
  Coverage          ?   85.06%           
=========================================
  Files             ?        9           
  Lines             ?      154           
  Branches          ?        0           
=========================================
  Hits              ?      131           
  Misses            ?       23           
  Partials          ?        0
Impacted Files Coverage Δ
src/AtomShell/window.jl 81.25% <ø> (ø)
src/content/api.jl 100% <ø> (ø)
src/content/config.jl 100% <100%> (ø)
src/AtomShell/install.jl 100% <100%> (ø)
src/rpc/rpc.jl 78.57% <100%> (ø)
src/content/content.jl 91.66% <100%> (ø)
src/AtomShell/process.jl 80% <75%> (ø)
src/content/server.jl 72.72% <88.88%> (ø)

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 3eff499...37a5ca0. Read the comment docs.

@rdeits
Copy link
Collaborator Author

rdeits commented Aug 21, 2018

@pfitzseb I don't think the windows uninstall issue is severe enough to block getting an otherwise-working 0.7 version out. Do you agree?

@pfitzseb
Copy link
Member

pfitzseb commented Aug 21, 2018

I implemented the suggestion from JuliaLang/julia#28229 (comment) , and it seems to work fine.

That won't work in Juno or IJulia or in any environment that sets up their own logger. I'd be in favour of setting up a BlinkLogger that discards all Info level (and lower) logs from inside HTTP.jl and pipes everything else through to the former current_logger(). Basically something like this.

Actually, HTTP.jl should probably take a logger argument for listen as well which could be used for this. See JuliaWeb/HTTP.jl#189, basically.

@pfitzseb I don't think the windows uninstall issue is severe enough to block getting an otherwise-working 0.7 version out. Do you agree?

Yeah, I agree. There still is the problem of creating zombie processes (at least on Windows, I didn't check on other OSs yet) that can't be closed without getting an error on the Julia side though.

That doesn't have to hold up this PR. so merge at your leisure. Thanks again for the great work :)

@piever
Copy link
Collaborator

piever commented Aug 21, 2018

Merging this and tagging a new release, even if it's not perfect, is very helpful to update the rest of the stack (I need Blink to get Knockout to pass tests on 0.7 and 1.0).

@pfitzseb pfitzseb merged commit f48c6ac into master Aug 21, 2018
@pfitzseb
Copy link
Member

Alright, done.

(Wanted to let @rdeits do the honours of merging, but I guess getting a release out quickly is a good idea)

@NHDaly
Copy link
Collaborator

NHDaly commented Aug 21, 2018

This was a stellar effort! Thank you all! I'm sorry i didn't help more in the process. Great job and thanks! :)

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.

Blink not building on Julia 1.0 Move to Julia 0.7
7 participants