Fix Process.run uses in compiler to handle exec failure #9893
Conversation
straight-shoota
added
kind:bug
|
This could also be merged directly instead of #9867 |
|
@straight-shoota I think we can go with this PR as is instead of #9867, but I would need you to remove the Draft status. |
straight-shoota
changed the title
|
This PR never passed Windows CI, and that was not a fluke. https://github.com/crystal-lang/crystal/actions?query=workflow%3A%22Windows+CI%22+branch%3Amaster
We need quick action; unfortunately Windows CI is broken in three different way on master right now. |
|
I can merge #9912 right away. After that the specs of this PR can be worked. Does that sound good? |
|
@oprypin Sorry, I didn't realize the failure was an acutal bug in this PR. Let's make CI reliable again =) |
| assert_with_defaults(ProjectInfo.new("bar", "2.0"), ProjectInfo.new("bar", "2.0", refname: nil)) | ||
| assert_with_defaults(ProjectInfo.new(nil, "2.0"), ProjectInfo.new("foo", "2.0", refname: nil)) | ||
| ensure | ||
| Crystal::Git.executable = "git" |
There was a problem hiding this comment.
I confirm that the specs get stuck immediately after exiting this spec. But I can't figure out where. Both the around_each and before_each above also finish, and then somehow spec gets stuck, before entering the next spec.
There was a problem hiding this comment.
We should probably just add a quick fix to disable this spec on win32 to get CI working.
There was a problem hiding this comment.
I think I understand now. Of course, the bug in entirely in Crystal's support of Windows.
This leaves a dangling fiber that's still trying to pipe output of the non-launched process. While background fibers getting permanently stuck is a known (horrible) bug, perhaps there's another bug that forgets to close something if the process never actually launched.
Repro in cmd:
crystal eval "p! Process.run(%(missing), output: IO::Memory.new) rescue nil; p! Process.run(%(git), output: IO::Memory.new)"There was a problem hiding this comment.
@straight-shoota Yes, and looks like that'll even be the somewhat long-term fix. #9913
Fixes more uses of
Process.runto handle exec error (see #9867 and #9789). Also refactors the git code used in ProjectInfo to a common location for reusability.See #9867 (comment)
Depends on #9867includes #9867Fixes #9789