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

[wip] Add a workflow to run clang-format and push back the changes #2

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

Conversation

tmadlener
Copy link

@tmadlener tmadlener commented Jul 14, 2023

This is a working draft for a workflow that runs clang-format -i and then pushes back the fixes on a PR when prompted via /fix-style in a comment of the PR.

This is all "handrolled", there are actions that could be used to achieve the same

Depending on how much control we want to have over the environment in which we run this we could probably replace quite a bit of this with those. In the end we mainly need to be consistent with what we run in the pre-commit workflow in the repository.

  • Graceful exit in case there are no changes
  • Confirm that this also works on PRs that were not opened by me

@m-fila
Copy link
Contributor

m-fila commented Nov 14, 2023

Is there a reason to have a separate python script that delegates the files to clang-format-12?
Would it be possible to have a CI job invoke the pre-commit then commit and push its changes? That way you don't have to worry about the formatting script and pre-commit going out of sync, other formatters beside clang-formatare handled too.

@tmadlener
Copy link
Author

IIRC mainly to be able to explicitly pick a clang-format version as different versions have slight differences in formatting (even with the same .clang-format config). Additionally, this is probably modeled after the clang-format-hook script that we usually use. That has been necessary in the past to be able to run pre-commit in the CI environments that we usually have.

Having said that, if there is a way to remove all or most of that, I am all for it.

@m-fila
Copy link
Contributor

m-fila commented Nov 15, 2023

I didn't notice that the repos use a custom local hook for clang, which is only proposing formatting changes and not fixing them in-place. I had probably a bit more common workflow in mind, where the hook is actually doing in-place fixing and then a CI 'format' action is doing pre-commit run --display-on-failure to display changes on CI log in case someone smuggled some bad formatting by commiting without having pre-commit installed.
In that scenario it would be easy to have this /fix-style action just run the pre-commit that would do an in-place fixing (enforcing the same rules as on developer's system) and then push changes.

@tmadlener
Copy link
Author

I did some digging to see why we had this special hook, and I think there are two reasons. Easily possible that either of them is mainly historical at this point.

  • Back when I first introduced pre-commit to podio we were not yet having LLVM in the Key4hep stack, so for people to run it locally they would have to setup a dedicated environment in which both clang-format and clang-tidy are present. This also meant that we couldn't run the workflow in a Key4hep environment, but used an LCG stack instead. As far as I remember (and this maybe also has changed): The pre-commit action wasn't able to deal with our CI setup, where we run everything inside a container that is effectively empty and then pull in all dependencies via CVMFS. I also found quite a few clang-format and clang-tidy hooks for pre-commit that didn't fully do what we wanted, and at that point it was quite simply to just quickly roll our own.
  • At the time the outputs of the hooks that did change code were a bit hard to decipher from the github actions log. Hence, I tried to make a slightly more instructive output. Again, this was mainly related to the fact that setting up an environment where pre-commit "simply worked" wasn't that easy.

Anyhow, long story short: Everything that simplifies the setup is good and I am more than happy to go in that direction.

@m-fila
Copy link
Contributor

m-fila commented Nov 28, 2023

I created a very small example repository to demonstrate how this could work with just pre-commit. The actions were based on this PR and other key4hep repos and slightly modified to handle everything through pre-commit only.

Example PR that demonstrates autofixing. The clang-format is run with 'pre-commit/mirrors-clang-format' hook

  • The execution time of the hooks seems to be a few minutes on a free runner, mostly spent on spinning up the environment. The environments are provided with 'cvmfs' action and either 'LCG view' or sourcing 'key4hep/setup.sh'.
  • The hooks directory installed by pre-commit could be probably cached between the CI jobs.
  • The hooks are run on all the files in a repo, which probably should be limited to files changed in a current PR.
  • For every comment in PR a new job is started and eventually skipped if criteria are not met (comment starting with specific string). This can very easily pollute the Actions tab with skipped jobs.

According to 'pre-commit' devs the recommended way to run 'pre-commit' hooks in CI jobs is their 3rd party service 'pre-commit.ci' or their lite action that require installing a github application on a repo. I'm not sure if it's really worth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants