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

Update Pkg, and move tests to node 1 #44828

Merged
merged 2 commits into from
Apr 5, 2022
Merged

Update Pkg, and move tests to node 1 #44828

merged 2 commits into from
Apr 5, 2022

Conversation

staticfloat
Copy link
Member

This updates Pkg and moves the Pkg tests to node 1, to fix breakage due to Pkg using redirect_stdin() in its tests, but being run in a distributed runner that has no stdin.

@staticfloat
Copy link
Member Author

Looks like this is working!

@DilumAluthge
Copy link
Member

I know I originally suggested this, but I'm concerned that it will nontrivially increase the duration of our tests (because all of the "move to node 1" test sets have to run in serial).

What is the specific challenge with redirect_stdin on the workers? Is there another way that we can work around it? Can we detect whether or not the worker has stdin, and then modify the behavior of redirect_stdin accordingly?

@DilumAluthge
Copy link
Member

We literally only use redirect_stdin for like two or three tests in Pkg.

Can we instead start a subprocess, and run the tests in that subprocess?

@staticfloat
Copy link
Member Author

@vtjnash what do you suggest here?

@vtjnash
Copy link
Member

vtjnash commented Apr 1, 2022

The stdin handle should have been set to devnull, except for that bug in the Pkg tests that forced us to set it to an invalid object for the Pkg tests. We can probably set it up correctly now.

@DilumAluthge DilumAluthge marked this pull request as draft April 1, 2022 20:06
@staticfloat
Copy link
Member Author

Feel free to push what "setting it up correctly" looks like to this branch, and we can test it out.

@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2022

diff --git a/stdlib/Distributed/src/cluster.jl b/stdlib/Distributed/src/cluster.jl
index 4bfc3a0bf8..37f1660e19 100644
--- a/stdlib/Distributed/src/cluster.jl
+++ b/stdlib/Distributed/src/cluster.jl
@@ -232,7 +232,10 @@ start_worker(cookie::AbstractString=readline(stdin); kwargs...) = start_worker(s
 function start_worker(out::IO, cookie::AbstractString=readline(stdin); close_stdin::Bool=true, stderr_to_stdout::Bool=true)
     init_multi()
 
-    close_stdin && close(stdin) # workers will not use it
+    if close_stdin # workers will not use it
+        redirect_stdin(devnull)
+        close(stdin)
+    end
     stderr_to_stdout && redirect_stderr(stdout)
 
     init_worker(cookie)

staticfloat added a commit that referenced this pull request Apr 4, 2022
This should fix the issues seen when bumping `Pkg` in
#44828
staticfloat added a commit that referenced this pull request Apr 4, 2022
This should fix the issues seen when bumping `Pkg` in
#44828
@DilumAluthge
Copy link
Member

Could we separate the two changes (fix Distributed, and bump Pkg) into two separate PRs? That way, we can backport just the "fix Distributed" PR to earlier Julia versions.

@staticfloat
Copy link
Member Author

@vtjnash we now have REPL failing because it reads from term.in_stream. I tried replacing that with a Pipe, but I can't figure out how to initialize a pipe and write to it. Do you think we should move the REPL tests to node 1 so it can directly mess with terminals, or should we figure out how to create a fake pipe so that we can feed keystrokes to it?

staticfloat added a commit that referenced this pull request Apr 4, 2022
This should fix the issues seen when bumping `Pkg` in
#44828
@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2022

Ah, it is the TerminalMenus tests. Those look like they have the same bug

write(stdin.buffer, keydict[key])

Past encounters with this bug (and this patch) were at https://github.com/JuliaLang/julia/compare/jn/io-handle-life, and apparently wasn't in a PR right now.

@staticfloat
Copy link
Member Author

I'm not sure your branch is much better; it seems to just avoid closing stdin for the REPL tests. Is that the option you want to pursue?

Also, it appears the Distributed tests are also broken because of this test. I guess we broke an assumption here by having no stdin in the parent process, so the child process inherits a closed stdin?

@staticfloat
Copy link
Member Author

I'm giving up on this for now, as it's got too many moving pieces that I don't know how to fix. I'll just move Pkg to node 1, and someone more familiar with all the pipe stuff can fix it later.

@staticfloat staticfloat force-pushed the sf/macos_pkg_fixes branch 2 times, most recently from f03dcb2 to 58ffb0c Compare April 5, 2022 14:16
@staticfloat staticfloat marked this pull request as ready for review April 5, 2022 16:01
@staticfloat staticfloat merged commit 3fb132f into master Apr 5, 2022
@staticfloat staticfloat deleted the sf/macos_pkg_fixes branch April 5, 2022 16:02
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.

3 participants