FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: define getpid() based on OS by cjihrig · Pull Request #4146 · nodejs/node · GitHub

/ node Public

src: define getpid() based on OS - #4146

Merged
cjihrig merged 0 commit into
nodejs:masterfrom
cjihrig:4145
Dec 4, 2015
ConversationCommits (0)ChecksFiles changed
Merged

src: define getpid() based on OS#4146
cjihrig merged 0 commit into
nodejs:masterfrom
cjihrig:4145

Conversation

cjihrig commented Dec 4, 2015

Copy link
Copy Markdown
Contributor

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 commented Dec 4, 2015

Copy link
Copy Markdown
Contributor

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 commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

I think you're right. Updating.

cjihrig commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

Updated.

cjihrig commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

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

cjihrig commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

Only CI failures are known flakey tests.

mscdex commented Dec 4, 2015

Copy link
Copy Markdown
Contributor

LGTM

cjihrig closed this Dec 4, 2015
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 4, 2015
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 deleted the 4145 branch December 4, 2015 02:25
cjihrig merged commit a9c0c65 into nodejs:master Dec 4, 2015

cjihrig commented Dec 4, 2015

Copy link
Copy Markdown
Contributor Author

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

rvagg commented Dec 5, 2015

Copy link
Copy Markdown
Member

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
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 mentioned this pull request Dec 5, 2015
Merged
This was referenced Dec 8, 2015
Closed
Merged
rvagg mentioned this pull request Dec 17, 2015
Closed
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 commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

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

MylesBorins mentioned this pull request Jan 14, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL