-
Notifications
You must be signed in to change notification settings - Fork 206
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 a clang-format format check in github pull request actions. #661
Conversation
Issues #653 Signed-off-by: Henner Zeller <h.zeller@acm.org>
@hzeller -- You might want to add a commit which deliberately break formatting to test it does the right thing? |
I did that in my local test repository: https://github.com/hzeller/verible/runs/1817888890 |
.github/workflows/code-style.yml
Outdated
VerifyFormatting: | ||
runs-on: ubuntu-20.04 | ||
|
||
steps: |
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.
Unless you want to have a separate status report and shield/badge for showing the status of this specific test/job independently from others, I would suggest not to create this new workflow. Instead, just copy this job (L7-L19) into the existing workflow.
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.
Done. I mostly started the new file while experimenting with the new flow, but merging it now makes it simpler.
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
sudo apt-get install clang-format |
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.
Unless this is used somewhere else, I would not recommend adding it as a separate file. Just put it in the workflow. In the current state, this script is platform specific, so not really very useful outside of that specific CI job. If/when it is modified for supporting installation on different environments, then it'll make sense to have it be a file.
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.
Sounds good. Done.
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.
LGTM in general
@@ -0,0 +1,31 @@ | |||
#!/bin/bash | |||
# Copyright 2020 The Verible Authors. |
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.
2021 😉
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.
Done.
FORMAT_OUT=${TMPDIR:-/tmp}/clang-format-diff.out | ||
|
||
# TODO(hzeller): only run on the files that are checked out | ||
clang-format -i --style=Google $(find . -name "*.h" -o -name "*.cc") |
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.
We could probably use something like:
$(git ls-tree HEAD -r --name-only | grep --color=never -e '\.cc$\|\.h$')
But assuming that this is run before building and before anything is generated I think find
should be okay too
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.
There's a recipe for modified files here (git_touched_files
):
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.
Both these assume that the files are local changed. Though I assume within the pull request environment it will already have applied the patch already as we have a clean head without diffs.
So maybe I need something like
git diff --name-only --diff-filter=AM HEAD~1
? Will have to try.
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.
So I did some experiments in a testing branch, trying to see how the git repository looks like when we're called in an action in a pull request
It looks weird: it looks like a git repository with a single branch (named like the branch from which is pushed) ,but no master branch. There is also only a single commit on it. So it looks like it clones everything, applies all the previous patches in the pull request, uses that as 'git init' baseline in a fresh repository with a single branch and then applies a single patch, which is the last push in that pull request.
Anyway, so this means, not even a git diff --name-only --diff-filter=AM -r origin/master
will work.
Apparently, there are github actions scripts on the marketplace that can find all changed files; will have to look into that.
So I suggest for now, we leave it with the find
all the files and apply clang-format to them. Which is fine, as we expect the code-base to be clean all the time so that we can afford running the format over all the files, even the ones not touched in the pull request.
Signed-off-by: Henner Zeller <h.zeller@acm.org>
Signed-off-by: Henner Zeller <h.zeller@acm.org>
You can change how the git repository is cloned. |
https://github.com/actions/checkout
|
Aha, bingo, this seems to be the ticket. Here the output of a run in my test repository: https://github.com/hzeller/verible/runs/1825048661 |
Signed-off-by: Henner Zeller <h.zeller@acm.org>
Not sure why it didn't run the actions now after the last commit, but it should work now :) |
FTR, the actions/checkout is both clever and annoying. Precisely, |
@hzeller, when merging both workflows, the indentation was not the same. At the same time, a |
Ah, interesting, thanks for hunting down the syntax issues. |
You go to the Actions tab above. You see, e.g.:
That's correct, because "verible-ci" is shown in the second line. When the workflow is defective, you see Unfortunately, all that is dynamically generated. Therefore, as soon as we fixed it, those failing items are not shown anymore. That's why I cannot point you to an specific example ATM. Anyway, the error just points to a line in the workflow. In this case, it was not intuitive to know that the problem was indentation. I knew it because I had hit that problem before. |
Issues #653
Signed-off-by: Henner Zeller h.zeller@acm.org