node.h [API] cleanup: remove node_internals.h & extra dependencies from node.h

[brodycj]

Checklist

• make -j8 test (UNIX) passes
• commit message follows commit guidelines

Affected core subsystem(s)

Node.js API (src)

Description of change

Removes the following lines from node.h (node.h#L155-L160):

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
#include "node_internals.h"
#endif

#include <assert.h>
#include <stdint.h>

UPDATED: also removes the following from node.h:

• mess in node.h#L167-L176 that defines size_t/ssize_t (Node.js API cleanup ideas for feedback #9766 (comment) by @bnoordhuis: "I think we can simply replace that with a #include <stddef.h> because it's a workaround for old Visual Studio versions that we no longer support." But node.h includes v8.h which in turn includes stddef.h so I think these lines can simply go away.)
• break in namespace node block

For discussion

I started the commit message with "api:" since node.h is exported for consumption by both adding and embedders. In case this is not desired I would be happy to change it to something like "src:", "include:", or perhaps something else.

I found that a number of source files unnecessarily include node.h and would be happy to fix this (here or in another PR).

From #9766: I also hope we can find a nice way rework the following lines in node.h (node.h#L167-L176) are now removed:

#ifdef _WIN32
// TODO(tjfontaine) consider changing the usage of ssize_t to ptrdiff_t
#if !defined(_SSIZE_T_) && !defined(_SSIZE_T_DEFINED)
typedef intptr_t ssize_t;
# define _SSIZE_T_
# define _SSIZE_T_DEFINED
#endif
#else  // !_WIN32
# include <sys/types.h>  // size_t, ssize_t
#endif  // _WIN32

[mhdawson]

CI run for validation across platforms: https://ci.nodejs.org/job/node-test-pull-request/4975/

[brodycj]

CI run for validation across platforms: https://ci.nodejs.org/job/node-test-pull-request/4975/

@mhdawson it seems to indicate test failures but I cannot see what is going wrong. Can you give me a clue how to find the error message(s)?

[Trott]

it seems to indicate test failures but I cannot see what is going wrong. Can you give me a clue how to find the error message(s)?

Looks like lots of build failures. Here's a link to the (very long) console output from one of them:

https://ci.nodejs.org/job/node-test-commit-linux/6232/nodes=centos5-32/consoleText

Looks like the trouble is this:

  g++ '-DNODE_ARCH="ia32"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_HAVE_SMALL_ICU=1' '-DHAVE_INSPECTOR=1' '-DHAVE_OPENSSL=1' '-D__POSIX__' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DUCONFIG_NO_LEGACY_CONVERSION=1' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' -I../src -I../tools/msvs/genfiles -I../deps/uv/src/ares -I/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/out/Release/obj/gen -I../deps/v8_inspector/include -I/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/out/Release/obj/gen/include -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/openssl/openssl/include -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include  -pthread -Wall -Wextra -Wno-unused-parameter -m32 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/out/Release/.deps//home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/out/Release/obj.target/node/src/node.o.d.raw   -c -o /home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/out/Release/obj.target/node/src/node.o ../src/node.cc
In file included from ../src/node.cc:1:0:
../src/node.h: In function 'v8::Local<v8::Value> node::Encode(const void*, size_t, node::encoding)':
../src/node.h:291:349: error: 'assert' was not declared in this scope
 NODE_DEPRECATED("Use Encode(isolate, ...)",
                                                

[brodycj]

Thanks @Trott. Is the Node CI documented somewhere?

When I tried building on my Windows machine I find the following problems:

• assert is not defined (easy to fix)
• ssize_t is not properly defined

I am now thinking to do this right by first fixing node.h to follow a comment by @tjfontaine to use ptrdiff_t instead of the non-portable ssize_t type.

I would also like to see automatic CI for the major platforms (Linux, Windows, and UNIX) on public systems such as AppVeyor & TravisCI. This would make it easier for patch authors to verify that their changes do not break on the major platforms before submitting them. NAN seems to do this on AppVeyor for Windows and TravisCI for Linux & macOS (certified UNIX variant: http://unix.stackexchange.com/a/1490/164511).

I will close this now and take the following actions:

• Raise an issue to add AppVeyor & TravisCI builds;
• work on a new PR with fixes including: replace use of non-portable ssize_t type, remove node_internal.h dependency from node.h as proposed here, and cleanup some more header dependencies;
• test on macOS, Windows, and probably Linux before submitting.

[mhdawson]

@brodybits, from the link I pasted in you can drill down to the specific platforms and then there is an option to view the console output which is where you'd see the output @Trott provided. If you can test on macOS, Windows and Linux that most often catches problems. Only collaborators can kick of CI runs, but if you want to have a run in advance of a PR you could open an issue asking that we kick one off and I expect it would happen fairly quickly.