src: remove util-inl.h include in node.h by addaleax · Pull Request #27804 · nodejs/node · GitHub
Skip to content

src: remove util-inl.h include in node.h#27804

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:addons-fix
Closed

src: remove util-inl.h include in node.h#27804
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:addons-fix

Conversation

@addaleax

@addaleax addaleax commented May 21, 2019

Copy link
Copy Markdown
Member

node.h may only include public APIs, which util-inl.h is not.
There does not seem to be any reason for including it, so remove it,
because otherwise native addon compilation is broken due to us not
shipping the util-inl.h header.

Refs: #27631
Fixes: #27803

@BridgeAR It would be awesome if you could push out 12.3.1 with that soon, it completely breaks non-N-API addon compilation.

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

@addaleax
src: remove util-inl.h include in node.h
3853b0c 
`node.h` may only include public APIs, which `util-inl.h` is not.
There does not seem to be any reason for including it, so remove it,
because otherwise native addon compilation is broken due to us not
shipping the `util-inl.h` header.

Refs: nodejs#27631
Fixes: nodejs#27803
@nodejs-github-bot

nodejs-github-bot commented May 21, 2019

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 21, 2019
@addaleax addaleax added the fast-track PRs that do not need to wait for 72 hours to land. label May 21, 2019
@addaleax

addaleax commented May 21, 2019

Copy link
Copy Markdown
Member Author

Please 👍 this comment to approve fast-tracking.

@nodejs-github-bot

nodejs-github-bot commented May 21, 2019

Copy link
Copy Markdown
Collaborator

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

@targos

targos commented May 21, 2019

Copy link
Copy Markdown
Member

How is it that CITGM couldn't catch this?

@BridgeAR

BridgeAR commented May 21, 2019

Copy link
Copy Markdown
Member

@addaleax I'll try to put together a patch release tomorrow.

@addaleax

addaleax commented May 21, 2019

Copy link
Copy Markdown
Member Author

@targos I think CITGM uses --nodedir? That points node-gyp to a full Node.js source tree, so all headers are present there. This definitely isn’t the first time we’ve had this kind of issue. It’s not really trivial to solve, as far as I can tell, mostly because node-gyp is trying to be Helpful™ and refuses to build against non-release versions without --nodedir and/or requires a tarball even when all relevant headers are installed globally.

@targos

targos commented May 21, 2019

Copy link
Copy Markdown
Member

@addaleax could we for example, from citgm, copy the public headers to a temporary directory and point --nodedir to it?

@addaleax

addaleax commented May 21, 2019

Copy link
Copy Markdown
Member Author

@targos Yeah, I assume there is some way to make something like that work…

@addaleax

addaleax commented May 21, 2019

Copy link
Copy Markdown
Member Author

Landed in 47c5c3d

@addaleax addaleax closed this May 21, 2019
@addaleax addaleax deleted the addons-fix branch May 21, 2019 21:31
addaleax added a commit that referenced this pull request May 21, 2019
@addaleax
src: remove util-inl.h include in node.h
47c5c3d 
`node.h` may only include public APIs, which `util-inl.h` is not.
There does not seem to be any reason for including it, so remove it,
because otherwise native addon compilation is broken due to us not
shipping the `util-inl.h` header.

Refs: #27631
Fixes: #27803

PR-URL: #27804
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR

BridgeAR commented May 21, 2019

Copy link
Copy Markdown
Member

Thinking about this further: would it maybe be possible to do something like that directly in each CI run instead of in our CITGM runs?

@richardlau

richardlau commented May 21, 2019

Copy link
Copy Markdown
Member

Yes, it's possible. There was #12231 which stalled.

@msimerson msimerson mentioned this pull request May 21, 2019
Merged
@cjihrig cjihrig mentioned this pull request May 22, 2019
Closed
@jacobq jacobq mentioned this pull request May 22, 2019
Closed
@sam-github

sam-github commented May 22, 2019

Copy link
Copy Markdown
Contributor

BridgeAR pushed a commit that referenced this pull request May 22, 2019
@addaleax @BridgeAR
src: remove util-inl.h include in node.h
7438a55 
`node.h` may only include public APIs, which `util-inl.h` is not.
There does not seem to be any reason for including it, so remove it,
because otherwise native addon compilation is broken due to us not
shipping the `util-inl.h` header.

Refs: #27631
Fixes: #27803

PR-URL: #27804
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR mentioned this pull request May 22, 2019
Merged
BridgeAR added a commit that referenced this pull request May 22, 2019
@BridgeAR
2019-05-22, Version v12.3.1 (Current)
2817fde 
Notable changes

* deps:
  * Fix handling of +0/-0 when constant field tracking is enabled
    (Michaël Zasso) #27792
  * Fix `os.freemem()` and `os.totalmem` correctness (cjihrig)
    #27718
* src:
  * Fix v12.3.0 regression that prevents native addons from compiling
    #27804

PR-URL: #27814
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 22, 2019
@BridgeAR
2019-05-22, Version v12.3.1 (Current)
8a282dc 
Notable changes

* deps:
  * Fix handling of +0/-0 when constant field tracking is enabled
    (Michaël Zasso) nodejs#27792
  * Fix `os.freemem()` and `os.totalmem` correctness (cjihrig)
    nodejs#27718
* src:
  * Fix v12.3.0 regression that prevents native addons from compiling
    nodejs#27804

PR-URL: nodejs#27814
@bnoordhuis bnoordhuis mentioned this pull request May 22, 2019
Closed
@Martii Martii mentioned this pull request May 24, 2019
Merged
@abretaud abretaud mentioned this pull request Jul 30, 2019
Closed
@mistersandman mistersandman mentioned this pull request Dec 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig approved these changes
@richardlau richardlau richardlau approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+2 more reviewers
@sam-github sam-github sam-github approved these changes
@Fishrock123 Fishrock123 Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 72 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

util-inl.h missing

8 participants

@addaleax @nodejs-github-bot @targos @BridgeAR @richardlau @sam-github @Fishrock123 @cjihrig