Support Node 0.11.4

[kkoopa]

This is a backwards-compatible patch for Node 0.11.4+. It addresses #180 #151 #177. Travis is still stuck at 11.0.3, so it does not compile there.

[springmeyer]

Hi @kkoopa - thanks for the pull. Looks like you've put a lot of effort into the nan project. As I mention in #180, I plan to take a review the v8/node c++ changes once node v0.12.x is out. At that time I will review your pull and decide whether nan is appropriate. In the meantime, a few thoughts:

1. What about having projects that use nan.h depend on it as an actual node module via package.json. Then nan releases could be pushed to npmjs. For this to work the binding.gyp for a given project would simply need to put -I./node_modules/nan/ in the include flags.

2. Do you have any benchmarks of whether nan.h usage helps or hurts performance with node < v0.11.4?

[kkoopa]

Yes,the idea is to have it as a module configurable exactly like that. This will come in the near future, at latest when 0.12 arrives.

I have not benchmarked this, as I haven't tried it on other platforms than Linux, and I only have a VM available, which does not seem to offer a realistic benchmarking experience.

I did however do some benchmarking of hiredis-node redis/hiredis-node#40 (comment) and it showed no performance regressions. And from theory I can state that performance on old versions will be equal or better, as all of NAN is a set of conditional macros and inline functions.

[kkoopa]

As mentioned in kkoopa#1 it does not work on 0.11.0 < Node < 0.11.4, as they are outdated development releases and there is no point in supporting them. It works on 0.11.4+. Travis is still stuck at 0.11.3.

[kkoopa]

NAN is available through NPM now.

[springmeyer]

@kkoopa - I'm planning on taking a closer look at this patch in the coming weeks. Any updates you see as needed based on more movement in node core or NAN?

[kkoopa]

Updated NAN dependency to 0.4.1, which is the latest. Node 0.11.8 or should arrive at some point, but this builds with latest HEAD. There have been some deprecations in v8, so expect a new minor NAN release for 0.11.8.

[rvagg]

yes please! I'd like to start being able to benchmark on 0.11.

[kkaefer]

@rvagg Could you please explain what the benefit of making nan a dependency of this module would be? I'm not seeing large amounts of dropped code; mostly just wrapping existing callbacks.

[kkoopa]

You're looking at it from the wrong side, it's about not having large amounts of duplicated code.

[kkaefer]

Sure, but this module wasn't using nan before, and your pull request adds it. Yet, there's no removed code?

[kkoopa]

As explained in the commit message and #180, #151 and #177, the current version of node-sqlite3 does not work and will not work on anything newer than Node.js v0.10.x. That currently excludes the 0.11-series, which will become the 0.12 series in a couple of months. When 0.12 is released, all further development of 0.10 will stop. The reason it will not work is because a significant part of the API in v8 has been changed in an incompatible way.

As some people like to use outdated software, which might be legitimate due to stability concerns, this causes a problem for native addon developers, as their codebase suddenly has to support two very different platforms. The three basic options are then: support the outdated platform and lose all users with current versions, support the new platform and ignore the old production users, or make and maintain two separate versions for each group.

The best solution is actually none of these, it is wrapping all incompatible functions in an abstraction layer, unifying the two disjoint architectures and maintaining one version of the code, supporting both. This pull request offers supporting both architectures without code duplication.

[rvagg]

To see why NAN is useful, consider these two snippets that do exactly the same thing, i.e. declare a method to expose to JS, but one is for Node 0.10 and prior and the other is for 0.11 and onwards:

Node 0.10 and prior:

v8::Handle<v8::Value> DoSomething (const v8::Arguments& args) {
  v8::HandleScope scope;

  // ...

  return scope.Close(v8::String::New("We did something"));
}

Node 0.11+

void DoSomething (const v8::FunctionCallbackInfo<v8::Value>& args) {
  v8::Isolate* isolate = v8::Isolate::GetCurrent();
  v8::HandleScope scope(isolate);

  // ...

  args.GetReturnValue().Set(v8::String::New("We did something"));
}

It would be difficult to be more different! The V8 team decided to do some major API reworking and that's been bleeding into Node since around 0.11.3. NAN is all about making it unnecessary to have macro branching in your addon code just so you can support the different versions of Node that you want. NAN tests from 0.8 up to Node master and we're always adding new stuff as more V8 breakages come in. There's also some Node breakages that we smooth over too, currently this is mainly for Buffers. We also apparently support Node 0.6 but don't bother testing there.

The idea is that you include nan.h and code against the NAN API which provides a very thin layer (mostly macros) between your code and Node + V8 but it takes care of compatibility for you and will continue to take care of it as V8 and Node evolve. The alternative for Node 0.11/0.12 is going to be to drop support for 0.10 and prior or have the code full of #ifs and #elses for every one of these changes, and there are a lot of them.

The NAN version of the above code is this:

NAN_METHOD(DoSomething) {
  NanScope();

  // ...

  NanReturnValue(v8::String::New("We did something"));
}

This expands out to be whatever the target version of Node/V8 is wherever it's being compiled.

[rvagg]

Also, more directly answering the question about why pull it in as a dependency: the initial versions of NAN just distributed a header file that you put in your project and distribute yourself but now we're distributing it via npm so you don't need to have your own copy, you just pull it in via npm and compile against it. If you pin the dependency as ~0.6.0 then you would get any patch-level releases too which would include minor fixes to support current Node master as things change. It's basically developing against C++ assets like you do against JS assets in npm so you can think of it in the same way.

I believe the initial version of this pull request had nan.h embedded but that's gone now in favour of having it as a dependency.

[defunctzombie]

nan is a very good thing if your goal is to support node 0.10 and 0.12 (when it lands). Without NaN you would either be in #ifdef hell or be implementing all of nan yourself.

[kkaefer]

Okay, thanks @rvagg for explaining the rationale behind this pull request. This all makes sense.

[jeromew]

thank you @rvagg for your clear explanation of nan and how node 0.11.x is a potential nightmare for the "native modules" ecosystem in npm.

@springmeyer can you share your opinion on this and do you have an idea where this could fit in the node-sqlite3 roadmap ?

I am investigating the use of the new koa.js - http://koajs.com/ framework written by the main contributors of express.js but it only works with node 0.11.9 at this stage so I have to either go without koa.js or without node-sqlite3 which is tearing me apart ;-)

note that I only use "build-from-source" with node-sqlite3 so in my case I don't need a precompiled node-sqlite3 working with node 0.11.x but only the patched source code using Nan.

as a side node, nan was apparently accepted on node-postgres (commit by @rvagg ) here - brianc/node-postgres@f58ff73

[jeromew]

For information i tried the following today (I am using sqlite 3.8.1):

git clone --depth=1 https://github.com/kkoopa/node-sqlite3.git
cd node-sqlite3
nvm use 0.11.9
npm install --build-from-source --sqlite=/path/to/custom/sqlite

and get :

In file included from /home/user1/.node-gyp/0.11.9/src/node.h:61:0,
             from ../src/database.cc:2:
/home/user1/.node-gyp/0.11.9/deps/v8/include/v8.h: In member function ‘void v8::ReturnValue<T>::Set(uint32_t)’:
/home/user1/.node-gyp/0.11.9/deps/v8/include/v8.h:5816:31: warning: typedef ‘I’ locally defined but not used [-    Wunused-local-typedefs]
typedef internal::Internals I;
... several times and then
SOLINK_MODULE(target) Release/obj.target/node_sqlite3.node: Finished
...
[sqlite3]: Testing the binary failed: "Command failed: /home/user1/nvm/v0.11.9/bin/node: symbol lookup error: /home/user1/tmp/node-sqlite3/lib/binding/Release/node-v13-linux-x64/node_sqlite3.node: undefined symbol: _ZN2v87Integer3NewEi

The same steps work perfectly with

nvm use 0.10.22
...
[sqlite3]: Sweet: "node_sqlite3.node" is valid, node-sqlite3 is now installed!

I looked around a bit but have a hard time understanding what goes wrong. @koopa what are your steps to compile node-sqlite3 with 0.11.9 ?

Thank you

[kkoopa]

My initial guess is that your custom sqlite3 build is linking with gcc rather than g++.

[jeromew]

Looking into this (the hard way I am not a link expert ;-)

regarding the warning on "typedef internal::Internals I;", I don't think it is part of the problem.

This warning is pretty explicit in the v8.h code for this version of node

template<typename T>
void ReturnValue<T>::Set(uint32_t i) {
  TYPE_CHECK(T, Integer);
  typedef internal::Internals I;                             <==== NOT USED
  // Can't simply use INT32_MAX here for whatever reason.
  bool fits_into_int32_t = (i & (1U << 31)) == 0;
  if (V8_LIKELY(fits_into_int32_t)) {
    Set(static_cast<int32_t>(i));
    return;
  }
  Set(Integer::NewFromUnsigned(i, GetIsolate()));
}

The typedef has been removed from v8 trunk : https://code.google.com/p/v8/source/browse/trunk/include/v8.h#5789

template<typename T>
void ReturnValue<T>::Set(uint32_t i) {
  TYPE_CHECK(T, Integer);
                                                                       <==== removed
  // Can't simply use INT32_MAX here for whatever reason.
  bool fits_into_int32_t = (i & (1U << 31)) == 0;
  if (V8_LIKELY(fits_into_int32_t)) {
    Set(static_cast<int32_t>(i));
    return;
  }
  Set(Integer::NewFromUnsigned(GetIsolate(), i));
}

now back to the missing symbol. it seems to me that g++ is used by node-sqlite3 at link time ; can it really depend on my sqlite3 custom build (a pretty standard sqlite3 build configure/make/make install) ?

[springmeyer]

@jeromew - thanks for helping test and push this forward.

@springmeyer can you share your opinion on this and do you have an idea where this could fit in the node-sqlite3 roadmap?

I plan to 1) upgrade the bundled libsqlite3 version, 2) merge this, and 3) migrate the binary build system to node-pre-gyp when I find the time. I'm fully on board with using NAN and I am very thankful for the work put in to make it available. I anticipate I will be able to do this sometime in the next couple weeks.

[jeromew]

Hello. Here are the results of my research on this. Maybe @kkoopa or @rvagg you can take a look at these results. There is something weird somewhere that I don't understand.

Long story short :

1.the missing symbol is _ZN2v87Integer3NewEi
2.it can indeed be found in the compiled native extension :

 $ nm -A lib/binding/Release/node-v13-linux-x64/node_sqlite3.node | grep '_ZN2v87Integer3NewEi'                                           
 lib/binding/Release/node-v13-linux-x64/node_sqlite3.node:                 U _ZN2v87Integer3NewEi

3.but cannot be found in node v 0.11.9

$ nm -A /home/u/nvm/v0.11.9/bin/node | grep '_ZN2v87Integer3NewEi'
/home/u/nvm/v0.11.9/bin/node:0000000000703670 T _ZN2v87Integer3NewEiPNS_7IsolateE

4.instead we find this symbol _ZN2v87Integer3NewEiPNS_7IsolateE

5.different hints lead me to this block of code in v8.h

class V8_EXPORT Integer : public Number {
 public:
  static Local<Integer> New(int32_t value);
  static Local<Integer> NewFromUnsigned(uint32_t value);
  static Local<Integer> New(int32_t value, Isolate*);
  static Local<Integer> NewFromUnsigned(uint32_t value, Isolate*);
  int64_t Value() const;
  V8_INLINE static Integer* Cast(v8::Value* obj);
 private:
   Integer();
  static void CheckCast(v8::Value* obj);
};

6.it appears that only an "Isolated" version of New(uint32_t) exists in nvm node 0.11.9

7.I could not find any Nan helper that handles this case

8.note that the signature (int32_t, Isolate_) is reversed from NanNewLocal (Isolate_, Local)

9.the compilation works if I add to Nan something like

 static NAN_INLINE(v8::Local<v8::Integer> NanNewInteger(
      int32_t val
  )) {
    return v8::Integer::New(val, nan_isolate);
  }

10.and modify the node-sqlite3 code with

 cd src
 sed -i'' -e's/Integer::New/NanNewInteger/g' *

11.result: no more missing symbol at built time

 [sqlite3]: Sweet: "node_sqlite3.node" is valid, node-sqlite3 is now installed!

12.but this is not the end of the story

npm test
Creating test database... This may take several minutes.
node: symbol lookup error: /home/u/tmp/node_modules/sqlite3/lib/binding/Release/node-v13-linux-x64/node_sqlite3.node: undefined symbol: _ZNK2v85Value8IsRegExpEv

13.this probably belong to source->IsRegExp() and args[start]->IsRegExp() of statement.cc

14.this time less luck with nm

nm -A /home//nvm/v0.11.9/bin/node | grep -e 'Value[0-9]*Is'
/home//nvm/v0.11.9/bin/node:0000000000ee9458 b _ZGVZNK2v85Value7IsInt32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:0000000000ee9448 b _ZGVZNK2v85Value8IsUint32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:00000000008f6550 T _ZN2v88internal16CompileTimeValue18IsCompileTimeValueEPNS0_10ExpressionE
/home//nvm/v0.11.9/bin/node:00000000007c8e50 T _ZN2v88internal6HValue19IsInteger32ConstantEv
/home//nvm/v0.11.9/bin/node:0000000000729af0 W _ZN2v88internal6HValue29IsPurelyInformativeDefinitionEv
/home//nvm/v0.11.9/bin/node:00000000006fa760 T _ZNK2v85Value10IsExternalEv
/home//nvm/v0.11.9/bin/node:00000000006fa670 T _ZNK2v85Value10IsFunctionEv
/home//nvm/v0.11.9/bin/node:00000000006fd7a0 T _ZNK2v85Value6IsTrueEv
/home//nvm/v0.11.9/bin/node:00000000006fa6a0 T _ZNK2v85Value7IsArrayEv
/home//nvm/v0.11.9/bin/node:00000000006fd770 T _ZNK2v85Value7IsFalseEv
/home//nvm/v0.11.9/bin/node:00000000006fbe90 T _ZNK2v85Value7IsInt32Ev
/home//nvm/v0.11.9/bin/node:00000000006fa700 T _ZNK2v85Value8IsNumberEv
/home//nvm/v0.11.9/bin/node:00000000006fa6d0 T _ZNK2v85Value8IsObjectEv
/home//nvm/v0.11.9/bin/node:00000000006fbd70 T _ZNK2v85Value8IsUint32Ev
/home//nvm/v0.11.9/bin/node:00000000006fa730 T _ZNK2v85Value9IsBooleanEv
/home//nvm/v0.11.9/bin/node:00000000007c3220 W _ZNK2v88internal6HValue10IsConstantEv
/home//nvm/v0.11.9/bin/node:0000000000729ba0 W _ZNK2v88internal6HValue11IsDeletableEv
/home//nvm/v0.11.9/bin/node:00000000007c3240 W _ZNK2v88internal6HValue13IsInstructionEv
/home//nvm/v0.11.9/bin/node:00000000007c5110 T _ZNK2v88internal6HValue14IsDefinedAfterEPNS0_11HBasicBlockE
/home//nvm/v0.11.9/bin/node:00000000007299d0 W _ZNK2v88internal6HValue17IsBinaryOperationEv
/home//nvm/v0.11.9/bin/node:00000000007299f0 W _ZNK2v88internal6HValue20IsControlInstructionEv
/home//nvm/v0.11.9/bin/node:00000000007299e0 W _ZNK2v88internal6HValue24IsBitwiseBinaryOperationEv
/home//nvm/v0.11.9/bin/node:00000000007299c0 W _ZNK2v88internal6HValue27IsArithmeticBinaryOperationEv
/home//nvm/v0.11.9/bin/node:0000000000ee9460 b _ZZNK2v85Value7IsInt32EvE10minus_zero
/home//nvm/v0.11.9/bin/node:0000000000ee9450 b _ZZNK2v85Value8IsUint32EvE10minus_zero

15.and

nm -C /home/jwagner/nvm/v0.11.9/bin/node | grep -e 'IsRegExp'
00000000009db7d0 T v8::internal::FullCodeGenerator::EmitIsRegExp(v8::internal::CallRuntime*)
00000000009d73d0 T v8::internal::FullCodeGenerator::EmitIsRegExpEquivalent(v8::internal::CallRuntime*)
00000000007ec360 T v8::internal::HOptimizedGraphBuilder::GenerateIsRegExp(v8::internal::CallRuntime*)
00000000007d9d90 T v8::internal::HOptimizedGraphBuilder::GenerateIsRegExpEquivalent(v8::internal::CallRuntime*)

So IsRegExp does not seem to be exported.

I have learned a lot of things doing this but I am a bit stalled understanding why those symbols are not in the binaries given by "nvm install 0.11.9" (http://nodejs.org/dist/v0.11.9/node-v0.11.9-linux-x64.tar.gz)

16.losing hope I tried to compile node 0.11.9 from the sources

nvm uninstall 0.11.9
nvm install -s 0.11.9

17.and now ... everything works like a charm !! all tests passing green straight from @kkoopa's branch :-)

I knew "node.js" was bleeding edge so I am the only one to blame for spending so many hours on this ;-) but if you can help me understand what is going on here I will be grateful !!

Can we fix something so that these symbols get exported in the official node.js binaries or is it best practice to always compile node.js from source when using native extensions ?

[jeromew]

Just for simplicity, the issue I created will lead you to another open issue on node.js. There is indeed a problem on the pre-packaged binaries and they are looking into it.

so until they fix this issue and if you want to test @kkoopa's branch on 0.11.x, compile node.js from the source and do not use pre-packaged versions

[springmeyer]

@kkoopa - I've finished a bunch of packaging work (#245) and tagged v2.2.0. Next thing on my mind is testing your patch and getting it merged. Let me know if you might have time to update the branch (and ideally squash to a few commits), otherwise I can take a look at doing this when I have time (est 2-3 weeks).

[springmeyer]

This has now been merged! Node-sqlite >= v2.2.4 will now support node >= v0.11.13 and above thanks to the hard work of the Nan contributors.

[rvagg]

To clarify: as of nan@1.0.0 we removed support for all 0.11 versions prior to 0.11.13, the changes were too traumatic to maintain anything else. 0.8, 0.10 (and even 0.6 I believe) are all fine though.

[springmeyer]

@rvagg - sounds good. Edited my statement above to clarify.

[dimsuz]

Maybe README.md needs to be updated then?
It still says "Depends: Node.js v0.10.x or v0.8.x"

[Mithgol]

@dimsuz Good idea. I've updated README.md (commit 28e5774).

[dimsuz]

@Mithgol thanks! At last it is possible to use it with recent node-webkit! Will try to migrate this evening :)

[Mithgol]

@dimsuz Some changes are still necessary for such migration to be successful. See #308 for details.

[dimsuz]

@Mithgol thank you very much, I will follow up

[gpetrov]

@Mithgol - So are we OK to build for NW 0.9.2? I still got plenty of errors ...

[Mithgol]

@gpetrov No, it's not OK to build for NW 0.9.2.

Node-webkit v0.9.2 is based on Node.js v0.11.9, and that version is not supported by node-sqlite3.

In the v0.11.x branch of Node.js we support only v0.11.13 and newer (that's what nan@1.0.0 does as @rvagg explained in a comment above).

It's likely that node-webkit 0.9.2 is not to be supported by node-sqlite3 at all.

[Mithgol]

P. S.   Currently even a support for node-webkit 0.10.0-rc1 requires installing node-sqlite3 from the experimental travis-refactor branch, see #311 (comment) for details.

(That branch is likely to be eventually landed to the master and published as a new version of the sqlite3 package, but not right now.)

[gpetrov]

@Mithgol Yes I found that out the hard way. However node-sqlite3 has just landed version 2.2.4 which should build just fine in node 0.11.

However nw-gyp is now giving problems nwjs/nw-gyp#32

Hope @rogerwang will solve them soon