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.

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

nan v1.8.4 for io.js 2.0.0 compatibility #61

Merged
merged 1 commit into from
May 28, 2015
Merged

nan v1.8.4 for io.js 2.0.0 compatibility #61

merged 1 commit into from
May 28, 2015

Conversation

imyller
Copy link
Contributor

@imyller imyller commented May 5, 2015

Compiling dtrace-provider 0.4.0 fails with io.js 2.0.0:

In file included from ../dtrace_provider.cc:1:
In file included from ../dtrace_provider.h:1:
In file included from ../node_modules/nan/nan.h:74:
In file included from ../node_modules/nan/nan_new.h:181:
../node_modules/nan/nan_implementation_12_inl.h:172:66: error: too many arguments to function call, expected at most 2, have 4
  return v8::Signature::New(v8::Isolate::GetCurrent(), receiver, argc, argv);
         ~~~~~~~~~~~~~~~~~~                                      ^~~~~~~~~~
/Users/-/.node-gyp/2.0.0/deps/v8/include/v8.h:4188:3: note: 'New' declared here
  static Local<Signature> New(
  ^
1 error generated.
make: *** [Release/obj.target/DTraceProviderBindings/dtrace_provider.o] Error 1

This PR updates nan to version 1.8.4 allowing compilation with latest io.js version.

@19h
Copy link

19h commented May 5, 2015

FWIW this doesn't fix it (here). The API changed between 1.5.3 -> 1.8.4 and causes failure.

@imyller
Copy link
Contributor Author

imyller commented May 5, 2015

@KenanSulayman I'm able to get dtrace-provider to compile with nan 1.8.4 .. with one compiler warning.

So it is true that there is deprecation warning about usage of FatalException when going from nan 1.5.3 -> 1.8.4

../dtrace_probe.cc:67:7: warning: 'FatalException' is deprecated: Use FatalException(isolate, ...) [-Wdeprecated-declarations]
      FatalException(try_catch);
      ^
/Users/-/.node-gyp/2.0.0/src/node.h:263:29: note: 'FatalException' has been explicitly marked deprecated here
                inline void FatalException(const v8::TryCatch& try_catch) {
                            ^
/Users/-/.node-gyp/2.0.0/src/node.h:47:42: note: expanded from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                                         ^
1 warning generated.

Use of this deprecated symbol should be addressed by someone familiar with dtrace_probe.cc. Or am I missing something here?

@imyller
Copy link
Contributor Author

imyller commented May 5, 2015

Referencing nodejs/node#1620

@kkoopa
Copy link

kkoopa commented May 5, 2015

The change was in node 0.11.4 or so, but the deprecation warning never showed due to a bug in Node.
nodejs/nan#327
But don't wait around for a new version to get published, because that will probably not happen soon. Just use a preprocessor conditional.

#if NODE_MODULE_VERSION < 12
    FatalException(try_catch);
#else
    FatalException(Isolate::GetCurrent(), try_catch);
#endif

@imyller
Copy link
Contributor Author

imyller commented May 5, 2015

This PR is about just bumping nan dependency to ~1.8.4 for compatibility with latest io.js.

Use of deprecated nan features can be fixed later; maybe with preprocessor conditional solution @kkoopa offered.

@davepacheco
Copy link
Collaborator

I've confirmed that this patch both builds and passes tests with Node v0.12.2 and v0.10.30 on SmartOS, but the tests don't seem to work at all on OS X using v0.12.0.

@imyller
Copy link
Contributor Author

imyller commented May 15, 2015

This PR needs some attention.
dtrace-provider still not compatible with io.js 2.x.x.
/cc @rvagg

@davepacheco davepacheco mentioned this pull request May 18, 2015
@stephank
Copy link

Tests on OS X fail on io.js 1.8.1 and node.js 0.10.38 just the same, so I'm not sure if this broken anyway, and unrelated to the PR.

@davepacheco davepacheco merged commit 1032eb7 into chrisa:master May 28, 2015
@davepacheco
Copy link
Collaborator

I dug into the test failures and realized it was that they need to be run with sudo on OS X. Just published v0.5.0 with this merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants