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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@addaleax
Copy link
Member

commented May 21, 2019

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
src: remove util-inl.h include in node.h
`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
@sam-github

This comment has been minimized.

Copy link

commented on 3853b0c May 21, 2019

I included it because some of our cctest code appeared to me to assume it was present, so I thought it was part of our public API, but I was clearly mistaken. This fix LGTM.

@nodejs-github-bot

This comment has been minimized.

@addaleax addaleax added the fast-track label May 21, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Please 👍 this comment to approve fast-tracking.

@nodejs-github-bot

This comment has been minimized.

@targos

This comment has been minimized.

Copy link
Member

commented May 21, 2019

How is it that CITGM couldn't catch this?

@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

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

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Landed in 47c5c3d

@addaleax addaleax closed this May 21, 2019

@addaleax addaleax deleted the addaleax:addons-fix branch May 21, 2019

addaleax added a commit that referenced this pull request May 21, 2019

src: remove util-inl.h include in node.h
`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

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

This comment has been minimized.

Copy link
Member

commented May 21, 2019

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

@sam-github

This comment has been minimized.

Copy link
Member

commented May 22, 2019

It would be good to get #12231 going again. I added the broken includes because some test/cctest code was failing to build, code that included only (I thought at the time) "node.h", leading me to the wrong conclusion that those headers were expected as part of the node API. Building addon tests like actual addons would be very helpful.

@targos Sorry for the trouble, and thanks for doing a fixup release.

BridgeAR added a commit that referenced this pull request May 22, 2019

src: remove util-inl.h include in node.h
`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 referenced this pull request May 22, 2019

Merged

v12.3.1 proposal #27814

BridgeAR added a commit that referenced this pull request May 22, 2019

2019-05-22, Version v12.3.1 (Current)
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

2019-05-22, Version v12.3.1 (Current)
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

@Martii Martii referenced this pull request May 24, 2019

Merged

Some dep updates #1607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.