-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Add paths filter to github runner builds, to skip CI when appropriate. #100212
base: master
Are you sure you want to change the base?
Conversation
While this is intriguing I feel this is pretty fragile and potentially error prone, especially with updating paths creating potential missed areas, and for what I feel is pretty limited gains Also what happens on merge? We should always build all builds on merge or we'll mess up the caches etc. |
The filters can be configured like a blacklist (as I have here), I think this is unlikely to cause problems. But yeah, of course, it's not a guarantee, since i don't have any experience with the action.
Good question! I don't know if the action has this 'feature' builtin, but worst case we can work around it since the |
At minimum it has to compile properly on merge commits, so that would need to be confirmed But it would be useful to get an idea of the portion of PRs that actually only are like this, platform specific changes (even with more specific filters like selecting specific drivers etc.) to see if this would be reasonable compared with the risks (including the additional CI step which isn't free in itself) If only 1/100 PRs are platform specific properly then the cost to maintain this (especially to keep the filter up-to-date) would be disproportionate, for example |
Good point. I ran a script to try to get a rough idea of this. count-changes.shfolder_path="platform"
max_commits=1000 # Maximum number of commits to search
count=0
#commits=$(git log --format="%H" -n "$max_commits" -- "$folder_path")
commits=$(git log --format="%H" -n "$max_commits")
# Loop through each commit hash
for commit_hash in $commits; do
# Get list of changed files for the commit
files=$(git diff-tree -m --no-commit-id --name-only -r "$commit_hash")
# Check if all changed files are within the specified folder
inside_folder=true
for file in $files; do
if [[ $file != $folder_path/* ]]; then
inside_folder=false
break
fi
done
# If all changes are inside the folder, increment the count
if [ "$inside_folder" = true ]; then
count=$((count + 1))
fi
done
echo "Total commits that only changed files in '$folder_path': $count" platform/ . (it does not test whether any of those modify multiple subfolders in platform/ )
Running this gave me the number Since some changes may only pertain to Edit: the number is |
Now the most impactful would be ones not affecting Windows or Linux/X11, but I suspect that's smaller in proportion Granted avoiding iOS or macOS builds (because of the cap on runners for those) would be useful, but the cost of running Android is trivial, as is Web |
1585302
to
2927f7f
Compare
I have reformatted the PR to use a I have also added a skip for the With 2 file changes, the added Edit: Also, here's a concept how the |
2927f7f
to
483ee71
Compare
483ee71
to
28d1368
Compare
@@ -0,0 +1,21 @@ | |||
shared: &shared | |||
- '**' | |||
- '!platform/*/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export and API files should not be excluded either I think, as they are built on individual other platforms AFAIK, so changes there should trigger a build to ensure any potential errors are detected, unsure which targets should build each though, this is clearly very tangled and hard to determine safely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I just checked my compile_commands.json
file; there are a bunch of files in those that are included cross-platform. I won't resolve this for now; I think it's solvable, but I confess it wouldn't be as clean as I'd hoped it might be.
This PR lets the CI / GitHub Runner skip some of the jobs when running them would be unnecessary, because just from looking at the files it can be understood that no rebuild is needed. The CI will still pass even if some jobs are skipped.
Example: https://github.com/Ivorforce/godot/actions/runs/12241380885
(only Linux build is run for a change to the linux platform)
((to make the example work, i had to modify the base ref for the branch only, but it should work out of the box for PRs))
Discussion
Theoretically, GitHub has support for path filters builtin through the paths parameter.
However, it can only be used to skip the CI as a whole, not parts of it, making it unsuitable for our purposes. (And besides, if the CI is skipped, it is well-known that the PR is not mergable - it is only mergable on an OK result).
A solution is provided by https://github.com/dorny/paths-filter. The action uses
git
to check against a git ref. By default, the ref is the latest shared commit of the base branch (master
).Unfortunately, path filters are not applied sequentially, but evaluated in a
some
orall
pattern. This prohibits.gitignore
-like filters like:Fortunately, this has been addressed in a fork. Therefore, I am using the fork. I have not extensively tested the stability of either the fork or the original repository. I cannot guarantee it won't introduce problems with CI, for example, by incorrectly skipping CI.
Addendum
For now, only
platforms
contributions are filtered, which can skip CI for contributions like #100205, which only affect one particular platform's python files. Follow-up PRs should expand the filters to better pinpoint what other parts can be skipped, for example, on changes to the readme file.Also, the filters can be defined in a dedicated file like
.github/filters.yaml
. This would probably be advisable to avoid cluttering the workflow itself.