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

Failed to compile on Node 10.0.0 #1490

Closed
sadasant opened this issue Apr 30, 2018 · 29 comments
Closed

Failed to compile on Node 10.0.0 #1490

sadasant opened this issue Apr 30, 2018 · 29 comments

Comments

@sadasant
Copy link

sadasant commented 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

@sadasant
Copy link
Author

sadasant commented May 2, 2018

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.

@sadasant
Copy link
Author

sadasant commented May 2, 2018

@DanielRuf
Copy link

Same here.

@RobinGeuze
Copy link

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.

MartinHelmut added a commit to MartinHelmut/berries that referenced this issue May 13, 2018
This issue is related to the dependency nodegit. Related issue is nodegit/nodegit#1490
MartinHelmut added a commit to MartinHelmut/berries that referenced this issue May 13, 2018
This issue is related to the dependency nodegit. Related issue is nodegit/nodegit#1490
@getify
Copy link

getify commented 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.

@laomaiweng
Copy link

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. :(

@tbranyen
Copy link
Member

tbranyen commented Jun 5, 2018

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

@tbranyen
Copy link
Member

tbranyen commented Jun 5, 2018

@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....

@tbranyen
Copy link
Member

tbranyen commented Jun 5, 2018

λ 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: ???

@tbranyen
Copy link
Member

tbranyen commented Jun 5, 2018

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.

@tbranyen
Copy link
Member

tbranyen commented Jun 5, 2018

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.

@implausible
Copy link
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

@lenovouser
Copy link

Any progress?

@DanielRuf
Copy link

@lenovouser probably not but did you find further infos?

@tbranyen
Copy link
Member

@lenovouser I'll take another shot at it at some point, but I don't have the time at the moment to put any more effort into this. Hopefully someone else can take a stab in the meanwhile.

@barhoumio
Copy link

👍 👍 👍 👍 👍

@mschwartz
Copy link

+1

@ylecuyer
Copy link

At least you could add a warning somewhere to inform this is not compatible with node10

@mehdi-yeganeh
Copy link

mehdi-yeganeh commented Sep 19, 2018

Instruction of building nodegit with node 10.10.0: (I hope its be useful)

Pre build:
You should know enough about building nodegit in previous node versions and before building node git you should install some dependencies like node-gyp, node-pre-gyp, some crypt library and ...

Build:

  1. Clone the latest version of nodegit
  2. Open package.json and make sure promisify-node is ~0.5.0 and libssh2 is 1.8.0
"dependencies": {
    "fs-extra": "~0.27.0",
    "lodash": "^4.13.1",
    "nan": "^2.10.0",
    "node-gyp": "^3.6.2",
    "node-pre-gyp": "~0.6.39",
    "promisify-node": "~0.5.0"
  },
"vendorDependencies": {
    "libssh2": "1.8.0",
    "http_parser": "2.5.0"
  },
  1. Replace vendor/libssh2 with latest one from libssh2-1.8.0
  2. Open below file with an editor and change LockMasterSetStatus function as below:

generate/templates/templates/nodegit.cc

void LockMasterSetStatus(const FunctionCallbackInfo<Value>& info) {
  Nan::HandleScope scope;

  // convert the first argument to Status
  if(info.Length() >= 0 && info[0]->IsUint32()) {
    uint value = info[0]->Uint32Value();

    LockMaster::Status status = static_cast<LockMaster::Status>(value);
    if(status >= LockMaster::Disabled && status <= LockMaster::Enabled) {
      LockMaster::SetStatus(status);
      return;
    }
  }

  // argument error
  Nan::ThrowError("Argument must be one 0, 1 or 2");
}
  1. Now, You can run

npm install

@DanielRuf
Copy link

@myshark is this available as PR and already merged?

@tbranyen
Copy link
Member

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

@tbranyen
Copy link
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.

Croydon added a commit to Croydon/nodegit that referenced this issue Sep 19, 2018
@tbranyen
Copy link
Member

tbranyen commented Sep 19, 2018

@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

Croydon added a commit to Croydon/nodegit that referenced this issue Sep 19, 2018
Croydon added a commit to Croydon/nodegit that referenced this issue Sep 19, 2018
@Croydon
Copy link
Contributor

Croydon commented Sep 20, 2018

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

Croydon added a commit to Croydon/nodegit that referenced this issue Sep 20, 2018
@mehdi-yeganeh
Copy link

mehdi-yeganeh commented 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.

Croydon added a commit to Croydon/nodegit that referenced this issue Sep 25, 2018
Croydon added a commit to Croydon/nodegit that referenced this issue Sep 26, 2018
Croydon added a commit to Croydon/nodegit that referenced this issue Sep 27, 2018
MartinHelmut added a commit to MartinHelmut/berries that referenced this issue Feb 4, 2019
This issue is related to the dependency nodegit. Related issue is nodegit/nodegit#1490
@smithmr
Copy link

smithmr commented 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
Labels
None yet
Projects
None yet
Development

No branches or pull requests