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

build: add asan check in Github action #31902

Closed
wants to merge 1 commit into from
Closed

Conversation

@gengjiawen
Copy link
Member

gengjiawen commented Feb 21, 2020

This related to #30257

This currently only runs on linux only. Acutally windows has made some progress on
this too, see https://devblogs.microsoft.com/cppblog/addresssanitizer-asan-for-windows-with-msvc/, but it require cmake as build toolchain IIUC. Also current code asan checks broken on macOS, so I didn't add them.

currrent Docker Image source: https://github.com/gengjiawen/node-build/blob/master/Dockerfile.

todo

Hopefully we can remove continue-on-error in the future.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 21, 2020

Seems like a good idea but how long does make test take for an asan+debug build?

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Feb 21, 2020

Seems like a good idea but how long does make test take for an asan+debug build?

https://github.com/nodejs/node/pull/31902/checks?check_run_id=460428978 took 1 hr 40mins.

@gengjiawen gengjiawen force-pushed the gengjiawen:ci/asan branch to 29ae5aa Feb 22, 2020
@gengjiawen

This comment has been minimized.

Copy link
Member Author

gengjiawen commented Feb 22, 2020

ASAN checks failed on first error (https://github.com/nodejs/node/runs/460428978?check_suite_focus=true), so I add ASAN_OPTIONS: halt_on_error=0 to run all the tests.

@gengjiawen

This comment has been minimized.

Copy link
Member Author

gengjiawen commented Feb 25, 2020

@bnoordhuis @addaleax @richardlau Can you review this ? thanks.

Copy link
Member

bnoordhuis left a comment

LGTM and thanks, this is useful!

To save everyone some clicking: it takes 205 minutes or about 3.5 hours for a full run. 1.5h is just the build itself though so maybe that can be sped up somehow.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

gengjiawen added a commit that referenced this pull request Mar 3, 2020
PR-URL: #31902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gengjiawen

This comment has been minimized.

Copy link
Member Author

gengjiawen commented Mar 3, 2020

Landed in 3ec4b21

@gengjiawen gengjiawen closed this Mar 3, 2020
@gengjiawen gengjiawen deleted the gengjiawen:ci/asan branch Mar 3, 2020
@richardlau richardlau mentioned this pull request Mar 4, 2020
2 of 2 tasks complete
MylesBorins added a commit that referenced this pull request Mar 4, 2020
PR-URL: #31902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
codebytere added a commit that referenced this pull request Mar 16, 2020
PR-URL: #31902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
codebytere added a commit that referenced this pull request Mar 17, 2020
PR-URL: #31902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gengjiawen gengjiawen mentioned this pull request Mar 20, 2020
0 of 13 tasks complete
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request Mar 26, 2020
PR-URL: nodejs#31902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.