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.

Sign up for GitHub

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

Jump to bottom

[Distributed] Set stdin to devnull before closing it #44881

Merged
merged 2 commits into from
Apr 12, 2022
Merged

[Distributed] Set stdin to devnull before closing it #44881

merged 2 commits into from
Apr 12, 2022

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 6, 2022

Distributed closes and destroys stdin, but some tests attempted to
explicitly use it, leading to test problems. We previously interpreted
this as passing devnull, but this is better to be explicit.

@DilumAluthge @staticfloat

@vtjnash vtjnash mentioned this pull request Apr 6, 2022
Merged
staticfloat
staticfloat approved these changes Apr 6, 2022
View reviewed changes
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

Looks good to me, let's see how CI does!

vtjnash
vtjnash commented Apr 7, 2022
View reviewed changes
stdlib/REPL/src/TerminalMenus/util.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/TerminalMenus/util.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/TerminalMenus/util.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash force-pushed the jn/bad-stdin-tests branch from 0b2eb3c to 340587f Compare April 7, 2022 17:32
@staticfloat
Copy link
Member

Buildkite CI looks good to me; x86_64-apple-darwin and x86_64-linux-gnu both passed.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 7, 2022

Do I need to revert any other PRs to properly test this now?

@staticfloat
Copy link
Member

Just the REPL and Pkg, which you've done just now.

@DilumAluthge DilumAluthge added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Apr 7, 2022
@vtjnash
Copy link
Member Author

vtjnash commented Apr 8, 2022

I don't know why buildbot is broken, but most of buildkite looked good and just the common crashes in Pkg and LibGit2 tests on musl?

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 8, 2022
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 8, 2022
@DilumAluthge
Copy link
Member

DilumAluthge commented Apr 8, 2022 •
edited
Loading

@vtjnash There are some test failures in the REPL test set on x86_64-apple-darwin on Buildkite.

https://buildkite.com/julialang/julia-master/builds/10862#cffe05fa-9ecb-4eb3-8255-0987bc738687

Error in testset REPL:
Test Failed at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-macmini-x64-5.0/build/default-macmini-x64-5-0/julialang/julia-master/julia-6552c91511/share/julia/stdlib/v1.9/REPL/test/repl.jl:285
  Expression: s == "\e[0m\n"
   Evaluated: "\r\r\r\e[0K\e[1A\r\e[0K\e[1A\r\e[0K\e[1A\r\e[0K\e[1A\r\e[0K\e[1A\r\e[0K\e[1A\r\e[0K\e[31m\e[1mshell> \e[0m\e[0m\r\e[7C/Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-macmini-x64-5.0/build/default-macmini-x64-5-0/julialang/julia-master/julia-6552c91511/bin/julia -Cnative -J/Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-macmini-x64-5.0/build/default-macmini-x64-5-0/julialang/julia-master/julia-6552c91511/lib/julia/sys.dylib --depwarn=error --check-bounds=yes -g1 --startup-file=no -e \"println(\\\"HI\\\")\" \n" == "\e[0m\n"
Error in testset REPL:
Test Failed at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-macmini-x64-5.0/build/default-macmini-x64-5-0/julialang/julia-master/julia-6552c91511/share/julia/stdlib/v1.9/REPL/test/repl.jl:290
  Expression: fetch(get_stdout) == "HI\n"
   Evaluated: "" == "HI\n"

@vtjnash
Copy link
Member Author

vtjnash commented Apr 8, 2022

The rest passed, so I assume it is a flaky test with writing too much or too long of a path or something

@DilumAluthge
Copy link
Member

I've restarted it; let's see if it passes the second time.

@DilumAluthge DilumAluthge requested a review from staticfloat April 8, 2022 03:37
@DilumAluthge
Copy link
Member

@staticfloat The Buildbot tester_macos64 is also failing, but the log does not seem to explain why.

@DilumAluthge
Copy link
Member

@vtjnash I get the same REPL failures when I rerun the x86_64-apple-darwin job, so I don't think it's a flaky test.

https://buildkite.com/julialang/julia-master/builds/10862#c5ccc5ab-aef0-43d2-a275-b8582bfaf614

@vtjnash vtjnash force-pushed the jn/bad-stdin-tests branch from 6552c91 to 2a07bcf Compare April 11, 2022 17:07
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 11, 2022
vtjnash added 2 commits April 11, 2022 21:59
@vtjnash @DilumAluthge
[Distributed] Set stdin to devnull before closing it
64a86f9 
Distributed closes and destroys stdin, but some tests attempted to
explicitly use it, leading to test problems. We previously interpreted
this as passing devnull, but this is better to be explicit.
@vtjnash @DilumAluthge
Revert "Testsystem: for now, move the REPL tests to node 1 (#44880)"
e4accde 
This reverts commit 7401e92.
@DilumAluthge DilumAluthge merged commit 2ae677d into master Apr 12, 2022
@DilumAluthge DilumAluthge deleted the jn/bad-stdin-tests branch April 12, 2022 05:11
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 12, 2022
@DilumAluthge
Copy link
Member

DilumAluthge commented Apr 12, 2022 •
edited
Loading

Both Pkg and REPL passed on the x86_64-linux-gnu tester.

(Note: we skip Pkg when running under rr, so you'll need to look at the non-rr job.)

DilumAluthge added a commit that referenced this pull request Apr 13, 2022
@DilumAluthge
Revert " [Distributed] Set stdin to devnull before closing it (#44881)"
745530d 
This reverts commit 2ae677d.
@DilumAluthge DilumAluthge mentioned this pull request Apr 13, 2022
Closed
@IanButterworth IanButterworth added the backport 1.8 Change should be backported to release-1.8 label May 24, 2022
@IanButterworth IanButterworth mentioned this pull request May 24, 2022
Merged
45 tasks
@IanButterworth IanButterworth removed the backport 1.8 Change should be backported to release-1.8 label May 24, 2022
@IanButterworth IanButterworth mentioned this pull request May 24, 2022
Merged
67 tasks
Keno pushed a commit that referenced this pull request Jun 5, 2024
@vtjnash
[Distributed] Set stdin to devnull before closing it (#44881)
e5e570b 
* [Distributed] Set stdin to devnull before closing it

Distributed closes and destroys stdin, but some tests attempted to
explicitly use it, leading to test problems. We previously interpreted
this as passing devnull, but this is better to be explicit.

* Revert "Testsystem: for now, move the REPL tests to node 1 (#44880)"

This reverts commit 7401e92.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticfloat staticfloat staticfloat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vtjnash @staticfloat @DilumAluthge @IanButterworth