Skip to content

Failed to compile on Node 10.0.0 #1490

Closed
Closed
Failed to compile on Node 10.0.0#1490

Description

@sadasant
sadasant
opened on Apr 30, 2018

nodegit fails to compile on Node 10.0.0 and NPM 5.6.0. Here's a way to reproduce the issue:

cd ~
mkdir test_repo
cd test_repo
npm init
npm i --save nodegit

Here are my logs:

https://gist.github.com/sadasant/4fce501d8cffda53bc8fb53f4950eaa3

Also happens with NPM 6.0.0:

https://gist.github.com/sadasant/cf598e5d232be909e426135a3de87847

May 1 update

Even though nodegit's issue is deeper that what I'm about to mention, the following issue is the first breaking point for building nodegit on Node 10:

There was a recent change on Node 10's Function's toString method. Here's an issue that I created: nodejs/node#20459

This change in Node 10 causes promisify-node to break, here's the relevant issue: nodegit/promisify-node#28

May 2 update

The exception happens when we try to run node node_modules/.bin/node-pre-gyp install --build-from-source. The issue seems to be that libssh2 is expected to be built using openssl-0.9.8: https://github.com/libssh2/libssh2/search?utf8=%E2%9C%93&q=openssl-0.9.8&type= However, node 10 moved openssl forward to 1.1.0: https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=OpenSSL+has+been+updated+to+1.1.0h.&type=

Here are my logs: https://gist.github.com/sadasant/031d3f8bf1c4e5d2ac83b3841b7807fc

Activity

  1. sadasant commented on May 2, 2018

    @sadasant
    sadasant
    on May 2, 2018
    Author

    So far, the issues seem to be:

    • promisify-node having a faulty RegExp.
    • nodegit's dependency on libssh2, which expects to be built on top of openssl 0.9.8, when Node 10 moved to openssl 1.1.0.

  2. sadasant commented on May 2, 2018

    @sadasant
    sadasant
    on May 2, 2018
    Author

    The relevant issues are:

  3. DanielRuf commented on May 7, 2018

    @DanielRuf
    DanielRuf
    on May 7, 2018

    Same here.

  4. RobinGeuze commented on May 9, 2018

    @RobinGeuze
    RobinGeuze
    on May 9, 2018

    For the libssh2 part an upgrade to version 1.8.0 of the library should fix it. That version is listed on their site and is tagged in github, but it doesn't have a github release for some reason.

  5. MartinHelmut
    added 2 commits that reference this issue on May 13, 2018

    ci: for the time being, latest node build are deactivated

    9f63e74

    ci: for the time being, latest node build are deactivated

    c333396

  6. getify commented on May 19, 2018

    @getify
    getify
    on May 19, 2018

    Many of us have upgraded to node10 and now this is a huge blocker. Please increase the priority of fixing this build failure.

  7. laomaiweng commented on May 31, 2018

    @laomaiweng
    laomaiweng
    on May 31, 2018

    libssh2 should build fine with nodegit's bundled OpenSSL 1.0.2.

    However with Node 10 and OpenSSL 1.1.0, the build system fails to pick up the bundled OpenSSL 1.0.2 headers when building libssh2, and ends up using node-gyp's OpenSSL 1.1.0 headers in ~/.node-gyp/10.1.0/include/node/openssl. This is because node-gyp seems to prepend -I $(HOME)/.node-gyp/10.1.0/include/node to the CFLAGS when building, ignoring the -I ../vendor/openssl/openssl/include added by the nodegit build system.

    If we could somehow instruct node-gyp to actually prepend -I ../vendor/openssl/openssl/include to the include dirs, before -I $(HOME)/.node-gyp/…, then libssh2 would pick up the compatible 1.0.2 headers. However I tried with include_dirs+ as suggested in the node-gyp reference, without luck. :(

  8. tbranyen commented on Jun 5, 2018

    @tbranyen
    tbranyen
    on Jun 5, 2018
    Member

    I've resolved the promisify-node blocker, and will investigate the compiler issue later today.

  9. tbranyen commented on Jun 5, 2018

    @tbranyen
    tbranyen
    on Jun 5, 2018
    Member

    @laomaiweng that definitely is the fix for this particular issue. I tried using include_dirs! which looks like it should override the includes completely, but does not. I moved this block to the root of our GYP file and it still does not take. I have moved past this issue, by forking node-gyp and conditionally setting include_dirs behind a variable.

    The next issue was with ToInt32 not being in v8 anymore. I've moved past that issue using Nan and now have a compiled version against Node 10. Here's what happens if I try and require it:

    λ node
> require('.')
zsh: segmentation fault (core dumped)  node

    So now the fun times of figuring out why....

  10. tbranyen commented on Jun 5, 2018

    @tbranyen
    tbranyen
    on Jun 5, 2018
    Member
    λ valgrind node -e 'require(".")'
==28122== Memcheck, a memory error detector
==28122== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==28122== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==28122== Command: node -e require(".")
==28122== 
==28122== Warning: set address range perms: large range [0x4cb4ce34000, 0x4cb6ce34000) (noaccess)
==28122== Invalid write of size 4
==28122==    at 0x5D63810: SSL_CTX_set_verify (in /usr/lib/libssl.so.1.1)
==28122==    by 0xD4258E3: git_openssl_stream_global_init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0xD3FFDAF: init_once (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0x7508DEE: __pthread_once_slow (in /usr/lib/libpthread-2.26.so)
==28122==    by 0xD3FFE92: git_libgit2_init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0xD1FC079: init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0x568ABC: ??? (in /usr/bin/node)
==28122==    by 0x72A4AF: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) (in /usr/bin/node)
==28122==    by 0x78280F: ??? (in /usr/bin/node)
==28122==    by 0x782BE6: ??? (in /usr/bin/node)
==28122==    by 0x4CB4CE8427C: ???
==28122==    by 0x4CB4CE944F6: ???
==28122==  Address 0x140 is not stack'd, malloc'd or (recently) free'd
==28122== 
==28122== 
==28122== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==28122==  Access not within mapped region at address 0x140
==28122==    at 0x5D63810: SSL_CTX_set_verify (in /usr/lib/libssl.so.1.1)
==28122==    by 0xD4258E3: git_openssl_stream_global_init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0xD3FFDAF: init_once (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0x7508DEE: __pthread_once_slow (in /usr/lib/libpthread-2.26.so)
==28122==    by 0xD3FFE92: git_libgit2_init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0xD1FC079: init (in /home/tbranyen/git/nodegit/build/Release/nodegit.node)
==28122==    by 0x568ABC: ??? (in /usr/bin/node)
==28122==    by 0x72A4AF: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) (in /usr/bin/node)
==28122==    by 0x78280F: ??? (in /usr/bin/node)
==28122==    by 0x782BE6: ??? (in /usr/bin/node)
==28122==    by 0x4CB4CE8427C: ???
==28122==    by 0x4CB4CE944F6: ???

  11. tbranyen commented on Jun 5, 2018

    @tbranyen
    tbranyen
    on Jun 5, 2018
    Member

    Judging by /usr/lib/libssl.so being listed in that trace, I'm guessing our statically linked vendor deps are not being respected and this may be causing the issue.

  12. tbranyen commented on Jun 5, 2018

    @tbranyen
    tbranyen
    on Jun 5, 2018
    Member

    Tonight I'm going to try and use a GYP file for libssh2 like (https://github.com/peters/curl-for-windows/blob/master/libssh2.gyp) and mark it as a dependency to the nodegit target.

  13. implausible commented on Jun 6, 2018

    @implausible
    implausible
    on Jun 6, 2018
    Member

    @tbranyen Take a look at what I did here, this might speed up the process https://github.com/implausible/nodegit/blob/remove/openssl/vendor/libgit2.gyp#L483

  14. lenovouser commented on Jun 21, 2018

    @lenovouser
    lenovouser
    on Jun 21, 2018

    Any progress?

  15. 11 remaining items

  16. tbranyen commented on Sep 19, 2018

    @tbranyen
    tbranyen
    on Sep 19, 2018
    Member

    Testing this out, and if true, I'll ensure it gets merged today.

  17. tbranyen commented on Sep 19, 2018

    @tbranyen
    tbranyen
    on Sep 19, 2018
    Member

    Okay so here's what I see. This does in fact get the build to work, excellent and minimal set of steps to do it too! But, I'm seeing a number of test failures. From what I can tell they are all related to promisify-node with the exception: Error: Callback is required and must be a Function.. So we'll need to resolve those errors first.

    Then there's the whole appveyor build passing we'll need to get through, and hopefully after all that we'll have a stable Node 10 version.

  18. Croydon
    added a commit that references this issue on Sep 19, 2018

    Patch LockMasterSetStatus

    e56b722

  19. tbranyen commented on Sep 19, 2018

    @tbranyen
    tbranyen
    on Sep 19, 2018
    Member

    @Croydon do you wanna open the PR? I could help you with it? I see you're already working on something.

    Edit: Nevermind found it and it's referenced above: #1519

  20. Croydon
    added 2 commits that reference this issue on Sep 19, 2018

    Patch LockMasterSetStatus

    7f4fd05

    Patch LockMasterSetStatus

    5547fc1

  21. Croydon commented on Sep 20, 2018

    @Croydon
    Croydon
    on Sep 20, 2018
    Contributor

    @myshark On what platform(s) did you try it? AppVeyor (Windows) keeps failing (see my pull request)

  22. Croydon
    added a commit that references this issue on Sep 20, 2018

    Patch LockMasterSetStatus

    e14aa05

  23. mehdi-yeganeh commented on Sep 20, 2018

    @mehdi-yeganeh
    mehdi-yeganeh
    on Sep 20, 2018

    @Croydon, Wow you are so fast guys, I cloned https://github.com/Croydon/nodegit.git and tested it on MacOS (Sierra 10.12.6) and CentOS (7) both is ok and worked. If you need some help, i`m here.

  24. Croydon
    added 3 commits that reference this issue on Sep 25, 2018

    Patch LockMasterSetStatus

    fa48c61

    Patch LockMasterSetStatus

    1b65147

    Patch LockMasterSetStatus

    63e31e6
  25. implausible
    closed this as completedin #1519on Oct 2, 2018
  26. MartinHelmut
    added a commit that references this issue on Feb 4, 2019

    ci: for the time being, latest node build are deactivated

    a9eae5d

  27. smithmr commented on Jun 17, 2019

    @smithmr
    smithmr
    on Jun 17, 2019

    I had this issue for a couple of days and fixed it by running this npm config set python {path to python2.7}. I was running with node 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions