ci: update debug node action and test workflow to use on several steps #6291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matthewkeil
force-pushed
the
mkeil/update-debug-node-workflow
branch
from
56447e1
to
0e58cea
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
jeluard
reviewed
Jan 13, 2024
| if: steps.cache-build-restore.outputs.cache-hit == 'true' | ||
| run: yarn build | ||
| run: | | ||
| if [ "${{ env.NODE_DEBUG }}" == "true" ]; then |
There was a problem hiding this comment.
Is this required for the build step too?
There was a problem hiding this comment.
yes. there have been a number of segfaults during build
There was a problem hiding this comment.
I guess I was more curious about the install step actually. Probably it makes sense to be consistent and do it everywhere.
| yarn test:unit | ||
|
|
||
| - uses: './.github/actions/core-dump' | ||
| if: ${{ always() && env.NODE_DEBUG == 'true' && steps.unit_tests.conclusion == 'failure' }} |
There was a problem hiding this comment.
I'm wondering if there could be a way to integrate the core-dump as part of the previous step script. It would help with readability.
There was a problem hiding this comment.
Not a bad idea but it needs to run as the last step of a job so that it can catch failures from any preceding step. Ideally this action should be added at the end of any job, as the last step, that may potentially throw a native fault
nflaig
changed the title
I moved this to draft because I am working on a few things to make it better. My big picture goal is to modify the setup node action to check for a debug version of the binary and us that instead of the base node. When I was debugging the download of node was brittle and its built manually by another workflow in this repo. I am building a Chainsafe/node_debug fork that tracks the main repo and builds versions of node as they are released. The repo will serve to host the node binary for download by workflows in a predictable location that has no bandwidth limits. Then the updated setup-node action will allow passing a debug_version flag that will select the correct binary. I dont want to have to build and host all platforms, only linux, which give me pause to PR the action to the main repo though. This will get rid of the manual overwrite of the node binary and will also make sure we will be using the same node versions as the setup-node action. As for the ulimit command I am not against putting that behind a custom action but that feels like a lot of work to get away from a single bash conditional. I am open to it but think its kinda overkill considering the workflows do not get updated too often. Mostly its the versions of node that change regularly, which is why I am putting a bit of effort into building the releases in the node_debug fork so we are testing across several versions. |
|
Thanks for the detailed answer! Makes sense!
Totally. We can always revisit later if need be. |
|
Got the debug recipe PR finished and waiting for final approval Need to rewrite the setup-node work I built to use Chainsafe/node_debug fork as it will end up getting deprecated now that the official one will be up soon |
|
The unofficial-build PR was just given the go ahead... Fake out!!! Got closed accidentally by CI. Its been reopened and is awaiting approval and merge. Will ask what the process for building retroactive version is |
|
The first unofficial-build PR was merged!!! 🎉 Sadly though there is a bug/issue with the system that prevents historical builds from being queued. I have started a PR here: nodejs/unofficial-builds#116 to fix that and got initial feedback but it needs a bit more work to get over the hump. Will try and finish it this week and get some debug versions published. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
We are getting a lot of seg faults in various steps.
.github/actions/setup-debug-nodeto overwrite node with a debug version of node.NODE_DEBUGflag that controls usage of regular or debug version throughout workflow.Testing
This workflow needs to be tested to verify that the added scripts and functionality works as expected. Will go through a round of reviews and then may need to update if there are bugs.