Node: all native modules on 7.7.0 are broken because trace_event.h is not included in the distributable, but referred by node.h

[jakutis]

• Version: v7.7.0
• Platform: Linux localhost.localdomain 4.9.12-200.fc25.x86_64 #1 SMP Thu Feb 23 19:31:49 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Modules with node-gyp (e.g. heapdump) fail with:

make: Entering directory '<...>/node_modules/heapdump/build'
  CXX(target) Release/obj.target/addon/src/heapdump.o
In file included from ../src/heapdump.cc:15:0:
/home/jakutis/.node-gyp/7.7.0/include/node/node.h:44:33: fatal error: tracing/trace_event.h: No such file or directory
 #include "tracing/trace_event.h"
                                 ^
compilation terminated.
addon.target.mk:92: recipe for target 'Release/obj.target/addon/src/heapdump.o' failed
make: *** [Release/obj.target/addon/src/heapdump.o] Error 1

[richardlau]

11106 shouldn't have been released without also backporting #10959 (https://github.com/nodejs/node/pull/11106#issuecomment-276748731)

[gjuchault]

Note: this fails all travis build with native modules

[targos]

https://github.com/nodejs/node/pull/10959 didn't exactly need a backport. It wasn't cherry-picked to v7.x because it had the dont-land-on-v7.x label.

I'm tried to cherry-pick the commit but now run into crashes with 2 tests so this might require another commit to work.

[targos]

Retrying with more tracing commits...

[targos]

https://github.com/nodejs/node/pull/11634

[mhdawson]

If we have any native modules in CitGM you would have expected the tests to fail since we'd not be able to compile the module. If we don't have one we should probably add one, my suggestion would be heapdump

[sam-github]

I would suggest nan as well, except its tests fail even for 7.4.0 on Linux, just me?

[richardlau]

@mhdawson There are at least two CITGM jobs:

1. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/
2. https://ci.nodejs.org/view/Node.js-citgm/job/gibfahn-citgm-smoker-more-platforms/

I believe 1. is used for testing Node.js PR's but this would not have picked up the problem because it compiles native addons against the Node.js source tree (where the header is present). This is not the default behavior of npm/node-gyp, which downloads the headers tarball from https://node.js.org. Building against the headers tarball is not the same as building against the source tree, because:

• Only a subset of the headers are copied into the headers tarball
• The headers that are copied are relocated in the headers tarball (see https://github.com/nodejs/node-gyp/pull/1055)

Obviously before the release is made the headers tarball is not available on https://node.js.org.

The other job, 2., is used for testing CitGM PR's and is how we actually caught this problem in master (see #10929 and #10959). This compiles native addons against the binaries from https://node.js.org -- When we picked up the problem in master it was against a Node 8 nightly.

cc @nodejs/citgm

[richardlau]

We do already have node-report, as a native addon, in CitGM and I'm in the process of getting node-gyp added (https://github.com/nodejs/citgm/pull/370) which has a basic addon test in its tests. But as I pointed out in my previous comment they wouldn't have failed due to the way the jobs are currently building against the Node.js source tree.

[gibfahn]

@mhdawson to add to what @richardlau said, we do test a good number of native modules in node, the problem is that we don't test our releases as external users before actually releasing them.

Raised https://github.com/nodejs/citgm/issues/377 to document the native modules in citgm.

_EDIT:_ CITGM currently tests 12 native modules according to https://github.com/nodejs/citgm/pull/378

[mhdawson]

@gibfahn, @richardlau ok, got it. So its specific to the release packaging. Does raise the issue that some level of testing on the release binaries would make sense. We've had some prior discussion about download testing due to the packages being corrupted or not build correctly. This might be another case were we should have some tests that run after a release is made to validate what went up on the release site. At least that way we'd hopefully find the problems before other people ran into them.

[blacksun1]

+1

[targos]

v7.7.1 has been released.

[GongT]

update Docker please ~

[jakutis]

why is it that 7.7.1 is published as released, but docker link (https://hub.docker.com/_/node/) in the announcement page https://nodejs.org/en/download/current/ is actually 7.7.0?

[gibfahn]

why is it that 7.7.1 is published as released, but docker link (https://hub.docker.com/_/node/) in the announcement page https://nodejs.org/en/download/current/ is actually 7.7.0?

This is a question for @nodejs/docker, but I'm pretty sure the docker update isn't done as part of the release (the docker team update the images separately).

[Starefossen]

There is a delay for the Docker Image as official Docker Images needs to go through a spree at approval process with Docker.

The new version is on its way and should be available later today.

On 2 Mar 2017, at 11:32, Gibson Fahnestock [e-mail] wrote:

why is it that 7.7.1 is published as released, but docker link (https://hub.docker.com/_/node/) in the announcement page https://nodejs.org/en/download/current/ is actually 7.7.0?

This is a question for @nodejs/docker, but I'm pretty sure the docker update isn't done as part of the release (the docker team update the images separately).

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[Starefossen]

The new Docker Image was just accepted by Docker and should be available from Docker Hub any time now. Sorry for the inconvenience folks and thanks for your patience.