Prebuilt binaries

[spmiller]

Hello,

Thanks for your efforts on this library! We find it fills an important part of our post-mortem toolkit, and we're particularly grateful for the work @paulrutter has done in working with the Node and V8 communities to get this implemented.

We have the pleasure of deploying on both Windows and Linux, and as this library needs a native component, we currently need to run an npm install on both OSes to get a suitable distribution.

Would you be interested in releasing pre-built binaries via node-pre-gyp? I would be happy to have a go at contributing a patch to support this via Travis / Appveyor.

[paulrutter]

Hi @spmiller,

Thanks for the appreciation!
I would certainly be interested in pre-built binaries; i'm not familiar with how this process would/should work, so i would need to investigate this further.
A patch/PR would really help here!

Let me know if you need help/review code 👍
TravisCI is already used, so that might help.

See https://travis-ci.org/github/blueconic/node-oom-heapdump

[spmiller]

Great, I'll start on this first thing Monday. It will be a learning exercise for me too!

[spmiller]

Hi @paulrutter, I opened #11 for this. I've tested this on my fork, and it seems to work pretty well on both Linux and Windows (though I haven't a MacOS machine to test with).

This will only work if we create a Github release for each release, which I don't think is part of your current workflow (we need to store the binaries somewhere and Github seems as good a place as any). I can have a go at creating another Github action to automatically create a Github release whenever a release tag is created if you would like to keep your workflow the same?

Edit: I must have been blind, you are creating releases. In that case I don't think any further changes are required.

[paulrutter]

Hi @spmiller,

Thanks for your quick work! We indeed create Github releases for each release, with a version tag.
I'm in the middle of finishing a release sprint, after that i'll have more time to review the PR.

[spmiller]

Sounds good. Best of luck for your sprint!

[spmiller]

Thanks for merging! Perhaps a beta release first to try it out?

[paulrutter]

The action has run successful, but i'm not seeing the artifacts on https://github.com/blueconic/node-oom-heapdump/releases/tag/1.3.0. Could this be because it's a pre-release?

[spmiller]

Ugh, I can't see any documentation that says prereleases can't have assets uploaded. I'll try debugging on my fork. Sorry about this ☹️

[paulrutter]

Not a problem, even when 1.3.0 is used (npm install node-oom-heapdump) will fallback to building from source.
So nothing is broken ATM.

[paulrutter]

The weird thing is:

https://github.com/blueconic/node-oom-heapdump/runs/803163441?check_suite_focus=true

Vs your fork:

https://github.com/spmiller/node-oom-heapdump/runs/803237340?check_suite_focus=true

See the difference in output in the upload task.

[paulrutter]

I think i found it; the PR contained the "remove_path" in the package. After it was removed, the binaries appeared.
It looks like it's working now:

PS C:\temp\prebuilt> npm install node-oom-heapdump                                                                      
> gc-stats@1.4.0 install C:\temp\prebuilt\node_modules\gc-stats
> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using needle for node-pre-gyp https download
node-pre-gyp WARN Tried to download(403): https://node-binaries.s3.amazonaws.com/gcstats/v1.4.0/Release/node-v72-win32-x64.tar.gz
node-pre-gyp WARN Pre-built binaries not found for gc-stats@1.4.0 and node@12.18.0 (node-v72 ABI, unknown) (falling back to source compile with node-gyp)
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  gcstats.cc
  win_delay_load_hook.cc
     Creating library C:\temp\prebuilt\node_modules\gc-stats\build\Release\gcstats.lib and object C:\temp\prebuilt\node
  _modules\gc-stats\build\Release\gcstats.exp
  gcstats.vcxproj -> C:\temp\prebuilt\node_modules\gc-stats\build\Release\\gcstats.node
  Copying C:\temp\prebuilt\node_modules\gc-stats\build\Release\/gcstats.node to C:/temp/prebuilt/node_modules/gc-stats/
  build/gcstats/v1.4.0/Release/node-v72-win32-x64\gcstats.node
          1 file(s) copied.

> node-oom-heapdump@1.3.0 install C:\temp\prebuilt\node_modules\node-oom-heapdump
> node-pre-gyp install --fallback-to-build

node-pre-gyp WARN Using needle for node-pre-gyp https download
[node-oom-heapdump] Success: "C:\temp\prebuilt\node_modules\node-oom-heapdump\build\Release\node_oom_heapdump_native.node" is installed via remote

+ node-oom-heapdump@1.3.0
added 141 packages from 37 contributors and audited 141 packages in 10.013s

1 package is looking for funding
  run `npm fund` for details

found 3 low severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details

Too bad the gc-stats is not having all the right binaries in place, still falling back to building from source though.

[paulrutter]

Closing as fixed.
Thanks @spmiller!

[spmiller]

Thanks for debugging @paulrutter!

I think i found it; the PR contained the "remove_path" in the package. After it was removed, the binaries appeared.

Hmm, this doesn't make sense to me -- I had thought that remote_path was only used by node-pre-gyp to download the binaries. It shouldn't have had any effect on the upload action in the workflow?

I am slightly concerned that the upload step is unreliable when working with pre-releases. I'll keep playing in my fork and I'll see if I can get to the bottom of it.

Too bad the gc-stats is not having all the right binaries in place, still falling back to building from source though.

At least submitting a PR for them shouldn't take me as much time this time round :)

[paulrutter]

Hmm, that's a bit concerning indeed. I tested a pre-release in your forked repo, which worked fine.
I read a bit more about Github Actions, and only found parameters to disable it for pre-releases. This would mean that it, by default, works the same as for regular releases.

A PR for gc-stats would be nice.
Let me know if you have any more info on the subject.

[spmiller]

Hey @paulrutter, I've been playing around in my fork and I can't reproduce the borked release that happened here. I hope it was just a once-off; if we change the workflow to trigger when the release is edited as well as published then it would give it several chances to correct itself it it turns out to be unreliable.

It's worth noting that I'm using this release asset action instead of the official one because the official one doesn't support dynamically generated filenames (#4, #9. When one of those are implemented we should switch over as presumably it will be more reliable.

Re gc-stats, I tagged you on a comment there. They already have binaries being created, they just had an issue with one particular build. When gcstats@1.5.0 is released on NPM it will have all the binaries we need.

We've been using the new version of node-oom-heapdump here and it works great. Thanks again for your help and support with this!

[paulrutter]

Thanks for the update 👍
Glad it works for you!