Skip to content

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

Merged
merged 3 commits into from
Oct 10, 2023
Merged

gh-107652: Fix CIFuzz build #110576

merged 3 commits into from
Oct 10, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 9, 2023

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

Copy link
Member

@hugovk hugovk left a 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?

@hugovk
Copy link
Member

hugovk commented Oct 9, 2023

cc @illia-v

Copy link
Contributor

@illia-v illia-v left a 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!

@@ -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
Copy link
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

Copy link
Member Author

@sobolevn sobolevn Oct 9, 2023

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

Copy link
Member

@hugovk hugovk left a comment

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:main Oct 10, 2023
@sobolevn
Copy link
Member Author

sobolevn commented Oct 10, 2023

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

Thanks, everyone!

@sobolevn
Copy link
Member Author

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

@hugovk
Copy link
Member

hugovk commented Oct 10, 2023

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

@hugovk hugovk changed the title Fix CIFuzz build gh-107652: Fix CIFuzz build Oct 10, 2023
@illia-v
Copy link
Contributor

illia-v commented Oct 10, 2023

@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 free to 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