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.

Sign up for GitHub

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

Jump to bottom

Add .clang-format file which defines coding style as GNU #2851

Closed
wants to merge 1 commit into from
Closed

Add .clang-format file which defines coding style as GNU #2851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Language: Cpp
BasedOnStyle: GNU
ColumnLimit: 99
22 changes: 21 additions & 1 deletion ci/codestyle.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,27 @@
#!/usr/bin/env bash
# Tests that validate structure of the source code;
# can be run without building it.
set -euo pipefail
set -eo pipefail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we keep this and instead...


if command -v clang-format >/dev/null && [ -n "$GITHUB_ACTOR" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...use [ -n "${GITHUB_ACTOR:-}" ]

git remote add github_actor https:///github.com/$GITHUB_ACTOR/ostree.git
git remote add https_upstream https://github.com/ostreedev/ostree.git
git fetch --recurse-submodules=no github_actor &
git fetch --recurse-submodules=no https_upstream main &
LOG_LINE=$(git show -s --format=%B | grep Merge)
wait
THIS_COMMIT=$(echo $LOG_LINE | awk '{print $2}')
THAT_COMMIT=$(echo $LOG_LINE | awk '{print $4}')
git reset --hard $THIS_COMMIT
git rebase $THAT_COMMIT

echo -n "Formatting with git clang-format... "
if ! git clang-format $THAT_COMMIT; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes this a git subcommand?

echo "git clang-format $THAT_COMMIT failed; run: please commit the changes"
exit 1
fi
echo "ok"
fi

# Don't hard require Rust
if command -v cargo >/dev/null; then
Expand Down
2 changes: 1 addition & 1 deletion ci/gh-build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ set -euo pipefail
set -x

# First, basic static analysis
./ci/codestyle.sh
./ci/codestyle.sh clang-format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this argument do?

Copy link
Collaborator Author

@ericcurtin ericcurtin Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove the "clang-format" arg if needs be. I added it because I felt the ci/codestyle.sh shouldn't do git operations like fetch, rebase, etc. and apply reformats by default. But it's the only way I could thing of, of getting git clang-format to just look at the changes in a PR. If we reformat the whole codebase though, you can simply call clang-format to check the whole codebase.

Another thing I've seen happen, just to be mindful of it if we start seeing it, is sometimes you have to do version checks eventually like only check if clang-format version is greater than 12. Because sometimes how it reformats between versions can change slightly, and you get into the situation where so some builds thing the style is ok, and some don't depending on clang-format version.

This is the way I did it in inotify-tools CI, is we only support clang-format greater than 11 because earlier versions reformat a little differently:

https://github.com/inotify-tools/inotify-tools/blob/8367bdd1eda71c10ab0a49c694846a622e543807/build_and_test.sh#L80

I didn't reformat the whole codebase in inotify-tools. Like I am proposing here, all new changes to inotify-tools are mandated to correspond with the .clang-format style. It avoids the "git blame" being confusing problem.


NOCONFIGURE=1 ./autogen.sh

Expand Down
1 change: 1 addition & 0 deletions ci/gh-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ case "$ID" in
build-essential
bubblewrap
ca-certificates
clang-format
cpio
debhelper
dh-exec
Expand Down