-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-107652: Fix CIFuzz build #110576
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
gh-107652: Fix CIFuzz build #110576
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.
There's an error with this, please could you check?
cc @illia-v |
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.
I'm sorry for the error. Thank you for the fix!
.github/workflows/build.yml
Outdated
@@ -82,11 +82,15 @@ jobs: | |||
# CPython, so CIFuzz should be run only for code that is likely to be | |||
# merged into the main branch; compatibility with older branches may | |||
# be broken. | |||
if [ "$GITHUB_BASE_REF" = "main" ]; then | |||
FUZZ_RELEVANT_FILES='(\.c$|\.h$|\.cpp$|^configure$|^\.github/workflows/build\.yml$|^Modules/_xxtestfuzz)' | |||
if [ "$GITHUB_BASE_REF" = "main" ] && [ "$(git diff --name-only origin/$GITHUB_BASE_REF.. | grep -qvE $FUZZ_RELEVANT_FILES; echo $?)" -eq 1 ]; then |
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.
Do I get this logic right?
First part:
[ "$GITHUB_BASE_REF" = "main" ]
- this is a PR
Second part:
-
git diff --name-only origin/$GITHUB_BASE_REF..
- lists files changed compared withmain
-
grep -qvE $FUZZ_RELEVANT_FILES
- checks the changed files are NOT of the relevant type -
-eq 1
- error code 1, so true only when the files ARE found
Instead of checking no match is false: (...; echo $?)" -eq 1
Can we check for a match, something along the lines of this?
if [ "$GITHUB_BASE_REF" = "main" ] && [ "$(git diff --name-only origin/$GITHUB_BASE_REF.. | grep -qvE $FUZZ_RELEVANT_FILES; echo $?)" -eq 1 ]; then | |
if [ "$GITHUB_BASE_REF" = "main" ] && [ "$(git diff --name-only origin/$GITHUB_BASE_REF.. | grep -qE $FUZZ_RELEVANT_FILES)" ]; then |
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.
When using grep -q
in $()
, we have to rely on exit code, because no output is produced.
So:
(.venv) ~/Desktop/cpython fix-ci-fuzz-build ✔
» echo 'abc' | grep -qE 'b'; echo $?
0
(.venv) ~/Desktop/cpython fix-ci-fuzz-build ✔
» echo 'abc' | grep -qE 'y'; echo $?
1
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.
Thanks!
Btw, I use https://explainshell.com/explain?cmd=grep+-qE all the time, can recommend for Thanks, everyone! |
I can verify that my C changes now trigger CIFuzz: #110573 |
#109854 was created before merging CIFuzz, it changed I updated it from Do you know why? |
@hugovk I guess it happened because the merge commit contained changes to the relevant files |
When CI job was skipped the job was failing: https://github.com/python/cpython/actions/runs/6459321247/job/17536559744?pr=110573
This happened because
run_cifuzz
was not set. Now we use the same logic as for other tools by setting it tofalse
Refs #107653
Refs #107652