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

Does not build on Node.js 4.2.0 (LTS) (OS X) #13

Closed
mgwalker opened this issue Oct 13, 2015 · 13 comments
Closed

Does not build on Node.js 4.2.0 (LTS) (OS X) #13

mgwalker opened this issue Oct 13, 2015 · 13 comments

Comments

@mgwalker
Copy link

Node.js has just cut its first LTS release, version 4.2.0. I'd dive in to try to offer up a PR, but I've never worked with Node's native bindings stuff so I really don't have much to offer. Here's some info about my environment:

  • OS X 10.10.5
  • Node.js 4.2.0
  • npm 3.3.8
  • hdf5 1.8.15
  • gcc 4.9

Note that this works fine when I switch back to Node 0.12.7. I haven't tried on any Linux variants yet, nor Windows.

From what I can tell, it looks like the new version of V8 is throwing a wrench into the works? Here's what appears to be the relevant bits of the build (please let me know if more info would be helpful, I'll be glad to provide as much as I can):

Thread model: posix
gcc version 4.9.3 (Homebrew gcc49 4.9.3) 
COLLECT_GCC_OPTIONS='-D' 'NODE_GYP_MODULE_NAME=h5im' '-D' '_DARWIN_USE_64_BIT_INODE=1' '-D' '_LARGEFILE_SOURCE' '-D' '_FILE_OFFSET_BITS=64' '-D' 'BUILDING_NODE_EXTENSION' '-I' '/Users/username/.node-gyp/4.2.0/include/node' '-I' '/Users/username/.node-gyp/4.2.0/src' '-I' '/Users/username/.node-gyp/4.2.0/deps/uv/include' '-I' '/Users/username/.node-gyp/4.2.0/deps/v8/include' '-I' '/usr/local/Cellar/hdf5/1.8.15/include' '-Os' '-gdwarf-2' '-mmacosx-version-min=10.5' '-m64' '-Wall' '-Wendif-labels' '-Wextra' '-Wno-unused-parameter' '-std=gnu++11' '-fno-rtti' '-fno-threadsafe-statics' '-fPIC' '-O4' '-std=c++11' '-fexceptions' '-v' '-MMD' '-MF' './Release/.deps/Release/obj.target/h5im/src/h5im.o.d.raw' '-c' '-o' 'Release/obj.target/h5im/src/h5im.o' '-mtune=core2'
 /usr/local/Cellar/gcc49/4.9.3/libexec/gcc/x86_64-apple-darwin14.4.0/4.9.3/cc1plus -quiet -v -I /Users/username/.node-gyp/4.2.0/include/node -I /Users/username/.node-gyp/4.2.0/src -I /Users/username/.node-gyp/4.2.0/deps/uv/include -I /Users/username/.node-gyp/4.2.0/deps/v8/include -I /usr/local/Cellar/hdf5/1.8.15/include -MMD Release/obj.target/h5im/src/h5im.d -MF ./Release/.deps/Release/obj.target/h5im/src/h5im.o.d.raw -MQ Release/obj.target/h5im/src/h5im.o -D__DYNAMIC__ -D NODE_GYP_MODULE_NAME=h5im -D _DARWIN_USE_64_BIT_INODE=1 -D _LARGEFILE_SOURCE -D _FILE_OFFSET_BITS=64 -D BUILDING_NODE_EXTENSION ../src/h5im.cc -fPIC -quiet -dumpbase h5im.cc -mmacosx-version-min=10.5 -m64 -mtune=core2 -auxbase-strip Release/obj.target/h5im/src/h5im.o -gdwarf-2 -Os -O4 -Wall -Wendif-labels -Wextra -Wno-unused-parameter -std=gnu++11 -std=c++11 -version -fno-rtti -fno-threadsafe-statics -fPIC -fexceptions -o /var/folders/gm/prk8pp4n6b3b4p82cpmdfssdykv292/T//ccY4bnzL.s
GNU C++ (Homebrew gcc49 4.9.3) version 4.9.3 (x86_64-apple-darwin14.4.0)
    compiled by GNU C version 4.9.3, GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
ignoring nonexistent directory "/usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/../../../../../../x86_64-apple-darwin14.4.0/include"
ignoring nonexistent directory "/Users/username/.node-gyp/4.2.0/src"
ignoring nonexistent directory "/Users/username/.node-gyp/4.2.0/deps/uv/include"
ignoring nonexistent directory "/Users/username/.node-gyp/4.2.0/deps/v8/include"
#include "..." search starts here:
#include <...> search starts here:
 /Users/username/.node-gyp/4.2.0/include/node
 /usr/local/Cellar/hdf5/1.8.15/include
 /usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/../../../../../../include/c++/4.9.3
 /usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/../../../../../../include/c++/4.9.3/x86_64-apple-darwin14.4.0
 /usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/../../../../../../include/c++/4.9.3/backward
 /usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/include
 /usr/local/include
 /usr/local/Cellar/gcc49/4.9.3/include
 /usr/local/Cellar/gcc49/4.9.3/lib/gcc/4.9/gcc/x86_64-apple-darwin14.4.0/4.9.3/include-fixed
 /usr/include
 /System/Library/Frameworks
 /Library/Frameworks
End of search list.
GNU C++ (Homebrew gcc49 4.9.3) version 4.9.3 (x86_64-apple-darwin14.4.0)
    compiled by GNU C version 4.9.3, GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 588f768c4156eb0b3065f0148fd605f3
In file included from ../src/h5im.cc:4:0:
../src/h5_im.hpp: In static member function 'static void NodeHDF5::H5im::read_image(const v8::FunctionCallbackInfo<v8::Value>&)':
../src/h5_im.hpp:104:132: error: call of overloaded 'New(v8::Isolate*, const char*, hsize_t)' is ambiguous
     v8::Local<v8::Object> buffer=node::Buffer::New(v8::Isolate::GetCurrent(), (const char*)contentBuffer.get(), planes*width*height);
                                                                                                                                    ^
../src/h5_im.hpp:104:132: note: candidates are:
In file included from ../src/h5_im.hpp:5:0,
                 from ../src/h5im.cc:4:
/Users/username/.node-gyp/4.2.0/include/node/node_buffer.h:31:40: note: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, v8::Local<v8::String>, node::encoding) <near match>
 NODE_EXTERN v8::MaybeLocal<v8::Object> New(v8::Isolate* isolate,
                                        ^
/Users/username/.node-gyp/4.2.0/include/node/node_buffer.h:31:40: note:   no known conversion for argument 3 from 'hsize_t {aka long long unsigned int}' to 'node::encoding'
/Users/username/.node-gyp/4.2.0/include/node/node_buffer.h:43:40: note: v8::MaybeLocal<v8::Object> node::Buffer::New(v8::Isolate*, char*, size_t) <near match>
 NODE_EXTERN v8::MaybeLocal<v8::Object> New(v8::Isolate* isolate,
                                        ^
/Users/username/.node-gyp/4.2.0/include/node/node_buffer.h:43:40: note:   no known conversion for argument 2 from 'const char*' to 'char*'
In file included from ../src/h5im.cc:4:0:
../src/h5_im.hpp: In static member function 'static void NodeHDF5::H5im::make_palette(const v8::FunctionCallbackInfo<v8::Value>&)':
../src/h5_im.hpp:128:53: warning: narrowing conversion of 'rankValue.v8::Local<T>::operator-><v8::Value>()->v8::Value::ToInt32().v8::Local<T>::operator-><v8::Int32>()->v8::Int32::Value()' from 'int32_t {aka int}' to 'hsize_t {aka long long unsigned int}' inside { } [-Wnarrowing]
     hsize_t pal_dims[1]{rankValue->ToInt32()->Value()};
                                                     ^
../src/h5_im.hpp:129:12: warning: unused variable 'err' [-Wunused-variable]
     herr_t err=H5IMmake_palette ( args[0]->ToInt32()->Value(), *dset_name, pal_dims, (const unsigned char *)buffer->Buffer()->Externalize().Data());
            ^
make: *** [Release/obj.target/h5im/src/h5im.o] Error 1
@mgwalker
Copy link
Author

I decided to dust off my C++ and see if I could be helpful. I added Nan as a dependency (it abstracts Node and V8 APIs: https://www.npmjs.com/package/nan) and used that in the places where the build was breaking. I made some naive assumptions to get it to build, so it probably doesn't actually work, but building was one step!

I'll try tomorrow to see if I can get the tests to run and everything, and then if you're okay with adding Nan as a dependency, I can create a PR for you. In the meantime, here's the commit where I got everything building on Node 4.2 (and still builds on Node 0.12.7):

mgwalker@a8de73c

@rimmartin
Copy link
Collaborator

I'm not into nan. for the long haul. I have it building for v4; just didn't commit and publish. Am branching the code for v12 and v4.
I'll commit and publish tonight

@mgwalker
Copy link
Author

Cool, thanks for the update!

@rimmartin
Copy link
Collaborator

Nan limits experimenting with the latest change in v8 and there are some cool things they keep adding

The incompatible change is node::Buffer initialization on the native side going through a check

@rimmartin
Copy link
Collaborator

I pushed and published. I did this with nodejs v4.1.1 while building v4.2.1. Now this project builds with v4.2.1 yet during runtime with hdf5-1.8.15-patch1 I see a bug and am looking for it

@rimmartin
Copy link
Collaborator

Also the mac I have to use is at work and I'm getting the network admin to put nodejs v4.2.1 on it to test ad create prebuilt packages

@rimmartin
Copy link
Collaborator

node::Buffer::HasInstance

is returning a true for an object that is not a node::Buffer. Investigating further...

@rimmartin
Copy link
Collaborator

HasInstance 1
constructor? Uint8Array
and Uint8Array is not a node::Buffer

Have to study this some more; definitely different between v4.1.1 and 4.2.1. Haven't tested 4.2.0

@rimmartin
Copy link
Collaborator

Ah!
https://nodesource.com/blog/cpp-addons-for-nodejs-v4
node::Buffer has been completely rewritten with Uint8Array as its base. [Hope the buffer size is not now limited to int.]

When I do

            var buffer=new Buffer(5*8, "binary");
            buffer.type=H5Type.H5T_NATIVE_DOUBLE;
            buffer.writeDoubleLE(1.0, 0);
            buffer.writeDoubleLE(2.0, 8);
            buffer.writeDoubleLE(3.0, 16);
            buffer.writeDoubleLE(4.0, 24);
            buffer.writeDoubleLE(5.0, 32);
            console.dir("Buffer name "+buffer.constructor.name);

it says it's a Buffer; but the same object on the native side will say Uint8Array and also satisfy the HasInstance regardless if constructed by Buffer or Uint8Array. v4.2.1 has the change; v4.1.1 doesn't; building and testing v4.2.0... v4.2.0 has the issue.

I don't know why they change the constructor name going from javascript to native side; going to try to join a discussion on this.

@mgwalker
Copy link
Author

Yeah 4.2.0 is when they upgraded the v8 engine. It was the thing that held up the LTS blessing.

Interesting find. I will follow the node discussion as well.

Thanks all for all your hard work!

@rimmartin
Copy link
Collaborator

ugh. unhappy with their take on it. Buffer=Uint8Array now

limits size to int and this is the size of all h5 dataset dimensions multiplied together.

It also is back to dealing with the Externalize which is a one time thing and breaks the multiple use of a buffer and passes memory cleanup to native. Have to think and study. Been wanting to look at streaming which will move multiple buffers along.

One of my design principles is avoid copying things and the Externalize just to get to the data causes a single use per buffer/object

@rimmartin
Copy link
Collaborator

in the case of binaryjs streaming all the way to a browser page, native streaming of chunks/sections was on my list to tackle. As a step I had implemented regions access of a dataset; foreseeing numerous applications won't want to send a terabyte to the browser anyway.

need to move on from 4.1.x because of other broken things such as Buffer.concat

@rimmartin
Copy link
Collaborator

Currently making Uint8Array work on the native side for Buffer and be as interchangeable at this point as possible. Hope your dataset size is within the size limit on Uint8Array.

Then going to develop streams

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

No branches or pull requests

2 participants