Skip to content

src: include uv.h in node_binding header#31265

Closed
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:fix-uv-include
Closed

src: include uv.h in node_binding header#31265
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:fix-uv-include

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Jan 8, 2020 •
edited
Loading

This was removed in 3bb085d but is needed by Electron on Windows, as otherwise compilation errors will occur as a result of unknown libuv types:

In file included from ../../third_party/electron_node/src/node_binding.cc:1:
../../third_party/electron_node/src/node_binding.h(76,3): error: unknown type name 'uv_lib_t'
  uv_lib_t lib_;
  ^

See https://ci.appveyor.com/project/electron-bot/electron-ljo26/builds/29979721.

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

@codebytere codebytere requested a review from addaleax January 8, 2020 23:00
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 8, 2020
@codebytere
src: include uv.h in node_binding header
4d8ffe2 
This was removed in nodejs@3bb085d
but is needed by Electron on Windows, as otherwise compilation errors
will occur as a result of unknown libuv types.
@gengjiawen
Copy link
Member

gengjiawen commented Jan 9, 2020

Maybe add some comment to prevent from remove again ?

addaleax
addaleax approved these changes Jan 9, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM although, just to be cٓlear, including this header is not officially supported anyway

bnoordhuis
bnoordhuis approved these changes Jan 9, 2020
View reviewed changes
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, and frankly, I don't understand why our Windows build aren't failing... There's nothing that pulls in uv_lib_t, as far as I can determine.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 9, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2020

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

codebytere added a commit that referenced this pull request Jan 10, 2020
@codebytere
src: include uv.h in node_binding header
a3d8de9 
This was removed in 3bb085d
but is needed by Electron on Windows, as otherwise compilation errors
will occur as a result of unknown libuv types.

PR-URL: #31265
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere
Copy link
Member Author

codebytere commented Jan 10, 2020

Landed in a3d8de9

@codebytere codebytere closed this Jan 10, 2020
@codebytere codebytere deleted the fix-uv-include branch January 10, 2020 23:04
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@codebytere @MylesBorins
src: include uv.h in node_binding header
06fbc03 
This was removed in 3bb085d
but is needed by Electron on Windows, as otherwise compilation errors
will occur as a result of unknown libuv types.

PR-URL: #31265
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Merged
codebytere added a commit that referenced this pull request Mar 14, 2020
@codebytere
src: include uv.h in node_binding header
689cdc7 
This was removed in 3bb085d
but is needed by Electron on Windows, as otherwise compilation errors
will occur as a result of unknown libuv types.

PR-URL: #31265
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Mar 17, 2020
@codebytere
src: include uv.h in node_binding header
21bcc96 
This was removed in 3bb085d
but is needed by Electron on Windows, as otherwise compilation errors
will occur as a result of unknown libuv types.

PR-URL: #31265
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere added a commit to electron/electron that referenced this pull request Apr 13, 2020
@codebytere
Remove uv header patch
cc88fe8 
nodejs/node#31265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

+1 more reviewer

@devnexen devnexen devnexen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@codebytere @gengjiawen @nodejs-github-bot @bnoordhuis @jasnell @Trott @addaleax @cjihrig @devnexen @BridgeAR