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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

addaleax
Copy link
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: 290315a
Fixes: nodejs#15552
@nodejs-github-bot nodejs-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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

@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
Copy link
Member

@addaleax I think you understood me fine.

mcollina
mcollina approved these changes Sep 22, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM expedited.

@richardlau
Copy link
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
Copy link
Member Author

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

@addaleax
Copy link
Member Author

Landed in 84063a7

@addaleax addaleax closed this Sep 22, 2017
@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: 290315a
Fixes: #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: 290315a
Fixes: 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: 290315a
Fixes: #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
Copy link
Member

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@AndreasMadsen AndreasMadsen AndreasMadsen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@richardlau richardlau richardlau approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
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.
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