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

src: define getpid() based on OS #4146

Merged
merged 0 commits into from
Dec 4, 2015
Merged

src: define getpid() based on OS #4146

merged 0 commits into from
Dec 4, 2015

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 4, 2015

94b9948 added unistd.h to src/env.cc in order to use getpid(). However, this doesn't exist on Windows. This commit conditionally defines getpid() based on the OS.

Closes #4145
R=@mscdex

@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2015

I believe we can forgo including process.h on Windows and just alias getpid to GetCurrentProcessId instead, which shouldn't require any additional includes.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

I think you're right. Updating.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

Updated.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/919/

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

Only CI failures are known flakey tests.

@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2015

LGTM

@cjihrig cjihrig closed this Dec 4, 2015
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 4, 2015
@cjihrig
src: define getpid() based on OS
a9c0c65 
94b9948 added unistd.h to src/env.cc in order to use getpid().
However, this doesn't exist on Windows. This commit conditionally
defines getpid() based on the OS.

Fixes: nodejs#4145
PR-URL: nodejs#4146
Reviewed-By: Brian White <mscdex@mscdex.net>
@cjihrig cjihrig deleted the 4145 branch December 4, 2015 02:25
@cjihrig cjihrig merged commit a9c0c65 into nodejs:master Dec 4, 2015
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2015

Thanks for the review. Landing this more quickly than normal since the Windows CI was borked.

@rvagg
Copy link
Member

rvagg commented Dec 5, 2015

commits missing again, perhaps this is what github is doing now if you don't update your PR with the latest version of the commits and delete the branch?

cjihrig added a commit that referenced this pull request Dec 5, 2015
@cjihrig @rvagg
src: define getpid() based on OS
458facd 
94b9948 added unistd.h to src/env.cc in order to use getpid().
However, this doesn't exist on Windows. This commit conditionally
defines getpid() based on the OS.

Fixes: #4145
PR-URL: #4146
Reviewed-By: Brian White <mscdex@mscdex.net>
@bnoordhuis bnoordhuis mentioned this pull request Dec 5, 2015
Merged
This was referenced Dec 8, 2015
Closed
Merged
@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
@jasnell jasnell added os Issues and PRs related to the os subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. lts-watch-v4.x and removed os Issues and PRs related to the os subsystem. labels Dec 17, 2015
@MylesBorins
Copy link
Contributor

94b9948 was semver major so I do not believe this patch should be backported. @jasnell feel free to modify.

@MylesBorins MylesBorins mentioned this pull request Jan 14, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@cjihrig
src: define getpid() based on OS
8018a25 
94b9948 added unistd.h to src/env.cc in order to use getpid().
However, this doesn't exist on Windows. This commit conditionally
defines getpid() based on the OS.

Fixes: nodejs#4145
PR-URL: nodejs#4146
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjihrig @mscdex @rvagg @MylesBorins @jasnell