Skip to content

src: do not include internals from node_buffer.h - #15554

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

src: do not include internals from node_buffer.h#15554
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:fix-buffer-include

Conversation

@addaleax

@addaleaxaddaleax commented Sep 22, 2017

Copy link
Copy Markdown
Member

node_buffer.h is a public header, so it should not be using the node_internals.h internal header.

Ref: 290315a
Fixes: #15552

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

addons

@addaleax
src: do not include internals from node_buffer.h
6dcf62c
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.
Ref: 290315aFixes: nodejs#15552
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 22, 2017
@addaleax

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-commit/12515/

@nodejs/collaborators I would like to fast-track this to unbreak addons.

This really should have been caught by our addon tests, and I’m working on fixing that, but it’s going to be trickier than the fix and probably shouldn’t be rushed.

@AndreasMadsen

AndreasMadsen commented Sep 22, 2017

Copy link
Copy Markdown
Member

the v8.x branch works. I don't think it is strictly necessary to fast track this but I'm not against it.

@addaleax

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
MemberAuthor

@AndreasMadsen Maybe I’m misunderstanding that last comment, but as @richardlau pointed out in #15509, the bug would be “about” to break 8.6.0.

I don’t think this needs to land asap, but it would be nice to not have to wait the full 72 hours (if only because of the broken nightlies).

@AndreasMadsen

AndreasMadsen commented Sep 22, 2017

Copy link
Copy Markdown
Member

@addaleax I think you understood me fine.

mcollina
mcollina approved these changes Sep 22, 2017
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM expedited.

@richardlau

richardlau commented Sep 22, 2017

Copy link
Copy Markdown
Member

This really should have been caught by our addon tests, and I’m working on fixing that, but it’s going to be trickier than the fix and probably shouldn’t be rushed.

Perhaps we can revive #12231.

@addaleax

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
MemberAuthor

@richardlau thanks, I didn’t remember that – it’s basically exactly the kind of test change we need …

@addaleax

addaleax commented Sep 22, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in 84063a7

@addaleax
addaleax deleted the fix-buffer-include branch September 22, 2017 14:30
addaleax added a commit that referenced this pull request Sep 22, 2017
@addaleax
src: do not include internals from node_buffer.h
84063a7
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.
Ref: 290315aFixes: #15552
PR-URL: #15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
@addaleax
src: do not include internals from node_buffer.h
df18f2a
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.
Ref: 290315aFixes: nodejs/node#15552
PR-URL: nodejs/node#15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
@addaleax@jasnell
src: do not include internals from node_buffer.h
24271a7
`node_buffer.h` is a public header, so it should not be using
the `node_internals.h` internal header.
Ref: 290315aFixes: #15552
PR-URL: #15554
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins

MylesBorins commented Oct 17, 2017

Copy link
Copy Markdown
Contributor

as #14697 did not land on v6.x I'm setting this to dont-land-on-v6.x lmk if this is a mistake

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
@richardlaurichardlaurichardlau approved these changes
+2 more reviewers
@AndreasMadsenAndreasMadsenAndreasMadsen approved these changes
@gibfahngibfahngibfahn 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++.lib / srcIssues 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.

fatal error: 'node_internals.h' file not found

9 participants

@addaleax@AndreasMadsen@richardlau@MylesBorins@mcollina@cjihrig@tniessen@gibfahn@nodejs-github-bot