Skip to content

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn 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

@hugovkhugovk 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

cc @illia-v

Copy link
Contributor

@illia-villia-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!

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

@sobolevnsobolevnOct 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

@hugovkhugovk left a comment

Choose a reason for hiding this comment

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

Thanks!

@hugovkhugovk merged commit def7ea5 into python:mainOct 10, 2023
@sobolevn
Copy link
MemberAuthor

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
MemberAuthor

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

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