Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

broken on Node master #116

Closed
mcollina opened this issue Dec 15, 2018 · 16 comments
Closed

broken on Node master #116

mcollina opened this issue Dec 15, 2018 · 16 comments

Comments

@mcollina
Copy link
Member

I think the goal is to have it in core by Node v12, right? So possibly this is a won't fix.
@mcollina mcollina mentioned this issue Dec 15, 2018
Closed
9 tasks
@targos targos mentioned this issue Dec 15, 2018
Closed
@richardlau
Copy link
Member

richardlau commented Dec 15, 2018 •
edited
Loading

I had started looking at this but am caught up with end of year red tape at work. One of the things we need to figure out is if fixing this requires us to drop support for older LTS releases of Node.js and thus require a semver major bump. (In the case that it does, it should be fine to drop support for Node.js 6 in the semver major as it will be EOL when 12 is released).

@richardlau
Copy link
Member

In theory this is fixed in the PR to merge node-report into core, but that PR has quite significant changes that it wasn't obvious at first glance what needs to change here.

@richardlau
Copy link
Member

For reference, here's a build.log from the node-report CI job (https://ci.nodejs.org/job/nodereport-continuous-integration/259/) showing the compilation failures: https://gist.github.com/richardlau/e8728b278f5ce53ec002cf46d6af012d

@richardlau
Copy link
Member

Some of the errors are from nan:

e.g.

12:10:21 In file included from ../node_modules/nan/nan_new.h:189:0,
12:10:21                  from ../node_modules/nan/nan.h:223,
12:10:21                  from ../src/node_report.h:4,
12:10:21                  from ../src/utilities.cc:1:
12:10:21 ../node_modules/nan/nan_implementation_12_inl.h: In static member function ‘static Nan::imp::FactoryBase<v8::StringObject>::return_t Nan::imp::Factory<v8::StringObject>::New(v8::Local<v8::String>)’:
12:10:21 ../node_modules/nan/nan_implementation_12_inl.h:356:37: error: no matching function for call to ‘v8::StringObject::New(v8::Local<v8::String>&)’
12:10:21 return v8::StringObject::New(value).As<v8::StringObject>();

others, e.g.

12:10:21 ../src/node_report.cc: In function ‘void nodereport::PrintJavaScriptErrorStack(std::ostream&, v8::Isolate*, v8::MaybeLocal<v8::Value>)’:
12:10:21 ../src/node_report.cc:512:52: error: no matching function for call to ‘v8::StackTrace::GetFrame(int&)’
12:10:21 PrintStackFrame(out, isolate, stack->GetFrame(i), i, nullptr);

are fixable, but the new function (that takes an isolate parameter) doesn't exist in neither Node.js 6 nor 8.

@richardlau
Copy link
Member

I'm comfortable in a semver major dropping support for Node.js 6 to support Node.js 12 as it will become EOL when 12 is released but Node.js 12 overlaps Node.js 8.

Hopefully Node.js 12 will include equivalent functionality to this module, and we'd just redirect people coming here to use the built-in functions and just keep this module around to support older still supported Node.js releases.

@richardlau
Copy link
Member

nodejs/nan#831 fixes the nan errors (it's in nan's master branch but not in a release). Until there is a new release of nan, node-report will be broken when compiling against Node.js master (12.0.0-pre).

For the non-nan errors I guess we can attempt to narrow down the V8 versions the replacement function was introduced in and have ifdef's (or maybe be less accurate and gate on the V8 versions in Node.js 6/8/10).

richardlau added a commit to richardlau/nodereport that referenced this issue Dec 21, 2018
@richardlau
Fix use of deprecated methods removed in V8 7.0
8486893 
StackFrame::GetFrame(uint32_t index) was deprecated in V8 6.9 and
removed in V8 7.0.

Refs: nodejs/node#22531
Refs: nodejs#116
@richardlau richardlau mentioned this issue Dec 21, 2018
Merged
@richardlau
Copy link
Member

I've opened #119 to fix the errors that originate from this project. To fully resolve we need a nan release containing nodejs/nan#831.

@MylesBorins
Copy link

Looks like this is the PR that broke node-report

nodejs/node#24861

Just backed it out of 11.x until the nan fix lands

@MylesBorins MylesBorins mentioned this issue Dec 26, 2018
Closed
4 tasks
richardlau added a commit to richardlau/nodereport that referenced this issue Jan 22, 2019
@richardlau
Fix use of deprecated methods removed in V8 7.0
26f88d0 
StackFrame::GetFrame(uint32_t index) was deprecated in V8 6.9 and
removed in V8 7.0.

PR-URL: nodejs#119
Refs: nodejs/node#22531
Refs: nodejs#116
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@richardlau
Copy link
Member

There's a new release of nan which contains nodejs/nan#831. I missed one deprecated method, but once that's fixed we should be good: #124

@richardlau
Copy link
Member

Should be good now as of node-report@2.2.4 and nan@2.13.0.

@BridgeAR
Copy link
Member

It is broken on master again after the update to V8 7.4.

@BridgeAR
Copy link
Member

See https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1781/nodes=debian9-64/testReport/junit/(root)/citgm/node_report_v2_2_4/

@richardlau richardlau reopened this Mar 29, 2019
@richardlau
Copy link
Member 

 > node-report@2.2.4 install /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/citgm_tmp/502fd894-81d3-4c68-9aff-d75829bb5039/node-report
 > node-gyp rebuild
 make: Entering directory '/home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/citgm_tmp/502fd894-81d3-4c68-9aff-d75829bb5039/node-report/build'
   CXX(target) Release/obj.target/api/src/module.o
   CXX(target) Release/obj.target/api/src/node_report.o
 api.target.mk:119: recipe for target 'Release/obj.target/api/src/module.o' failed
 make: Leaving directory '/home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/citgm_tmp/502fd894-81d3-4c68-9aff-d75829bb5039/node-report/build'
 ../src/module.cc: In function 'void nodereport::Initialize(v8::Local<v8::Object>)':
 ../src/module.cc:430:75: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(TriggerReport)->GetFunction());
                                                                            ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:432:71: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(GetReport)->GetFunction());
                                                                        ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:434:71: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(SetEvents)->GetFunction());
                                                                        ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:436:71: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(SetSignal)->GetFunction());
                                                                        ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:438:73: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(SetFileName)->GetFunction());
                                                                          ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:440:74: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(SetDirectory)->GetFunction());
                                                                           ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 ../src/module.cc:442:72: error: no matching function for call to 'v8::FunctionTemplate::GetFunction()'
                 Nan::New<v8::FunctionTemplate>(SetVerbose)->GetFunction());
                                                                         ^
 In file included from /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/src/node.h:63:0,
                  from ../node_modules/nan/nan.h:53,
                  from ../src/node_report.h:4,
                  from ../src/module.cc:1:
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note: candidate: v8::MaybeLocal<v8::Function> v8::FunctionTemplate::GetFunction(v8::Local<v8::Context>)
    V8_WARN_UNUSED_RESULT MaybeLocal<Function> GetFunction(
                                               ^~~~~~~~~~~
 /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/node/deps/v8/include/v8.h:5950:46: note:   candidate expects 1 argument, 0 provided
 make: *** [Release/obj.target/api/src/module.o] Error 1
 make: *** Waiting for unfinished jobs....
 gyp ERR! build error 
 gyp ERR! stack Error: `make` failed with exit code: 2
 gyp ERR! stack     at ChildProcess.onExit (/home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/smoker/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:262:23)
 gyp ERR! stack     at ChildProcess.emit (events.js:194:13)
 gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
 gyp ERR! System Linux 4.9.0-6-amd64
 gyp ERR! command "/home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/smoker/bin/node" "/home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/smoker/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
 gyp ERR! cwd /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/citgm_tmp/502fd894-81d3-4c68-9aff-d75829bb5039/node-report
 gyp ERR! node -v v12.0.0-pre
 gyp ERR! node-gyp -v v3.8.0
 gyp ERR! not ok 
 npm ERR! code ELIFECYCLE
 npm ERR! errno 1
 npm ERR! node-report@2.2.4 install: `node-gyp rebuild`
 npm ERR! Exit status 1
 npm ERR! 
 npm ERR! Failed at the node-report@2.2.4 install script.
 npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
 npm ERR! A complete log of this run can be found in:
 npm ERR!     /home/iojs/build/workspace/citgm-smoker/nodes/debian9-64/npm_cache/_logs/2019-03-29T01_06_52_051Z-debug.log

@richardlau
Copy link
Member

These new errors are all coming from nan.

@richardlau
Copy link
Member

Ah calling the nan helpers rather than directly setting the exports appears to work. PR incoming.

@richardlau
Copy link
Member

PR to fix compilation with Node.js and V8 7.4: #125

@richardlau richardlau mentioned this issue Mar 29, 2019
Merged
richardlau added a commit to richardlau/nodereport that referenced this issue Mar 31, 2019
@richardlau
Use nan helpers to set exports
111595a 
Fixes compilation errors when built on Node.js master with V8 7.4.

`NAN_EXPORT()` requires the function name exported to JavaScript land
and the exported C++ method to match. Change the exported C++ methods
to begin with a lower case character to preserve the existing casing of
the name exported to JavaScript.

PR-URL: nodejs#125
Fixes: nodejs#116
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
richardlau added a commit that referenced this issue Mar 31, 2019
@richardlau
Use nan helpers to set exports
fabac78 
Fixes compilation errors when built on Node.js master with V8 7.4.

`NAN_EXPORT()` requires the function name exported to JavaScript land
and the exported C++ method to match. Change the exported C++ methods
to begin with a lower case character to preserve the existing casing of
the name exported to JavaScript.

PR-URL: #125
Fixes: #116
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mcollina @MylesBorins @richardlau @BridgeAR