-
-
Notifications
You must be signed in to change notification settings - Fork 320
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: Run clang-format faster using a different action that runs in parallel #3284
Conversation
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.
DoozyX/clang-format-lint-action was my choice for another repo (just through Docker, not action, for some reason). The speed improvement is awesome.
I'm just unclear here, how this behaves in case of error. Maybe do a test commit with bad formatting.
I had one that I triggered with extra white space and the logs clearly showed me the number of processes files and fixed files. I'll try to find them in my fork, but since it's been months it'll need a new run to see the results. Edit: see here: https://github.com/echoix/grass/actions/runs/6662606466/job/18816176322 It's not the same branch, but the original testing was done there (other changes were explored in this draft to help profile). |
I reread your comment, and I think that maybe you're asking that you want the action to fail completely when they are files that were reformatted. I can add that, of course, I see that I was missing it. But another possibility would be to commit it, but as we saw, without a PAT actions won't rerun. Other possibility would be to add revision comments if there isn't a lot of changes to make (that could be accepted and committed from the UI). I've never done it yet, but it could be possible if that's the easiest way of using it. If you're talking about the case where the code is so bad that even clang-format itself fails and can't format? In that case, if the action fails, well it fails, and we won't have a little extra help with the changed files ready to simply paste back and commit. Honestly, if the code was so bad it shouldn't be merged as is, and we hope other checks will fail too ;) |
Yes, that's what I expect and meant to say. If formatting is wrong, fail so that it is clear the PR should not be merged. Perhaps we can do smarter things, like a commit or preferably a comment, but it seems to me that these always need to be in addition to a failure which tell you clear "no, don't merge this". Clang check is set to Required to ensure that things are not merged with bad formatting. The original action does fail with bad formatting (at least it did in the past). I of course don't particularly care which workflow will ensure that as long as some workflow will. |
Ok, let me get back to it, but probably not tonight :) |
We may also remove this workflow, replace it with the use of |
Is that PR/configuration almost ready? If it is run on another service, who "administers" it, ie, can anyone in the repo help out to improve/fix config when there's problems or the person isn't there anymore? |
I didn't get to work on it yet for round 2, thus the draft status. Christmas parties are started ;) |
The GRASS repo is ready now, it only remains to activate it. The advantage is that all pre-commit hooks defined in https://github.com/OSGeo/grass/blob/main/.pre-commit-config.yaml is run on a (one) runner image just as it would locally. In my quick test fork (linked in #3255) all was finished below 2 min (often closer to 1 min). Current tests: There is loads of CI time to save and – even more importantly – avoids discrepancies. With pre-commit.ci we may remove (or initially just disable) the 'Python Code Quality' (~9 min), 'ClangFormat check' (now ~20 min, with this PR ~1 min), 'General linting' (~8 min), with the added bonus of auto fixes.
This workflow is activated (once) on repository admin level, but is hence directed by .pre-commit-config.yaml. The infrastructure is located at https://github.com/pre-commit-ci. Footnotes |
And would the checks be executed on forks, ie, would be able to run on a PR inside my own fork, to make sure everything is ready before submitting it upstream? (It's harder to investigate and prepare changes when "production" is different from "staging", where staging would be our own repos) |
If you activate pre-commit.ci on your fork, probably could. Anyways the checks are not enabled on forks as it now stands anyway. That's why you should use pre-commit locally, not drain unnecessary CI resources. |
There's been some progress on the original action: jidicula/clang-format-action#147 (comment) and jidicula/clang-format-action#174 |
Still a draft. The permission issue was probably the fact that the workflow isn't on main yet. I committed a copy on my main branch, and ran a PR with an error, and the comment appeared correctly with the suggestion. However, I ended up with a limitation of GitHub, where the way the suggestion is made, it can't be applied on deleted lines. I still have another trick up my sleeve. |
… use the artifact to replace with fixed files
Ready for a review. I might need to rebase to clean up the intermediate commits needed. When there are some changes, covered by code suggestions: When there are multiple files that need to be formatted, but none of them end up in the diff_context (changed lines, plus or minus 3 lines around) that allows to add a suggestion, I add the message that says to take the artifact archive and copy-paste into the repo to replace with the new files. If any of you knows how to have an exit code as a bash variable/env-var, without actually having an exit code that would fail the step, I could remove completely one action. For now, I keep tj-actions/verify-changed-files only for the boolean output since I wasn't able to use the exit code from |
The exit code of the previous command is stored in
|
In this case, yes, there is no other way. Similar to what the bots are doing, although here it is for an experiment and not for an automated PR. |
I remember seeing $? somewhere. I just have to make sure that having that fist failing command doesn't stop the execution of the workflow, since a command "failed". |
I have another idea. Can we work and agree to end up approving and merging the PR, but without the part of posting comments really enabled, and then enable it in another PR, but with the workflow already in main with the correct permissions? Or we might want to go with a pull_request_target event, and pulling the code from the exterior fork, and try to run the formatting. I just have to verify how to do it safely, as that's the point of pull_request_target. |
Sure, go ahead, I don't recall this being a requirement. I though the original motivation was speed and that's also specified in the PR title. |
So as it is right now:
The only uncertainty left now, is that is the failure to post code review comments (95% sure due to permissions), is due to the fact that the action in the main branch didn't use the token with these permissions, or its because it is a PR from a fork? I also added a workflow_dispatch trigger as an escape for us: since when we (with access to the repo) run it manually, the actor is us and should work for a PR (if we are able to select the correct target), just a bit more work. So I suggest that we merge it as is for now, in the seconds is way better than 20 mins. A little cleanup of the squash message should be done for this. |
I cleaned up the squash message in the auto merge |
I'm eager to see if the action suggester works correctly once merged. If so, there's a lot of opportunities to use them in our CI, black for example, and even shellcheck+shfmt |
I'll go and implement it for the grass-addons two, and merge it myself there, so it'll be already tested out in a more "staging"-like repo, since we seem a bit more precautious here. |
The maximum token permissions for a PR coming from a fork is listed in the last column of the table here: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token I'm working since this morning on a solution, after reading a lot. Even if technically, using pull_request_target and pulling the PR's branch could be argued safe here since we are only linting, it's not that clear that it will always be safe. I'm working on a more complete solution. |
Yesterday night I managed to get something working as I'd like. I'll be cleaning this soon, and I made myself an organization to have an external fork to create a pull request against when testing, and this time it works. |
In the same goal of speeding up our CI, I'm suggesting here to use a different action to run clang-format, as there was no response (yet) from the currently used jidikula/clang-format-action jidicula/clang-format-action#147 (comment).
In summary, the current action calls to start a new Docker container for each file. It takes about 22 minutes to process our code base. By running by calling the binary with a list of files, and splitting the file list to run enough instances, https://github.com/DoozyX/clang-format-lint-action can process our code base in about 30 seconds.
The goal of having faster workflows, aven if they aren't our longest, is that since we have 20 runners available, shared across all OSGeo projects, when there is a lot of activity, we can start another job sooner.
I include here the analysis I wrote, after checking that out at the end of october.