Building on Linux Mint 19 ATM.

Oh windows :(.

Looks like however we stood up libssh2 in NodeGit in the past utilized libssh2/win32/libssh2_config.h as the configuration file, which I think might be some sort of test fixture (I haven't looked too deeply into it). They changed that in 1.8 and reverting the use of libgcrypt for openssl seems to have done the trick on Windows. At least locally.

Windows not happy about libssh2. It might be that we need to update openssl to 1.1 now, as well.

So it's not a test fixture, but it's some weird config generated by cmake IIRC

No, I don't believe so. The file, libssh2/win32/libssh2_config.h exists before running CMake as it is part of the repository. You run CMake to generate a libssh2_config.h, but it's not the one that Windows builds have been relying on as far as I can tell. CMake will generate a libssh2_config.h inside of the bin folder, which you create during the CMake process.

Oh ok yeah, I remember that now.

I don't think the win32 one is a test fixture though, it was simply hardcoded (before we ever got our hands on it). I am vaguely recalling having to do some manual hacking on it last time we updated though :/ (take that with a grain of salt obviously)

Bumping openssl at the same time is definitely worth the effort in my experience.

Aha, yea test fixture was just a guess. I am not too sure what its actual origin / use really is, because it makes a lot of assumptions about the system.

As for updating openssl, potentially! I'm debating it. We need to update it, and that actually might help alleviate the issue we're seeing with windows there. Though that's only really a guess! I need to sit down with a debugger and see what's causing the segfault in windows. It might point me in the right direction.

It's possible there are header related issues for node 10 since node 10 exports openssl 1.1.0 and we node 6/8 are at 1.0.2 (where we are)...

I will get back to this, but probably not until the weekend. It looks like Node 10 starts LTS in October, so we should try to get this working by then.

In my extremely scientific opinion (based on a massive non-anecdotal dataset of 2 datapoints) updating openssl always alleviates the libssh issues, while bringing it's own issues (which are usually easier to solve, at least).

Although, when was the last time we updated openssl? I fear it's been a while. Last time I'd tried, it was a massive failure and even the node-core guys weren't sure what the issue was. I always pull in openssl from nodecore, as they've already tweaked some headers etc.

The curl commit is literally #1237 and the rest has overlap with my pull request #1519

@stevek-axo are actively working on this branch locally. We're currently ripping out openssl on non-electron targets. We've also found that on linux libcurl fulfills the openssl dependency for electron. On OSX, we can provide the openssl dependency statically from homebrew, and on windows sigmoid provides prebuilts which we will hoist onto the nodegit s3 bucket. We will only need those for electron builds in the electron-npg-automator though...

Please check out Conan it is a C/C++ package manager, written in Python (and Python is obviously already a dependency anyway of nodegit). You would have there an easy cross-platform way to install OpenSSL from conan-center and link against it.

Only warning, I don't know much about gyp, so I'm not sure how flexible gyp is for such purposes.

I feel pretty confident about our solution at this point - we'll see if CI agrees.TODO:

1. Include a solution to automatically pull the OpenSSL dependency for Electron builds on Windows.
2. Update the configureLibssh2 script to point to the appropriate OpenSSL distribution (NodeJS vs Electron)
   Originally, we considered throwing the Windows headers and library files onto S3 and pulling them down as part of the build process for Electron. I can take a look into Conan.

@Croydon We looked at Conan initially, but were unsure how well it would play with node-gyp. It seems like it ends up using a lot of CMake. I wonder if the inclusion of Conan would utlimately make the already complicated build system in NodeGit even more complicated. @tbranyen What do you think?

I am leaning toward the amount of trust conan provides for those binaries though.. I wonder if there's a way to get the openssl libraries without going through the rigmarole of using conan itself.

Aha, we figured out how to navigate their files. We're going to try and nab Mac OS and Windows openssl prebuilts from conan without adding conan as a dependency. Thanks @Croydon!

Aha. The libssh linking issue was the thing that prevented me from going down the rip-out-openssl road a long time ago. Beautiful work!

I definitely agree and would like to avoid a conan dependency. It's all well and great when its just on our boxes/ci, but when people need to build nodegit (new node version, different platform etc) thats ANOTHER hurdle for people getting involved.

As an aside, is the way to determine electron in node-gyp seriously checking if it's classified as iojs? That is so gross lol.

@maxkorp We spent a lot of time deliberating and searching before making that approach final. As of now, I haven't managed to find a single hook point for making the distinction. If we wanted to go a different approach, we need to have users define an env variable or something. I think for builds where a user has an npmrc defined, we can use npm to determine the disturl of the headers via npm config, but for users who are trying to debug their solutions (see https://github.com/electron/electron/blob/master/docs/tutorial/using-native-node-modules.md#manually-building-for-electron) it doesn't work quite as well.

What do you think?

Oh also, we agree that it's gross, but actual iojs is no longer in my support target, and electron seems to want to be known as iojs.

If I could find, and I would love the additional help, one works for all solution that is literally like, yes definitely it's building for electron right now that is not that, I'm all for it :)

I do remember someone in node-core saying there was a flag or such to allow you to determine if there were openssl headers bundled with node, such that even if it wasn't electron (but someone was building nodegit on some platform where they'd compiled node without exporting those headers) we'd still be able to determine that. I'll try to go through my old posts on the matter.

Hrm, no that flag was node_shared_openssl, which indicates if node was linked statically to openssl, or dynamically linked to the system openssl.

I think the issue here is going to end up being something a little (or a lottle) shitty, like, bundling openssl still, but also bundling the development headers. There's some way of passing flags into node-gyp, but I'm raking through the gyp sources right now and haven't found anything.

So it seems like the environment variable that gets set internally by npm might be available in gyp land, which should tell us what we need to know via dist-url (unless they've also passed nodedir or whatever it is that just lets them point explicitly to a set of headers on disk, but theres literally no solution for that bar listening to the npmrc target which I don't think most electron rebuildy type things set (it CAN be passed via CLI, but as it's not got a purpose outside of node-pre-gyp it's not widely used)

I think this branch is in a good place to be merged in.

Summary of Changes

• Bump libssh2 version to 1.8.0

• Remove vendored version of OpenSSL.

  Node 10 upgraded their version of OpenSSL to 1.1.0. Our main dependency on OpenSSL is a downstream through libcurl on Linux. In particular, libcurl is dynamically linked against libssl and libcrypto. On Mac, these libraries are provided by LibreSSL; on Linux, by OpenSSL. Luckily for us, these libraries are incompatible with one another. Our use case of libgit2 and libssh2 also rely on OpenSSL – mostly for essential crypto functions. As long as the provider of OpenSSL matches the version of OpenSSL NodeGit was built against, this just works. We started noticing problems on builds for which the provided OpenSSL was 1.1.0, but our builds utilized 1.0.2.

  The solution was to ensure there was an authoritative provider of OpenSSL, and for our builds of libgit2 and libssh2 to be properly linked to libssl and libcrypto. If our build target is Node, just use the headers provided by Node. Otherwise, (i.e. target is Electron):

  • Mac & Windows: Download OpenSSL headers and static binaries for libssl and libcrypto from Conan. Binaries are compiled with Clang 9 on Mac and Visual Studio 15 on Windows. These binaries will be statically linked into NodeGit as needed by libgit2 and libssh2.
  • Linux: Use system OpenSSL headers and binaries. NodeGit will be dynamically linked against libssl and libcrypto.

• Add utility scripts, discoverOpenSSLDistros.js and acquireOpenSSL.js.

  • discoverOpenSSLDistros: crawls Conan's release page for OpenSSL 1.1.0i and discovers URLS Mac and Windows builds. Creates a file vendor/openssl_distributions.json where these URLS can be statically stored as part of the repository. This script only needs to be run when the OpenSSL distributions need to be updated.
  • acquireOpenSSL: Downloads the platform-specific OpenSSL version by reading from vendor/openssl_distributions.json. It's less auto-magic than I would like. I created stubs for detecting things such as whether we are building for debug or figuring out which compiler version we're using. Right now, it's effectively pulling macOS-clang-9-static-release, win32-vs14-static-release, and win64-vs14-static-release.

• The naming convention I used to label versions was <os_name>-<compiler_version>-<binary_type>-<release_type>. (Binary type refers to static vs dynamic (only static is used) and release type refers to debug vs release builds.)

• Move the configureLibssh2.js out of lifecycleScripts (and subsequently the install script) into the utils directory. Instead of running during the
• Add acquireOpenSSL and configureLibssh2 steps to the binding.gyp.
  • acquireOpenSSL will conditionally run the acquireOpenSSL.js script if we detect that we're building for Electron, but will no-op otherwise.
  • configureLibssh2 will run the configureLibssh2.js script, which figures out which headers libssh2 should build against. If we are building for Node, it should use the provided OpenSSL headers from Node. Otherwise, utilize the headers that are helpfully provided by acquireOpenSSL.
• Numerous small code fixups to ensure that we compile against Node 10 and OpenSSL 1.1.0

Ideas for Future Work:

• Test suite for running tests in a non-standard Node environment, i.e. Electron
• Improve the acquireLibOpenSSL.js script to pull down debug releases if the --debug flag was passed to node-gyp.
• Improve the acquireLibOpenSSL.js script to pull down releases compiled with the same compiler installed on the system.

Notes:

There is a bit of wackiness in the node-gyp process for the configureLibssh2 and acquireOpenSSL steps. node-gyp, in its benevolence, assumes that all outputs of targets are binaries. Binaries we want to link against, apparently. So if you see output such as SOLINK_MODULE(target) Release/acquireOpenSSL.node, this is an unintended but harmless side effect of using targets and actions to run the Node scripts.

In order to make configureLibssh2.js play nice for both Mac and Linux builds, I had to add an additional condition. Previously, current environment variables are copied over into the child process when we call the vendor/libssh2/configure script, and we set the CPPFLAGS to include our OpenSSL headers. This is still the case for Linux builds. Mysteriously, this no longer works for Mac builds. It only works when we don't copy the other environment variables over, i.e. when only the CPPFLAGS variable is set. I'm still not entirely sure why this is the case.

That is some seriously impressive work. Nice job! I think folks are gonna be ecstatic to see this merged in!

I have crashed my browser 3 times trying to look at the code lol.

Can you add the files that get created during the libssh2 configure to the .gitignore across the different platforms? @stevek-axo

@stevek-axo it looks like ramda, request-promise-native, and tar-fs are part of the gyp process now. They will need to be moved into the dependencies group, because if anyone uses npm install --production, node-gyp will fail.

@stevek-axo the formatting in utils/README.md is off underneath the TODO in the acquireOpenSSL details.

https://github.com/implausible/nodegit/blob/feature/node-10-support/utils/README.md

You also spelled system wrong on line 21.

In utils/discoverOpenSSLDistros.js, can you replace:

const getDistributionConfigURLFromHash = itemHash =>
  `https://bintray.com/conan-community/conan/download_file?file_path=conan%2FOpenSSL%2F1.1.0i%2Fstable%2Fpackage%2F${itemHash}%2Fconaninfo.txt`

with

const getDistributionConfigURLFromHash = itemHash =>
  `https://dl.bintray.com/conan-community/conan/conan/OpenSSL/1.1.0i/stable/package/${itemHash}/conaninfo.txt`;

And the getDistributionsRootURL that you specified
https://bintray.com/package/files/conan-community/conan/OpenSSL%3Aconan?order=asc&sort=name&basePath=conan%2FOpenSSL%2F1.1.0i%2Fstable%2Fpackage&tab=files

Is actually easier to parse via https://dl.bintray.com/conan-community/conan/conan/OpenSSL/1.1.0i/stable/package/

@stevek-axo We should be consistent with our compiler versions in our CI environment. I know vs15 looks like msvs2015, but it's actually msvs2017. We're currently targeting msvs2015, so you'll want to change your windows target to vs14 in acquireOpenSSL.js

I'd say this looks pretty darn good.

God help us

I don't really catch your meaning. This PR eliminated the complicated process of building openssl in favor of utilizing node, system libs, or prebuilts from conan as providers of openssl. I think this is a huge win for build times and ease of openssl upgrades. Imagine not having to nab and translate gyp files from nodejs every time we want to update!

Oh it's a HUGE win. I was just saying, we've changed the build. We're batting a perfect 0 historically for build changes not breaking weird edge cases down the line is all I'm saying lol.

Aha, yea that's so true. NodeGit is so god damn complicated.

@implausible some day we'll be porting this to WASM and that will either make things 🌈 or 🌋

@tbranyen

How is threading support so far for WASM?

Also, just as an update on this big ordeal, we're currently testing the electron side of things for 2.0.9 in GitKraken. Everything seems to be going really well on that side of things!! How exciting 😄

So whatever broke is somewhere cold and dank, where old things lie and which make men tremble.

In all seriousness, that is great news.