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

Add paths filter to github runner builds, to skip CI when appropriate. #100212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Dec 9, 2024

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 or all pattern. This prohibits .gitignore-like filters like:

**
!platform/*/**
platform/android/**

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.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 9, 2024

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.

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Dec 9, 2024

especially with updating paths creating potential missed areas

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.

Also what happens on merge? We should always build all builds on merge or we'll mess up the caches etc.

Good question! I don't know if the action has this 'feature' builtin, but worst case we can work around it since the if is just an expression we have full control over.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 9, 2024

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

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Dec 9, 2024

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)

Good point. I ran a script to try to get a rough idea of this.

count-changes.sh
folder_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"
The script looks at the last 1000 commits (roughly 1 month of contributions 🎉), and counts those that only perform changes to platform/. (it does not test whether any of those modify multiple subfolders in platform/)

Running this gave me the number 38 (affecting 3.8% of commits). With the assumption that each of those commits touched exactly of half the platforms' files, needing a single commit each (which I think is conservative), it would have saved at least 114 full builds in the last month.

Since some changes may only pertain to md files, documentation, or others, the real number is likely to be larger.

Edit: the number is 45 for doc/, adding to 8.3%.

@AThousandShips
Copy link
Member

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

@Ivorforce Ivorforce force-pushed the workflow-call-paths branch 2 times, most recently from 1585302 to 2927f7f Compare December 9, 2024 23:20
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Dec 9, 2024

I have reformatted the PR to use a yaml file, and a shared section for convenience.

I have also added a skip for the dispatch steps on non-PR triggers, as requested by AThousandShips.
You can see that it checked the path filters as expected for the PR trigger: https://github.com/godotengine/godot/pull/100212/checks
But it skipped the check in my own repo for the push trigger, and just ran all of CI: https://github.com/Ivorforce/godot/actions/runs/12246113225/job/34161384346

With 2 file changes, the added Dispatch step takes 10 seconds to complete. (when skipped, it still takes 2 seconds).


Edit: Also, here's a concept how the path_filters.yaml file could look like after some discussion: https://github.com/Ivorforce/godot/blob/workflow-call-paths-concept/.github/path_filters.yaml
I'm opting not to put it into this PR yet in order to make it less controversial - some of those should be separately discussed, probably.

@Ivorforce Ivorforce force-pushed the workflow-call-paths branch from 2927f7f to 483ee71 Compare December 9, 2024 23:24
@@ -0,0 +1,21 @@
shared: &shared
- '**'
- '!platform/*/**'
Copy link
Member

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

Copy link
Contributor Author

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.

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.

2 participants