Skip to content

Implement getCurrentPid #205

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

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Conversation

yutotakano
Copy link
Contributor

Change

This PR adds a new exported function in System.Process: getCurrentPid.

This calls getCurrentProcessId from System.Win32.Process on Windows, and getProcessID from System.Posix.Process on POSIX systems.

Rationale

This seems like an oversight considering getPid is implemented already. I sometimes require the current running program's process ID. Currently, I have to do the CPP conditional logic myself every time, which is a pain. Putting this in process seems natural.

Furthermore, my understanding of process is that it is a unified interface to System.Win32.Process and System.Posix.Process. As both modules provide a function to "get the currently executing process id", I feel it makes sense to have a cross-platform wrapper function.

Remaining Issues

The @since field has been left as a TODO since I'm not sure what version this will be merged in.

Tests

I could not come up with a method to test the PR. Unlike with the test for getPid, there's nothing to compare the value against, I think. It's worked from my past experiences in personal projects.

I wasn't able to ensure a stable setup to test to begin with, as running the existing test suite stack test on the Windows version of Stack failed due to several reasons (getPid failing, and detatch_console failing). Any feedback or insight is therefore appreciated.

@snoyberg
Copy link
Collaborator

Thanks for the PR and the catch about the unstable stack test. I've pushed some commits to move over to GitHub Actions and temporarily disable the broken tests. Can you rebase against master?

@yutotakano yutotakano force-pushed the implement-getcurrentpid branch from e36cbc8 to dcaa85c Compare June 28, 2021 07:24
@yutotakano
Copy link
Contributor Author

I've confirmed the remaining tests now pass perfectly on both my Windows and Linux Haskell setups. I've also rebased and force-pushed the changes. Thanks for the quick reply!

@yutotakano
Copy link
Contributor Author

Huh, didn't mean to tag the user, sorry SInCE!

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Thank you!

@snoyberg snoyberg merged commit 882ea79 into haskell:master Jun 28, 2021
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