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: re-integrate headers into node.h #20939

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@addaleax
Member

addaleax commented May 24, 2018

Alternative to #20938
(clean revert) and #20925
(adding the headers to the release tarball).

The changes to src/node.h are a clean revert in the
same ways as #20938 does it,
the difference being that the new .cc files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into callback_scope.cc and exceptions.cc is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
#20925.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
src: re-integrate headers into node.h
Alternative to #20938
(clean revert) and #20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as #20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
#20925.
@jasnell

I'm good with this approach too. just want one of the three to land today :-D

@addaleax addaleax added the fast-track label May 24, 2018

@addaleax addaleax referenced this pull request May 24, 2018

Closed

src: revert 20789 as alternative to 20925 #20938

0 of 4 tasks complete

@jasnell jasnell referenced this pull request May 24, 2018

Closed

tools: add new headers to tarball #20925

2 of 2 tasks complete
@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

@jasnell Yes, me too. Assuming you’re doing the release (it sounds like it, but I’m not sure it’s been made explicit somewhere), I’d say you can just make a call and land this one or #20938. #20925 doesn’t work for the reasons Ben mentioned.

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

@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

@MylesBorins said he'll do the release once this lands. Let's get a clean CI, and since our CI does not cover the breaking case at all, we need separate verification that this fixes the build issue with sleep and other native addons. Once we get that verification @MylesBorins should be able to get a quick release out the door.

@refack

refack approved these changes May 24, 2018

@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

CI failures are the same that we've been consistently seeing this week.

@addaleax ... given that our CI does not cover the actual breakage that occurred, have you been able to verify that this does, in fact, resolve the issue?

@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

@jasnell Yes, to the degree to which that is possible locally. I can definitely build sleep with the current set of headers.

@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

Let's get this landed then. Thank you for the help! :-)

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented May 24, 2018

so as a heads up... both master + v10.2.0 work when sleep is built locally and pointing at the node directory for headers
npm install sleep --nodedir=~/code/node/master/

We likely need to do a test build with this to ensure that it actually fixes stuff in prod

@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

@MylesBorins Yeah, I know … this is also why we’re not picking this up in our own test suite.

You can verify that this works by installing 10.2.0 via nvm, trying to build an addon (→ breaks), checking out the v10.2.0 tag here, applying this patch, creating a tarball with make tar-headers, then manually replacing the include directory in ~/.node-gyp/10.2.0 with the one from the freshly created tarball, and then building again and seeing that is passes.

It’s not a pretty way to test it locally, but it should be pretty reliable, and since this is essentially a revert as far as public headers are concerned, I think we don’t need to wait for a test build.

@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

Heh, I just walked through that exact process locally and came up with the same results. I'll get this landed in master now.

jasnell added a commit that referenced this pull request May 24, 2018

src: re-integrate headers into node.h
Alternative to #20938
(clean revert) and #20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as #20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
#20925.

PR-URL: #20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

Landed in 3f4caec

@jasnell jasnell closed this May 24, 2018

@richardlau

This comment has been minimized.

Member

richardlau commented May 24, 2018

You could alternately probably use --tarball to point node-gyp at the headers tarball, or run HEADERS_ONLY=1 python tools/install.py install <dir> and point --nodedir there.

@addaleax addaleax deleted the addaleax:integrate branch May 24, 2018

@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

@richardlau TIL about --tarball :) That also makes it pass, yay.

The --nodedir trick doesn’t quite work because it expects a src/ directory, so you still need to fiddle around a bit.

MylesBorins added a commit that referenced this pull request May 24, 2018

src: re-integrate headers into node.h
Alternative to #20938
(clean revert) and #20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as #20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
#20925.

PR-URL: #20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@MylesBorins MylesBorins referenced this pull request May 24, 2018

Merged

v10.2.1 proposal #20943

@richardlau richardlau referenced this pull request May 24, 2018

Closed

core.h not found #20921

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

src: re-integrate headers into node.h
Alternative to nodejs#20938
(clean revert) and nodejs#20925
(adding the headers to the release tarball).

The changes to `src/node.h` are a clean revert in the
same ways as nodejs#20938 does it,
the difference being that the new `.cc` files are kept here.

This has the advantage of not being another large diff that
other PRs will have to rebase against, especially since
the split into `callback_scope.cc` and `exceptions.cc` is
something that we want to keep in the long run.

This essentialy implements bnoordhuis’s suggestion from
nodejs#20925.

PR-URL: nodejs#20939
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@XadillaX

This comment has been minimized.

Member

XadillaX commented Aug 14, 2018

@addaleax It seems that node-gyp's header for 10.2.0 dose not include core.h but node.h included it then. Should we re-upload the headers?

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 14, 2018

@XadillaX That was the problem being fixed here, right? In any case, I don’t think we’re going to overwrite already-published release files…

alexeykuzmin added a commit to electron/node that referenced this pull request Sep 4, 2018

fixup! chore: add GN build files
src: re-integrate headers into node.h
nodejs/node#20939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment