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

feat: add x64-debug build recipe #107

Closed
wants to merge 9 commits into from
Closed

feat: add x64-debug build recipe #107

wants to merge 9 commits into from

Conversation

matthewkeil
Copy link
Contributor

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
Copy link
Member

rvagg commented Jan 18, 2024

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 matthewkeil changed the base branch from main to rvagg/local_build January 18, 2024 01:47
@matthewkeil
Copy link
Contributor Author

matthewkeil commented Jan 18, 2024

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 matthewkeil marked this pull request as draft January 18, 2024 04:17
@matthewkeil matthewkeil marked this pull request as ready for review January 18, 2024 05:59
@matthewkeil
Copy link
Contributor Author

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

@matthewkeil
Copy link
Contributor Author

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

@rvagg
Copy link
Member

rvagg commented Jan 18, 2024

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

rvagg commented Jan 18, 2024

(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
Copy link
Contributor Author

matthewkeil commented Jan 19, 2024

@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

@rvagg rvagg deleted the branch nodejs:rvagg/local_build January 20, 2024 10:57
@rvagg rvagg closed this Jan 20, 2024
@matthewkeil
Copy link
Contributor Author

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

rvagg commented Jan 29, 2024

Screenshot 2024-01-29 at 13 33 12

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
Copy link
Contributor Author

Screenshot 2024-01-29 at 13 33 12 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 matthewkeil mentioned this pull request Jan 30, 2024
@matthewkeil
Copy link
Contributor Author

@rvagg put up PR #112

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

Successfully merging this pull request may close these issues.

2 participants