feat: add x64-debug build recipe

[matthewkeil]

Hello and thank you for your consideration. I am part of the ChainSafe/lodestar team and we are heavy users of both node and native dependencies. We often have segfaults pop up on our CI through normal course of business and having Node.js built with Debug symbols makes life MUCH easier to debug issues.

We use the Github workflow action actions/setup-node and would like to add a "debug" option to that to have it pull and install a Debug build of node. We have built a fork that creates these releases for ourselves ChainSafe/node_debug but as active contributors to the open source community would like to offer this PR as an addition to the unofficial-builds repo so they everyone in the community can benefit from them as much as we do.

If the images can get built here we will PR our actions/setup-node action back to the main repo so that the binaries are still being served by the node organization instead of our repo. Not sure this is critical but thought it would be more "official" even though its hosted in the "unofficial-builds". A penny for your thoughts on that?

This is my first time contributing to this repo and think I have crossed all my t's and dotted all of my i's but would love some feedback to make sure that it meets standards.

Thanks for the opportunity to give back to the community!

@matthewkeil

[rvagg]

Hi @matthewkeil, would you mind taking a look at #108 and rebasing on that branch and then trying to run this locally to see if you can get the binary that you want.

[matthewkeil]

Hi @matthewkeil, would you mind taking a look at #108 and rebasing on that branch and then trying to run this locally to see if you can get the binary that you want.

Rebased and am running the recipe now to test @rvagg

Left a note in the comments on #108 that I had issues with $(id -u) and had to revert them to 1000 to get the local_build.sh to run.

Also strangely I had to update the arch/destcpu to arm64 to get it to build because I am on a mac and was getting weird unknown flag (-m64). When I swapped to arm64 the build started running fine. I also removed the --gdb flag for the test run because that is not supported on mac either. Is weird that would come up though with the build running in a container....

I also added a PR nodejs/nodejs-dist-indexer#25 per the docs note

[matthewkeil]

@rvagg I updated one thing and verified the build is correct now. Ready for review

[matthewkeil]

@nazarhussain thanks for the idea to find this repo and build this! 🎸

[rvagg]

@nodejs/build would like to hear some feedback on the general idea here; the recipe seems fine to me but I'm not sure what folks are doing these days for debug builds or even how commonly they're used and recommended (I'm a bit out of the loop!). I buy the general case here and do recall the hassles of not having debug symbols when developing native addons, but do we have current documentation that suggest an alternative approach here? i.e. does this pass the bar of being reasonably useful to a non-trivial amount of the ecosystem and is doing it here going to be significantly easier than current approaches (e.g. if the current approach is just "build it yourself", then the answer to that part is probably yes).
@nodejs/addon-api

[rvagg]

(background note: we run this all on a single server, building serially when there's a new release, we can't just throw an endless number of builds into this thing so we have to be a little bit picky)

[matthewkeil]

@nodejs/build would like to hear some feedback on the general idea here; the recipe seems fine to me but I'm not sure what folks are doing these days for debug builds or even how commonly they're used and recommended (I'm a bit out of the loop!).

Sure @rvagg and @nodejs/build! We have run into a number of issues within native code. The one that proved most problematic was in node core itself (worker.cc). nodejs/node#51129 was a segfault that kept popping up for us, but frustratingly it only happened on CI (github runners) and was notoriously difficult to diagnose until we had a reliable native stack trace in the core dump.

We also are heavy users of LevelDB (and contributors Level/classic-level#85) and have found issues over the years. In particular there was a memory leak for batch processing. Once we figured out where it was coming from we found this issue (which funny enough I just looked at again and you are part of @rvagg lol!!! You are a prolific contributor!!!). Trying to track this issue down and fix it is one of my to-do's but finishing QUIC is higher up on the contribution list.

Building an Ethereum Client for the Ethereum Foundation is most definitely pushing the boundary of what is possible with the JS virtual machine. Over the years we have had to build our native-code practices as it powers much of our stack. In several cases we were challenged with where the code was breaking down (the Worker segfault that Ben helped us with is a great example) that we were able to identify and post to node core. As a note we found that bug and Ben is a consultant for our team at times. He is a rockstar and was gracious enough to help post the issue that we identified.

I buy the general case here and do recall the hassles of not having debug symbols when developing native addons, but do we have current documentation that suggest an alternative approach here?

As for how often debug builds are used, I cannot speak for other teams but we tend to use them regularly for a few things. In particular:

• core dump analysis (honestly the most important reason)
• native heap analysis (using heaptrack)
• cpu flamegraphs (better clarity from full stack traces when necessary)

While we tend to use debug builds out of practice the documented approach for investigating native memory leaks recommends using a debug build. https://github.com/nodejs/node/blob/main/doc/contributing/investigating-native-memory-leaks.md#enabling-debug-symbols-to-get-more-information

i.e. does this pass the bar of being reasonably useful to a non-trivial amount of the ecosystem

Here is an issue that was posted in the actions/setup-node repo requesting this feature actually!
actions/setup-node#891

In Nov that team mentioned investigating its feasibility so I built it out as we need this as a tool in our toolbox.

is doing it here going to be significantly easier than current approaches (e.g. if the current approach is just "build it yourself", then the answer to that part is probably yes).
@nodejs/addon-api

Yes most definitely! I had already built this for use by us privately (ChainSafe/node_debug is a pipeline for building debug releases that we can download into our runners and ChainSafe/setup-node is a fork that pulls debug builds from our releases) as a team but the amount of effort involved to make it happen was a heavy enough lift that my team suggested we propose moving the work upstream. We figured other members of the community would be able to lean on our efforts as it was kind of a pain to build.

Please let me know if there is any more info we can provide and I will be happy to do the legwork. Our goal is to help contribute to the community at large even if that means attempting this and it getting denied. Although that would make me sad I am a team player and understand that there are many forces at play with large ecosystems like Node.js.

We look forward to your consideration and thank you in advance for giving this a fair shake. It means a lot to us as a team.

@matthewkeil

cc @philknows

[matthewkeil]

@rvagg Thank you for the consideration and your reviews. Ill assume this did not meet the criteria for merge by the build team. We will continue to use our fork.

I look forward to working with you on the next one though (whatever that may be). The open sourcerer's guild is a small group lol and I'm sure we will bump into each other again.

[rvagg]

eh, sorry, I didn't actually close this! I came back here to merge this and the dist-indexer PRs and noticed they're both closed. I guess because you were merging into my rvagg/local_build branch but for some reason GitHub didn't retarget those to master? Which is odd, because that was a straightforward merge.

@matthewkeil feel free to reopen this and the other PR and I'll get them merged. I think this is a reasonable recipe to add. Sorry for the confusion!

[matthewkeil]

eh, sorry, I didn't actually close this! I came back here to merge this and the dist-indexer PRs and noticed they're both closed. I guess because you were merging into my `rvagg/local_build` branch but for some reason GitHub didn't retarget those to `master`? Which is odd, because that was a straightforward merge.

@matthewkeil feel free to reopen this and the other PR and I'll get them merged. I think this is a reasonable recipe to add. Sorry for the confusion!

AWESOME!!!! Thank you very much @rvagg! I deleted the forks after I saw the PR closed but will put them both back up today and reference the old ones for historical tracking. Thanks again!

[matthewkeil]

@rvagg put up PR #112