search

mikeseven / node-opencl

Low-level OpenCL 1.x and 2.x bindgings for node.js
156 stars 33 forks source link

Fix node v12.0.0 errors, update to nan v2.13 #69

Closed trxcllnt closed 4 years ago

trxcllnt commented 5 years ago

Hi @mikeseven! Here's a PR with updates to make everything compile in node v12.0.0. It includes a few small fixes (like https://github.com/mikeseven/node-opencl/commit/3e54df55cf17facc0e4ec2d1730326681f11a25b), and I updated a bunch of tests where appropriate. I've tested with the Intel Core i7 8700 and nvidia GTX 1070s in my laptop, but will also be testing on the V100s in a little bit.

It also adds a feature to return Uint8Arrays of the program binaries from cl.getProgramInfo() (test here). I updated the NoCLWrapper::Unwrap method to call NewInstance if the Local<Value> is a Uint8Array. This was the easiest way I found to make cl.createProgramFromBinary() work when it's passed a list of Uint8Arrays.

Look it over and let me know what you think. I still need to update our other dependencies for node 12 so I haven't tried in production settings yet, but I'll update this PR if I run into anything. Once this looks good and is merged, do you think we could do a new release of node-opencl to npm? We've been referencing my fork via a git hash, but it'd be great to have an official v0.5.0 out on npm.

lu4 commented 5 years ago

Thank you very much!

trxcllnt commented 5 years ago

@lu4 no problem! heads up, I learned after submitting this PR that the ToChecked() method is a fairly new alias of FromJust(), so this PR won't work on older versions of node until I find/replace all those calls.

lu4 commented 5 years ago

Hey @trxcllnt But I see that the last commit already contains mentioned fix from you here https://github.com/mikeseven/node-opencl/pull/69/commits/c64da8dcb1d601e43001de19fd2e420a6d035974 ? However backward compatibility is not an issue for me.

Is this repo live at all?

All this work in this PR is precious but is invisible to everyone, such a waste. I've incidentally dropped in here and was happy to find it. Maybe it's time for a new maintainer or even project?

trxcllnt commented 5 years ago

@lu4 oh, you're right! I guess I did the find/replace when I first figured it out, then forgotten I did it. As to the other thing, you'd have to ask @mikeseven. I won't be able to contribute much further from here on out, but I'd be happy to help with any changes necessary to get this PR merged.

lu4 commented 5 years ago

Since the maintainer is not active I'll probably try to somehow fork or spin-off new project based on this work (as a temporary solution) I'll try to include all the PRs suggested by the community as per my understanding of BSD-3 license ( https://tldrlegal.com/license/bsd-3-clause-license-(revised) ) it should be fine

lu4 commented 5 years ago

Also having a TypeScript support would be great

lu4 commented 5 years ago

@trxcllnt JFYI, I've noticed that the contents of some #ifndef CL_VERSION_2_0 ... #endif were not covered with the fixes you've introduced. Currently I see just one file having this problem: sampler.cpp

lu4 commented 5 years ago

@trxcllnt I can create PR for your fork?

lu4 commented 5 years ago

Here it is: https://github.com/trxcllnt/node-opencl/pull/1

lmeyerov commented 4 years ago

Extra waiting commit: https://github.com/trxcllnt/node-opencl/pull/3

We're kind of stuck now. We'll maintain http://github.com/graphistry/node-opencl#next as our node 12 prod and I'll ping Mike about what's happening here.

lmeyerov commented 4 years ago

Great, I have commit access now -- merging. (Already been using in prod for graphistry.)

lmeyerov commented 4 years ago

NOTE: Asking Mike on how to push a version update to npm, so almost there.