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

implement Base.redirect_{stdout,stderr,stdin} #641

Merged
merged 3 commits into from
Mar 24, 2018

Conversation

rdeits
Copy link
Contributor

@rdeits rdeits commented Mar 15, 2018

Fixes #640. Please let me know if this isn't the appropriate solution to the problem.

@@ -100,13 +100,13 @@ function init(args)
start_heartbeat(heartbeat[])
if capture_stdout
read_stdout[], = redirect_stdout()
eval(Base, :(STDOUT = $(IJuliaStdio(STDOUT,"stdout"))))
redirect_stdout(IJuliaStdio(STDOUT,"stdout"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are calling redirect_stdout twice...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, you're calling the overloaded method below...

@JobJob
Copy link
Contributor

JobJob commented Mar 21, 2018

Datapoint: this fixes TraceCalls in IJulia (cstjean/TraceCalls.jl#60) which makes me happy.

src/stdio.jl Outdated
@@ -24,6 +24,9 @@ if VERSION >= v"0.7.0-DEV.1472" # Julia PR #23271
Base.unwrapcontext(io::IJuliaStdio) = Base.unwrapcontext(io.io)
end
Base.setup_stdio(io::IJuliaStdio, readable::Bool) = Base.setup_stdio(io.io.io, readable)
Base.redirect_stdout(io::IJuliaStdio) = (eval(Base, :(STDOUT = $io)); io)
Base.redirect_stderr(io::IJuliaStdio) = (eval(Base, :(STDERR = $io)); io)
Base.redirect_stdin(io::IJuliaStdio) = (eval(Base, :(STDIN = $io)); io)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right to me. It's not returning the documented (rd, wr) pair...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation seems to be wrong in this case. Calling redirect_stdout(STDERR), for example, just returns STDERR. See: https://github.com/JuliaLang/julia/blob/v0.6.2/base/stream.jl#L1031

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@stevengj stevengj Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's only called for restoring the original STDOUT stream in redirect_*. In that case, however, probably check that this is the case. Something like:

for s in ("stdout", "stderr", "stdin")
    f = Symbol("redirect_", s)
    S = QuoteNode(Symbol(uppercase(s)))
    @eval function Base.$f(io::IJuliaStdio)
        io[:jupyter_stream] != $s && error("expecting ", $s, " stream)
        eval(Base, Expr(:=, $S, :io))
        return io, io
    end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can do that. It does seem a bit odd for the return type to be inconsistent with the Base implementation of redirect_stdout(STDOUT), though, doesn't it?

Copy link
Member

@stevengj stevengj Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't notice that redirect_stdout(STDOUT) returned only a single stream. That seems like a bug in base, since redirect_stdout is documented to return two streams. Ah, now I see that you filed an issue for that.

But sure, in that case do return io rather than return io, io. But we should still check that it is restoring the correct stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done

@stevengj stevengj merged commit 2ef4873 into JuliaLang:master Mar 24, 2018
@chethega
Copy link

Datapoint: Before this patch, I got error #640 on the following in jupyter. After this patch, the julia kernel hangs somewhere (no CPU consumption; still reacts to "restart kernel", but does not react to "interrupt kernel"). On the REPL, this works like expected.

using Suppressor
output = @capture_out begin
    println("should get captured, not printed")
end;
@show output;

@stevengj stevengj mentioned this pull request Apr 19, 2018
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