Skip to content

gh-107652: Fix CIFuzz build - #110576

Merged
hugovk merged 3 commits into
python:mainfrom
sobolevn:fix-ci-fuzz-build
Oct 10, 2023
Merged

gh-107652: Fix CIFuzz build#110576
hugovk merged 3 commits into
python:mainfrom
sobolevn:fix-ci-fuzz-build

Conversation

@sobolevn

@sobolevnsobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member

When CI job was skipped the job was failing: https://github.com/python/cpython/actions/runs/6459321247/job/17536559744?pr=110573

Error: The template is not valid. .github/workflows/build.yml (Line: 620, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.

This happened because run_cifuzz was not set. Now we use the same logic as for other tools by setting it to false

Refs #107653
Refs #107652

@hugovkhugovk left a comment

Copy link
Copy Markdown
Member

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?

@hugovk

Copy link
Copy Markdown
Member

cc @illia-v

@illia-villia-v left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread.github/workflows/build.yml Outdated
# 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

Copy link
Copy Markdown
Member

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 with main

  • 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?

Suggested change
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

@sobolevnsobolevnOct 9, 2023

Copy link
Copy Markdown
MemberAuthor

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

@hugovkhugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hugovk
hugovk merged commit def7ea5 into python:mainOct 10, 2023
@sobolevn

sobolevn commented Oct 10, 2023

Copy link
Copy Markdown
MemberAuthor

Btw, I use https://explainshell.com/explain?cmd=grep+-qE all the time, can recommend for bash reviews :)

Thanks, everyone!

@sobolevn

Copy link
Copy Markdown
MemberAuthor

I can verify that my C changes now trigger CIFuzz: #110573

@hugovk

Copy link
Copy Markdown
Member

#109854 was created before merging CIFuzz, it changed .pre-commit-config.yaml and Tools/patchcheck/patchcheck.py.

I updated it from main, and it did the CIFuzz checks, somewhat surprisingly.

Do you know why?

@hugovkhugovk mentioned this pull request Oct 10, 2023
@hugovkhugovk changed the title Fix CIFuzz buildgh-107652: Fix CIFuzz buildOct 10, 2023
@illia-v

Copy link
Copy Markdown
Contributor

@hugovk I guess it happened because the merge commit contained changes to the relevant files

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@sobolevn@hugovk@illia-v