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

Don't close already closed pipes #81

Merged
merged 2 commits into from
Dec 13, 2016
Merged

Don't close already closed pipes #81

merged 2 commits into from
Dec 13, 2016

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Dec 12, 2016

a9a8e91 added some close calls which attempted to close pipes that we already closed previously. This resulted in the error code we read from the failed child being overwritten, resulting in the process004 test failing.

a9a8e91 added some `close` calls which
attempted to close pipes that we already closed previously. This resulted in the
error code we read from the failed child being overwritten, resulting in
the `process004` test failing.
@hvr
Copy link
Member

hvr commented Dec 12, 2016

Btw, the changelog entry for 1.4.3 didn't even mention the code change resulting in this regression: http://hackage.haskell.org/package/process-1.4.3.0/changelog

Yuras referenced this pull request Dec 13, 2016
Make sure stdin, stdout and stderr pipes are closed when exec call
fails, e.g. when the executable doesn't exist.
@snoyberg
Copy link
Collaborator

Thanks @bgamari. Is it reasonable to hold off on a release to Hackage until you've tested this in GHC?

As a side note, it would be really nice to configure Travis/AppVeyor to run the GHC test cases in this repo, I'm just not sure of how to even go about that.

@snoyberg snoyberg merged commit a71d831 into haskell:master Dec 13, 2016
snoyberg added a commit that referenced this pull request Dec 13, 2016
@bgamari
Copy link
Contributor Author

bgamari commented Dec 13, 2016

Thanks @bgamari. Is it reasonable to hold off on a release to Hackage until you've tested this in GHC?

It passes validation.

As a side note, it would be really nice to configure Travis/AppVeyor to run the GHC test cases in this repo, I'm just not sure of how to even go about that.

It would be nice if we could make that happen but I to am rather unsure of how to go about it. I can think of a few options,

  • Pulling GHC's testsuite driver into process (yuck)
  • Teaching the CI scripts to clone a GHC tree and invoke GHC's testsuite driver with HC=`which ghc`
  • Coming up with a minimal test driver which is loosely compatible with GHC's for inclusion with process

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