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

os: implement os.release() using uv_os_uname() #25600

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 21, 2019

Ignore the first commit, which is the yet to be merged libuv 1.25.0 update.

For non-Windows platforms, the happy path behavior should be identical. On Windows, uv_os_uname() attempts to use RtlGetVersion() before falling back to the deprecated GetVersionExW() that Node was previously using. This could lead to some Windows users seeing a different value for os.release() after this change. Technically, this is a bug fix, but I'd also understand if some collaborators are in favor of marking this semver-major.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Jan 21, 2019
@bnoordhuis bnoordhuis added os Issues and PRs related to the os subsystem. and removed libuv Issues and PRs related to the libuv dependency or the uv binding. labels Jan 21, 2019
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. No strong opinion on whether it should be semver-major but it doesn't seem like a big deal to me.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 21, 2019

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2019
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

PR-URL: nodejs#25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@cjihrig cjihrig merged commit 914af23 into nodejs:master Jan 23, 2019
@cjihrig cjihrig deleted the os-release branch January 23, 2019 05:17
targos pushed a commit that referenced this pull request Jan 24, 2019
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

PR-URL: #25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2019

I've landed this on v10.x-staging. Please lmk if it should be backed out before the semver minor release

edit: nvm mind. including in libuv update

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request May 16, 2019
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

PR-URL: nodejs#25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
For non-Windows platforms, the happy path behavior should be
identical. On Windows, uv_os_uname() attempts to use
RtlGetVersion() before falling back to the deprecated
GetVersionExW() that Node was previously using.

Backport-PR-URL: #27728
PR-URL: #25600
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants