-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[infra] Remove sancov and rename "profile" to "coverage". #1839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just one comment.
Also, could you please remove the corresponding code in corpus pruning and oss_fuzz_setup ?
export COVERAGE_FLAGS= | ||
fi | ||
|
||
export CFLAGS="$CFLAGS $SANITIZER_FLAGS $COVERAGE_FLAGS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is $COVERAGE_FLAGS no longer needed here? don't we need it for regular fuzzing builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually SANITIZER_FLAGS is no longer included after this deletion too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch re COVERAGE_FLAGS
! Those are still defined by base-builder/Dockerfile and might be overwritten by fuzzing engines compile_*
scripts.
SANITIZER_FLAGS
should still be there (weird GitHub UI moved them up to line 45).
Either way, I'm bringing this coverage flags override code back, as otherwise it looks confusing to have empty COVERAGE_FLAGS
for a coverage build :D
If this renames |
yes sure!
Thanks for the heads up! Looks like there are some other projects that also rely on |
|
@pdknsk can you please upload a PR which I will merge right before merging mine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* [infra] Remove sancov and rename "profile" to "coverage". * Bring coverage flags back. * Update projects files that rely on SANITIZER="profile".
…w-up google#1839)." (google#1848) This reverts commit f132eaf.
Ready for review, but please don't merge it until I land some changes on CF side.