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

Fix up node-syscall to work with Node 12. #949

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@nevkontakte
Copy link
Contributor

nevkontakte commented Oct 21, 2019

Some APIs were removed in Node 12, and new "safer" APIs must be used
instead. Perhaps this implementation is not the most elegant or produces
less then ideal error messages, but at least it compiles.

While at it, added an arena type to manage and gracefully free temporary
buffers we allocate for the duration of the call.

I couldn't find any tests for the syscall module, so I wrote a small
one.

Closes #935

Some APIs were removed in Node 12, and new "safer" APIs must be used
instead. Perhaps this implementation is not the most elegant or produces
less then ideal error messages, but at least it compiles.

While at it, added an arena type to manage and gracefully free temporary
buffers we allocate for the duration of the call.

I couldn't find any tests for the syscall module, so I wrote a small
one.
@nevkontakte

This comment has been minimized.

Copy link
Contributor Author

nevkontakte commented Oct 24, 2019

Also this PR obsoletes #935 :-)

@nevkontakte

This comment has been minimized.

Copy link
Contributor Author

nevkontakte commented Oct 28, 2019

@dmitshur @hajimehoshi would you mind taking a look, please? I think this PR should be pretty straightforward to merge.

Copy link
Member

dmitshur left a comment

Thanks for working on this and sorry about the delay in review.

I left some inline comments. I am not very familiar with the details of this code, but the change looks reasonable, and as long as it compiles and syscall works with it, we should be able to proceed.

Since this PR adds support for Node 12, how do you feel about changing circle.yml to start using Node 12, so that the change is tested?

node-syscall/binding.gyp Show resolved Hide resolved
node-syscall/binding.gyp Show resolved Hide resolved
node-syscall/syscall.cc Outdated Show resolved Hide resolved
node-syscall/syscall.cc Outdated Show resolved Hide resolved
node-syscall/syscall_test.go Outdated Show resolved Hide resolved
  - Moved the syscall Go test into tests/ subdirectory.
  - Properly handled Array::Set() return values.
  - Consistent use of trailing commas in the gyp file.
Copy link
Contributor Author

nevkontakte left a comment

Thanks for working on this and sorry about the delay in review.

No worries at all. Thanks for taking the time to do the review!

I left some inline comments. I am not very familiar with the details of this code, but the change looks reasonable, and as long as it compiles and syscall works with it, we should be able to proceed.

Thanks! I've addressed the comments. The code does seem to work the best I can tell. I don't have any real-world codebase with complicated syscall usage that I could test it against, but the smoke test I added seems to pass well.

Since this PR adds support for Node 12, how do you feel about changing circle.yml to start using Node 12, so that the change is tested?

I am a bit on a fence about this. Technically, this PR doesn't add anything Node 12-specific, it only switches away from APIs which were already deprecated in Node 10 and were completely removed in Node 12 (hence the breakage). But it still works with Node 10, so it seems to make sense to continue testing against the oldest version we support to make sure we don't break it by accident. WDYT?

node-syscall/binding.gyp Show resolved Hide resolved
node-syscall/syscall.cc Outdated Show resolved Hide resolved
node-syscall/syscall.cc Outdated Show resolved Hide resolved
node-syscall/syscall_test.go Outdated Show resolved Hide resolved
Copy link
Member

dmitshur left a comment

I am a bit on a fence about this. Technically, this PR doesn't add anything Node 12-specific, it only switches away from APIs which were already deprecated in Node 10 and were completely removed in Node 12 (hence the breakage). But it still works with Node 10, so it seems to make sense to continue testing against the oldest version we support to make sure we don't break it by accident. WDYT?

Sounds good. I tested locally that it fixes the problem with Node 12. We can bump the minimum requirement later, whenever Node 10 becomes "too old" and inconvenient to support.

LGTM. Thank you for sending this change!

@dmitshur dmitshur merged commit ce3c9ad into gopherjs:master Nov 6, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@flimzy

This comment has been minimized.

Copy link
Member

flimzy commented Nov 6, 2019

Thanks for working on this!

@nevkontakte nevkontakte deleted the nevkontakte:node12 branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.