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

syscalls: move away from deprecated CloneElementAt #810

Merged
merged 9 commits into from Jun 28, 2018

Conversation

Projects
None yet
5 participants
@myitcv
Collaborator

myitcv commented Apr 29, 2018

We currently use Node 6.2.2 in CI for our gopherjs test runs.

Ever since the start of the Node 6.x series, v8::Array::CloneElementAt has been deprecated; here's an example from our most recent master build:

make: Entering directory `/home/ubuntu/gopherjs/node-syscall/build'
  CXX(target) Release/obj.target/syscall/syscall.o
../syscall.cc: In function ‘intptr_t toNative(v8::Local<v8::Value>)’:
../syscall.cc:28:51: warning: ‘v8::Local<v8::Object> v8::Array::CloneElementAt(uint32_t)’ is deprecated (declared at /home/ubuntu/.node-gyp/6.2.2/include/node/v8.h:3009): Cloning is not supported. [-Wdeprecated-declarations]
       native[i] = toNative(array->CloneElementAt(i));
                                                   ^

Per #809, in Node 10.x they have finally removed this method and hence attempts to follow the syscall steps fail with:

make: Entering directory '/home/norunners/poc/vert/src/github.com/gopherjs/gopherjs/node-syscall/build'
  CXX(target) Release/obj.target/syscall/syscall.o
../syscall.cc: In function ‘intptr_t toNative(v8::Local<v8::Value>)’:
../syscall.cc:28:35: error: ‘class v8::Array’ has no member named ‘CloneElementAt’
       native[i] = toNative(array->CloneElementAt(i));
                                   ^~~~~~~~~~~~~~

This PR therefore drops our usage of the deprecated method, instead preferring to use v8::Object::Clone() as required.

My C++ skills are however sorely lacking... so perhaps @neelance can offer some thoughts here (given I assume he originated the code?)

We also bump to using Node 10.x in the CI which brings us up-to-date and helps to verify the fix (all the tests pass).

Fixes #809

@myitcv

This comment has been minimized.

Collaborator

myitcv commented Apr 29, 2018

@neelance - would appreciate if you had a couple of minutes spare to look this over.

@shurcooL @hajimehoshi - this PR is also ready for review.

@hajimehoshi

This comment has been minimized.

Member

hajimehoshi commented Apr 29, 2018

I'm not familiar with node details, so I'd like to defer to other reviewers on this :-)

@dmitshur

Thanks for working on this! Just a few comments for things I could spot.

Does anyone know where the syscall.cc code comes from? Is there an "upstream" version of it? Knowing if that's the case could make reviewing this easier.

@@ -25,7 +25,17 @@ intptr_t toNative(Local<Value> value) {
Local<Array> array = Local<Array>::Cast(value);
intptr_t* native = reinterpret_cast<intptr_t*>(malloc(array->Length() * sizeof(intptr_t))); // TODO memory leak
for (uint32_t i = 0; i < array->Length(); i++) {
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
v8::Local<v8::Value> elem = array->Get(i);

This comment has been minimized.

@dmitshur

dmitshur Apr 29, 2018

Member

This file does using namespace v8; on top, and 4 lines above we have Local<Array> array instead of v8::Local<v8::Array> array, etc.

So I think it'd be better to be consistent and drop the namespace here as well:

Local<Value> elem = array->Get(i);

Similarly on another line below.

This comment has been minimized.

@myitcv

myitcv Apr 30, 2018

Collaborator

Fixed.

@@ -25,7 +25,17 @@ intptr_t toNative(Local<Value> value) {
Local<Array> array = Local<Array>::Cast(value);
intptr_t* native = reinterpret_cast<intptr_t*>(malloc(array->Length() * sizeof(intptr_t))); // TODO memory leak
for (uint32_t i = 0; i < array->Length(); i++) {
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)

This comment has been minimized.

@dmitshur

dmitshur Apr 29, 2018

Member

What's the difference between NODE_MODULE_VERSION (here) and NODE_MAJOR_VERSION (above, on line 10)? Is it better to use NODE_MODULE_VERSION here?

This comment has been minimized.

@myitcv

myitcv Apr 30, 2018

Collaborator

I honestly don't know; rather pattern matching when it comes to C++... I've switched to using NODE_MAJOR_VERSION

@myitcv

This comment has been minimized.

Collaborator

myitcv commented Apr 30, 2018

Does anyone know where the syscall.cc code comes from? Is there an "upstream" version of it? Knowing if that's the case could make reviewing this easier.

No, I don't, hence why I pinged @neelance in case he had a couple of minutes to review.

I've just pushed up a couple of fixes in response to your comments. Given the tests pass, which make fairly heavy use of this layer, I think we can be fairly confident the fix is "good enough".

@neelance

This comment has been minimized.

Member

neelance commented Apr 30, 2018

I wrote the syscall.cc, there is no upstream.

Is it even necessary to do the Clone?

@myitcv

This comment has been minimized.

Collaborator

myitcv commented Apr 30, 2018

Is it even necessary to do the Clone?

I'm really unsure; I would defer to you on that question 😄

I've pushed up accaa71 in an attempt to see whether the clone is required or not, at least against the reference that is our CI test suite.

@neelance

This comment has been minimized.

Member

neelance commented Apr 30, 2018

I honestly don't know without looking into it some more.

@theclapp

This comment has been minimized.

theclapp commented May 20, 2018

FWIW I dig into syscall.cc and see no reason to use a Clone(). Get() seems fine. My C++ is pretty rusty, but it looks like Get() returns a Local<Value>, which boils down to a *Value. No reason to clone it.

CI: Update to latest Node.js version.
Node.js 10.5.0 is the latest current version according to https://nodejs.org/en/download/current/.
@dmitshur

Thanks for the investigation and the fix. Sorry about the delay.

I've tested it and didn't find any issues. LGTM.

CI tests are all passing, except:

$ diff -u <(echo -n) <(git status --porcelain)
--- /dev/fd/63	2018-06-28 18:30:03.118797187 +0000
+++ /dev/fd/62	2018-06-28 18:30:03.118797187 +0000
@@ -0,0 +1 @@
+ M package-lock.json

diff -u <(echo -n) <(git status --porcelain) returned exit code 1

That's not happening on master.

@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 28, 2018

The diff I get after running npm install locally is:

diff --git a/package-lock.json b/package-lock.json
index ed79b7a..6118aad 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -21,8 +21,8 @@
       "integrity": "sha512-r+MU0rfv4L/0eeW3xZrd16t4NZfK8Ld4SWVglYBb7ez5uXFWHuVRs6xCTrf1yirs9a4j4Y27nn7SRfO6v67XsQ==",
       "dev": true,
       "requires": {
-        "commander": "2.13.0",
-        "source-map": "0.6.1"
+        "commander": "~2.13.0",
+        "source-map": "~0.6.1"
       }
     }
   }
@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 28, 2018

I guess I'll go with npm install --no-save as a solution. We don't want npm install to be modifying dependencies in package-lock.json in CI.

CI: Use --no-save option for npm install of dev dependencies.
We don't want npm install to be saving to dependencies and modifying
package-lock.json in CI. That should only be done by developers locally.

Reference: https://docs.npmjs.com/cli/install.
@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 28, 2018

Well, that didn't work, because Circle CI runs a second npm install without the --no-save option as part of its "inference commands":

image

dmitshur added some commits Jun 28, 2018

CI: Override dependency commands.
We don't want the inferred npm install command to run without --no-save.
CI: Revert back to Node.js 10.0.0.
Node.js 10.0.5 comes with a newer npm which causes package-lock.json
to be modified after npm install runs. Just use 10.0.0 for now where that
doesn't happen.
@dmitshur

This comment has been minimized.

Member

dmitshur commented Jun 28, 2018

Ended up going back to Node.js 10.0.0 which doesn't cause package-lock.json to get modified, it's the simplest solution for now. We'll deal with newer Node.js versions after first migrating off the deprecated Circle CI 1.0 (#766).

@dmitshur dmitshur merged commit 5232e51 into gopherjs:master Jun 28, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

ralimi added a commit to google/chrome-ssh-agent that referenced this pull request Nov 3, 2018

Update vendored files.
govendor fetch +vendor +modified

This pulls in gopherjs/gopherjs#810 for gopherjs to fix the build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment