Skip to content
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

bpo-40548: Fix "Check for source changes (pull_request)" GH Action job #21806

Merged
merged 2 commits into from Aug 10, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 10, 2020

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.

https://bugs.python.org/issue40548

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

The macOS failure is unrelated and can be ignored: https://bugs.python.org/issue40275#msg375122

@vstinner vstinner changed the title Fix "Check for source changes (pull_request)" GH Action job bpo-40548: Fix "Check for source changes (pull_request)" GH Action job Aug 10, 2020
@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

This change can be associated to https://bugs.python.org/issue40548 which introduced the "Check for source changes" job.

@shihai1991
Copy link
Member

shihai1991 commented Aug 10, 2020

Oh, you are so quick. I try to fix it a minute ago: shihai1991#30 it) Lol~

# into the PR branch anyway.
#
# https://github.com/python/core-workflow/issues/373
git diff --name-only origin/$GITHUB_BASE_REF.. | grep -qvE '(\.rst$|^Doc|^Misc)' && echo '::set-output name=run_tests::true' || true
Copy link
Member

@shihai1991 shihai1991 Aug 10, 2020

Choose a reason for hiding this comment

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

I test it in last weekend, it can work. and removing the git fetch origin $GITHUB_BASE_REF --depth=1 can fix it too.
I guess depth=1 is not enough deep to find the merge base.

@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

I guess depth=1 is not enough deep to find the merge base.

Correct. But depth=1 is part of https://bugs.python.org/issue40548 design to make the "Check for source changes" job fast. For example, on my latest change, it only took 18 seconds overall!

I don't think that "..." (3 dots) to find the last common commit is needed for this job. ".." (2 dots) should be enough.

Copy link
Member

@shihai1991 shihai1991 left a comment

Thanks, It's can work for me. so LGTM.

@vstinner vstinner merged commit eaa5517 into python:master Aug 10, 2020
8 of 9 checks passed
@vstinner vstinner deleted the fix_workflow_diff branch Aug 10, 2020
vstinner added a commit to vstinner/cpython that referenced this issue Aug 10, 2020
@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

I tested manully: with this change, doc-only PR still skip build jobs.

I created #21812 to test this PR. It's a "documentation-only" PR. Build GitHub Action jobs have been skipped as expected, and "Docs / Docs" is running as expected.

@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

@shihai1991 @brettcannon: Thanks for the review! I merged my PR. So far, it works as expected. If something goes wrong, we can change the test again.

Python 3.9 is also affected, example: #21809 Check for source changes: "fatal: ... no merge base". I backport the change to 3.8 and 3.9.

@miss-islington
Copy link
Contributor

miss-islington commented Aug 10, 2020

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington
Copy link
Contributor

miss-islington commented Aug 10, 2020

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Aug 10, 2020

GH-21813 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 10, 2020
pythonGH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

bedevere-bot commented Aug 10, 2020

GH-21814 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 10, 2020
pythonGH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington added a commit that referenced this issue Aug 10, 2020
GH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington added a commit that referenced this issue Aug 10, 2020
GH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
@FFY00
Copy link
Member

FFY00 commented Aug 10, 2020

@vstinner sorry for the delay, I did not have access to my computer over the weekend.

I implemented this in another project and was running into an issue, this was my solution:

https://github.com/FFY00/arch-python-repo/blob/9cf94335c441638a5a7fbed5b58e4852fbdd5792/.github/workflows/build.yml#L39

      - name: Check for source changes
        id: check
        env:
          PREVIOUS: ${{ github.event.before }}
          CURRENT: ${{ github.sha }}
        run: |
          git fetch origin $PREVIOUS
          git diff --name-only $PREVIOUS $CURRENT | grep -q ... || true

Turns out github actions makes the previous object SHA available for us to use. We can use this to explicitly fetch it and to run the diff. It should be more reliable than the current approach. What do you think?

@vstinner
Copy link
Member Author

vstinner commented Aug 10, 2020

Turns out github actions makes the previous object SHA available for us to use. We can use this to explicitly fetch it and to run the diff. It should be more reliable than the current approach. What do you think?

I tried git diff origin/$GITHUB_BASE_REF..$GITHUB_SHA: it avoids the "git checkout", but the checkout is part of actions/checkout@v2, we cannot avoid it. With the checkout, origin/$GITHUB_BASE_REF..$GITHUB_SHA is the same than origin/$GITHUB_BASE_REF..HEAD which is the same than origin/$GITHUB_BASE_REF...

From what I understood, in practice, git diff $PREVIOUS $CURRENT is the same than git diff origin/$GITHUB_BASE_REF...

@FFY00
Copy link
Member

FFY00 commented Aug 10, 2020

No, GITHUB_BASE_REF (also github.base_ref) is different from github.event.before.

github.base_ref:

The base_ref or target branch of the pull request in a workflow run. This property is only available when the event that triggers a workflow run is a pull_request.

github.event.before is the SHA of the top commit before the event.

It is documented in the webhooks documentation, see https://docs.github.com/en/actions/reference/events-that-trigger-workflows for context.

github.event.before:

The SHA of the most recent commit on ref before the push.

Github is doing a bad job with the documentation here, it took a lot of digging to uncover this. But it does work and is documented.

shihai1991 pushed a commit to shihai1991/cpython that referenced this issue Aug 20, 2020
pythonGH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
xzy3 pushed a commit to xzy3/cpython that referenced this issue Oct 18, 2020
pythonGH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
@miss-islington
Copy link
Contributor

miss-islington commented May 5, 2022

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 5, 2022
pythonGH-21806)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

bedevere-bot commented May 5, 2022

GH-92342 is a backport of this pull request to the 3.7 branch.

ned-deily pushed a commit that referenced this issue Sep 2, 2022
GH-21806) (GH-92342)

On Git 2.28, "git diff master..." (3 dots) no longer works when
"fetch --depth=1" is used, whereas it works on Git 2.26.

Replace "..." (3 dots) with ".." (2 dots) in the "git diff" command
computing the list of modified files between the base branch and the
PR branch.
(cherry picked from commit eaa5517)

Co-authored-by: Victor Stinner <vstinner@python.org>
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.

None yet

8 participants