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

[CI] Improve fetching of changed files #89980

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 28, 2024

PRs always use a merge into the repo on checkout, so checking for HEAD^1 will show all the changes regardless of the number of commits in a PR, as it represents the target commit of the merge

You can see the improved results by comparing these two builds:

It appears the CI will still catch file formatting without the files (I think it runs on all files if none are specified) but this ensures the files are properly listed, and it seems to fail on some files in the existing version when it fails to fetch files (my bad):

  • It skips codespell without this fix, and possibly also some other checks
  • It runs some checks on all files (like python, js, checks)

The reason it partially worked (which I failed to notice because some CI checks run regardless and I didn't check other cases like header guards) was that for single commit PRs it did the correct thing, checking the merge commit against its first parent, which was correct, but with multiple commit it instead broke, unsure exactly what broke with it though, but this new change ensures it always picks the first parent of the merge and that will include all the changes and have them properly aligned with the merged state

Follow up to:

PRs always use a merge into the repo on checkout, so checking for
`HEAD^1` will show all the changes regardless of the number of commits
in a PR
@AThousandShips AThousandShips added enhancement topic:buildsystem cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 28, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 28, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner March 28, 2024 12:55
@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 28, 2024

Will add some broken files to demonstrate that it catches these cases:

  • Codespell
  • Header guards

Didn't know quite how to fix a file check one as it doesn't catch trailing newlines for some reason, and trailing whitespace is already caught by clang, so leaving with those two checks being confirmed

@AThousandShips AThousandShips requested a review from a team as a code owner March 28, 2024 13:19
@AThousandShips AThousandShips force-pushed the ci_better_fix branch 3 times, most recently from 53ad933 to 950743c Compare March 28, 2024 13:35
@akien-mga akien-mga merged commit 29b3d9e into godotengine:master Mar 28, 2024
31 of 32 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the ci_better_fix branch March 28, 2024 18:07
@AThousandShips
Copy link
Member Author

Thank you!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 8, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Apr 8, 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