Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

upgrade and add support for Node 10.2.0 #113

Closed
wants to merge 27 commits into from
Closed

Conversation

shiftkey
Copy link
Contributor

@shiftkey shiftkey commented Jun 26, 2018

Electron has made the jump to Node 10.2.0 with their 3.0 beta, so I think it's time to figure out how to add that to the matrix for keytar.

Currently when I try and build without the benefit of prebuilt binaries (on macOS at least) I get this:

error /Users/shiftkey/src/desktop/app/node_modules/keytar: Command failed.
Exit code: 1
Command: node-gyp rebuild
Arguments:
Directory: /Users/shiftkey/src/desktop/app/node_modules/keytar
Output:
CXX(target) Release/obj.target/keytar/src/async.o
In file included from ../src/async.cc:3:
In file included from ../../nan/nan.h:190:
../../nan/nan_maybe_43_inl.h:112:15: error: no member named 'ForceSet' in 'v8::Object'
  return obj->ForceSet(isolate->GetCurrentContext(), key, value, attribs);
         ~~~  ^
In file included from ../src/async.cc:3:
../../nan/nan.h:832:18: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
    return node::MakeCallback(
                 ^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/node.h:93:1: note: 'MakeCallback' has been explicitly marked deprecated here
NODE_DEPRECATED("Use MakeCallback(..., async_context)",
^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/core.h:35:20: note: expanded from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                   ^
In file included from ../src/async.cc:3:
../../nan/nan.h:847:18: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
    return node::MakeCallback(
                 ^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/node.h:86:1: note: 'MakeCallback' has been explicitly marked deprecated here
NODE_DEPRECATED("Use MakeCallback(..., async_context)",
^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/core.h:35:20: note: expanded from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                   ^
In file included from ../src/async.cc:3:
../../nan/nan.h:862:18: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
    return node::MakeCallback(
                 ^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/node.h:79:1: note: 'MakeCallback' has been explicitly marked deprecated here
NODE_DEPRECATED("Use MakeCallback(..., async_context)",
^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/core.h:35:20: note: expanded from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                   ^
In file included from ../src/async.cc:3:
../../nan/nan.h:1471:31: warning: 'MakeCallback' is deprecated: Use MakeCallback(..., async_context) [-Wdeprecated-declarations]
    return scope.Escape(node::MakeCallback(
                              ^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/node.h:93:1: note: 'MakeCallback' has been explicitly marked deprecated here
NODE_DEPRECATED("Use MakeCallback(..., async_context)",
^
/Users/shiftkey/.node-gyp/iojs-3.0.0-beta.1/src/core.h:35:20: note: expanded from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                   ^
4 warnings and 1 error generated.
make: *** [Release/obj.target/keytar/src/async.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at emitTwo (events.js:126:13)
gyp ERR! stack     at ChildProcess.emit (events.js:214:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)
gyp ERR! System Darwin 17.6.0
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/shiftkey/src/desktop/app/node_modules/keytar
gyp ERR! node -v v8.11.2
gyp ERR! node-gyp -v v3.6.2

Goals for this PR:

  • upgrade to latest nan and node-gyp
  • add 10.2.0 to matrix of prebuilt versions
  • ensure AppVeyor build is still fine
  • ensure Travis build is still fine
  • figure out why Linux agent still complains about build_v8_with_gn not being set
  • add context for moving to AsyncResource

cc @vanessayuenn as we were talking about this yesterday

@daviwil
Copy link
Contributor

daviwil commented Jun 26, 2018

I've seen some issues prebuilding another module for Node 10 because of changes to how gyp works, curious to see how this goes for keytar.

@daviwil
Copy link
Contributor

daviwil commented Jun 26, 2018

Yep, this is what I've seen before:

gyp: name 'build_v8_with_gn' is not defined while evaluating condition 'build_v8_with_gn == "true"' in binding.gyp while trying to load binding.gyp

I tried to get around it with this workaround (more info here) but ultimately I just had to make the CI machines run Node 10 for it to work.

@shiftkey
Copy link
Contributor Author

@daviwil I think that's a good motivator to upgrade the CI agents to Node 10 - we've got prebuild to target all the different versions of V8 that we need to.

@daviwil
Copy link
Contributor

daviwil commented Jun 26, 2018

Yep, I think it's a safe bet to do that.

@shiftkey shiftkey force-pushed the upgrade-to-latest-nan branch from 42a5be1 to 9376f62 Compare June 26, 2018 19:30
.travis.yml Outdated
- npm install -g [email protected]
- |
if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then
node-gyp configure -- -Dbuild_v8_with_gn=false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm officially stumped as to how to set this correctly.

@daviwil @vanessayuenn any ideas aside from this workaround?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i backfilled the variable in binding.gyp instead so at least CI stopped complaining about lack of build_v8_with_gn, but now it's failing at /usr/include//c++/4.8/cstdio:120:11: error: no member named 'gets' in the global namespace.

@vanessayuenn
Copy link

vanessayuenn commented Jun 29, 2018

@shiftkey the docker image we're currently using to build for ia32 uses node 8, and according to this comment, node foundation isn't making 32bit Intel builds anymore starting at 10.x. 😕 Unsure if it's related to this error in Travis, however:

make: Entering directory `/project/build'
  CXX(target) Release/obj.target/keytar/src/async.o
In file included from ../src/async.cc:1:
In file included from /usr/include//c++/4.8/string:52:
In file included from /usr/include//c++/4.8/bits/basic_string.h:2815:
In file included from /usr/include//c++/4.8/ext/string_conversions.h:43:
/usr/include//c++/4.8/cstdio:120:11: error: no member named 'gets' in the global namespace
  using ::gets;
        ~~^
1 error generated.
make: *** [Release/obj.target/keytar/src/async.o] Error 1

@shiftkey
Copy link
Contributor Author

shiftkey commented Jul 3, 2018

@vanessayuenn @daviwil so I looked into what it would take to ship our own Dockerfile for testing, and I crimped a bunch of stuff from the current Alpine image for Node 10 but I decided to not use Alpine here because Alpine is based off musl libc, rather than glibc.

FROM i386/node:10-alpine

RUN apk add --update \
    python \
    python-dev \
    make \
    g++ \
    libsecret \
    libsecret-dev

Alpine is great for running services (very small footprint and image size, nice packaging system for pulling in additional stuff), but I think for our situation and building native modules we should stick to glibc-based distros, so I've reverted to Debian and now have a build image that builds Node from source for i386 (as it's no longer supported).

This was more for my curiosity and learning more about this stuff, and if you both think it's overkill (it makes Linux CI now take ~45 minutes to build a new image - worst case scenario - and I'm not sure how to cache these so Travis is more efficient with subsequent builds) an alternative is to follow the upstream guidance to drop prebuild support on x86 Linux for 10.* and leave it up to consumers to manage their own toolchains.

@shiftkey
Copy link
Contributor Author

shiftkey commented Jul 3, 2018

I had a chat with Vanessa about this PR and I think we're in agreement about following upstream and dropping support for 32-bit Linux on 10.x. I'm going to close this PR out and split this up into 2 different PRs now that I know more about these internals and the way forward:

  • upgrade tooling to Node 9.x, no changes to supported versions
  • add support for Node 10.x on supported platforms

@shiftkey shiftkey closed this Jul 3, 2018
@shiftkey shiftkey deleted the upgrade-to-latest-nan branch July 9, 2018 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants