Hmm actually the package step won't do the right thing still. I'll re-open in a bit.

If you want, you can just leave PRs open, and tag them [WIP] in the title.
I was thinking we may want to try looking for the same vars as node-pre-gyp in the npm config (which will get passed down via the scripts IIRC) rather than trying to determine using which-native-nodish, which was made before any of that stuff got standardized at all.

So my first thought here is that I don't know if I like reusing the installscript for packaging, because that also will trigger a reconfiguration of libssh2, and an additional regeneration of all of the C++ and gyp files etc. Nothing should have changed, but it seems a bit unneccesary. Factoring it out into some sort of node-pre-gyp helper might be a bit more on point, but that should be a simple transition, and most of your work should just drop right over.

As an aside, any chance you can interactive-rebase or cherry-pick around that merge and just rebase this sans merge-commit on top of master?

Awesome friggin work so far man.

I was thinking we may want to try looking for the same vars as node-pre-gyp in the npm config (which will get passed down via the scripts IIRC) rather than trying to determine using which-native-nodish, which was made before any of that stuff got standardized at all.

Yeah I'd love to get a second set of 👀 on this! So far as I can tell, node-pre-gyp looks at the process's version to see if it's node-webkit or not: https://github.com/mapbox/node-pre-gyp/blob/34224b4264db52441f94d4a91ed0b2d28cadf762/lib/util/versioning.js#L252 (which doesn't make a ton of sense to me?). Otherwise it relies on the --runtime flag. So I think which-native-nodish is still a legit way of checking 👍 But I don't know a ton about this, so lemme know if I'm wrong.

So my first thought here is that I don't know if I like reusing the installscript for packaging, because that also will trigger a reconfiguration of libssh2, and an additional regeneration of all of the C++ and gyp files etc.

I agree that's gross 👍 I'll change it.

Yeah, it's trying to deduct what platform it's being run in which I don't think we can assume in a lot of build environments. I feel like forcing that in node-pre-gyp might be the way to go. We can grab that from which-native-nodish or through an argument passed in.

yeah, just factoring it out though would rock, since the --runtime figuring, and the calling out to it etc are all shared.

I need to do a little work on which-native-nodish at minimum, but it seems thats fairly non standard the way I implemented it. It currently just recurses up the tree looking for ../.., where the first parent is a node_modules, until it cant go up anymore, then looks in the root most modules package.json to see if its got electron/atom-shell or nwjs/node-webkit in it's engines. I'd like to see it pull from the root-most levels npm configuration (so an npmrc in the root folder, or the users, or the global npm installations config) programatically, since that's basically what node-pre-gyp will do automatically for most modules if my understanding of how they detect that is correct. IIRC the runtime argument works for an override. Not sure if the same holds true at buildtime however. That would super simplify things though.

Alright, I've pulled apart the functionality into distinct install and package scripts. Lemme know how you all feel about it.

As for determining runtime, lemme make sure I understand you right: node-pre-gyp can pull the runtime from npm config, so nodegit should too. That means either updating which-native-nodish or doing it with nodegit itself. Does that sound right? Do you think that's necessary for this PR?

No, I'd argue lets put that in a separate pr, but I'd like to get it out asap. Sorry, just thinking out loud with all of this since this was all sort of one big todo in my head until now lol.

I do suspect it may be possible to just have node-pre-gyp detect that automatically however, rather than have nodegit/which-native-nodish do it. If not, I'll work that into which-native-nodish so it's easy to extract out to other modules.

I think this is in pretty good shape, but I'm not sure what's going on with the Appveyor builds. Any theories?

Huh. I mean, it's only really upset at the electron version ones. Have you tried it locally on a windows machine? I can give it a shot, or rdp into appveyor if needed.

Nice 👍

So I think this is almost ready. I'd say lets clean up the commit history since it's like 30 commits of just shooting in the dark with what's going on with CI. Squash those suckers into something more succinct and then let's merge this!

So it's dying on linux consistently but tbh I'm not sure why. It looks like a seg fault since the tests just bomb out but before they bomb out there are some errors.

If we're going to be bringing in methods like these, might be best to standardize on something like lodash's modular packages.

👍

At this point it seems like we've solved a lot of the systematic problems, but we're hitting a lot of test races. So I'll poke around and see if there's anything eminently fixable.

So @maxkorp and I talked about this a bit and we think it's because the other native modules that are used are build against node headers instead of electron. That could be related to this really weird situation.

Also we're using this in GitKraken and I can confirm that the tests that are failing are indeed working in GK.

edit: at least with our crazy build process in GK.

Denied >.<

I'm starting to seriously wonder if ATOM_SHELL_RUN_AS_NODE isn't really short for ATOM_SHELL_RUN_IN_TERMINAL_BAD.

I'm going to try importing these tests into our app that we know uses nodegit successfully, so they can run in electron in a normal way.

Superseded by #804. 🤘