Skip to content

Conversation

@sternmull
Copy link
Contributor

This implements my request #108

Tested on Linux and Windows. For Linux i used "stack test". For windows i did a seperate manual test (compared the PID with the info from the Task-Manger). The test/main.hs does not look like it could be run on Windows (for example echo is a builtin of cmd.exe and not a program on its own).

On Windows the PID stays valid as long as the handle is open (see https://blogs.msdn.microsoft.com/oldnewthing/20110107-00/?p=11803).

@snoyberg
Copy link
Collaborator

It looks like the echo bit ran fine on Windows, but for some reason the terminateProcess call (implicit in withCreateProcess) failed: https://ci.appveyor.com/project/snoyberg/process/build/1.0.186.

I'm also not sure of the motivation behind using an Integer value, instead of a CPid or similar.

Finally: there's a build failure with GHC 7.2.2: https://travis-ci.org/haskell/process/jobs/302093304.

@sternmull sternmull force-pushed the master branch 2 times, most recently from 55f7fc6 to 37a18af Compare November 15, 2017 16:37
@sternmull
Copy link
Contributor Author

The problem of withCreateProcess on Windows is unrelated to my changes. I opened #110 for this. The test for getPid now no longer uses withCreateProcess for this reason.

I chose Integer as return type as a lazy solution to have a type that can represent the PID on all systems (Int32 or possibly larger on POSIX, DWORD on Windows.. which is an unsigned 32bit integer). I knew it was not the optimal solution and did it anyway. I resolved that by adding a Pid type to System.Process that is an alias for the native type that represents the PID.

I saw that your tests run on msys. That explains why i had no success in plain cmd.exe. With a minor adjustment the test now passes on all systems (OSX builds are still pending... but i am confident).

@snoyberg
Copy link
Collaborator

Thanks for the update and the other bug report. One final request: can you update the changelog and add a @since comment to the Haddock?

@simonmar Would you mind having a quick look at this one and making sure I'm not missing some reason why this function would be problematic in the public API?

@sternmull
Copy link
Contributor Author

Updated the comments and changelog.

@snoyberg snoyberg merged commit bc9cac2 into haskell:master Nov 26, 2017
@snoyberg
Copy link
Collaborator

Thanks!

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.

2 participants