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

Clang format proposal v4 #5186

Closed
wants to merge 4 commits into from
Closed

Clang format proposal v4 #5186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94ea90a
Add clang-format file
roligugus Jul 7, 2020
af5cbd8
Add dev code-style
roligugus Jul 3, 2020
7df3813
Add make support for clang-format
roligugus Jul 3, 2020
8429123
Run formatting check on pull request
roligugus Jul 15, 2020
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
43 changes: 43 additions & 0 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Suricata settings as per
roligugus marked this conversation as resolved.
Show resolved Hide resolved
# https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style
roligugus marked this conversation as resolved.
Show resolved Hide resolved
#
# Some discussions to be had. See companion sample_formatted_code.do_not_merge.c
# for what this looks like.
---
BasedOnStyle: LLVM
AlignAfterOpenBracket: DontAlign
AlignConsecutiveMacros: true
AlignEscapedNewlines: Left
# clang 10: AllowShortBlocksOnASingleLine: Never
# clang 11: AllowShortEnumsOnASingleLine: false
AllowShortFunctionsOnASingleLine: None
# BreakBeforeBraces: Mozilla is closest, but does not split empty functions/structs
BraceWrapping:
AfterClass: true
AfterControlStatement: false
AfterEnum: false
AfterFunction: true
AfterStruct: false
AfterUnion: false
AfterExternBlock: true
BeforeElse: false
IndentBraces: false
SplitEmptyFunction: true
SplitEmptyRecord: true
BreakBeforeBraces: Custom
Cpp11BracedListStyle: true
ConstructorInitializerIndentWidth: 8
ContinuationIndentWidth: 8
ForEachMacros: ['json_array_foreach', 'json_object_foreach']
IndentCaseLabels: true
IndentWidth: 4
ReflowComments: true
SortIncludes: false

# implicit by LLVM style
#BreakBeforeTernaryOperators: true
#ColumnLimit: 80
#UseTab: Never
#TabWidth: 8

...
103 changes: 103 additions & 0 deletions .github/workflows/formatting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
name: formatting-check
roligugus marked this conversation as resolved.
Show resolved Hide resolved

on:
push:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enables running the checks on pushes to private forks which I kinda like. No need to create local pull requests to have formatting checked.

# Only run on pushes to pull request branches
branches-ignore:
- 'master'
- 'master-*'
pull_request:

jobs:

# Checking for correct formatting of branch for C code changes
check-formatting:
name: Formatting Check (clang 9)
roligugus marked this conversation as resolved.
Show resolved Hide resolved
runs-on: ubuntu-18.04
container: ubuntu:18.04
steps:

# Cache Rust stuff.
- name: Cache cargo registry
uses: actions/cache@v1
with:
path: ~/.cargo/registry
key: cargo-registry

- name: Install dependencies
run: |
apt update
apt -y install \
libpcre3 \
libpcre3-dev \
build-essential \
autoconf \
automake \
git \
libtool \
libpcap-dev \
libnet1-dev \
libyaml-0-2 \
libyaml-dev \
libcap-ng-dev \
libcap-ng0 \
libmagic-dev \
libnetfilter-queue-dev \
libnetfilter-queue1 \
libnfnetlink-dev \
libnfnetlink0 \
libhiredis-dev \
libjansson-dev \
libpython2.7 \
make \
python \
rustc \
software-properties-common \
wget \
zlib1g \
zlib1g-dev
- name: Install packages for clang-format 9
Copy link
Contributor Author

@roligugus roligugus Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Clang-format 9 is minimal requirement for the chosen .clang-format settings.

Tested with clang 10, clang 11 (trunk) as well. See overall pull request comments

run: |
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add -
# no need to install full clang using https://apt.llvm.org/llvm.sh
# using clang 9
add-apt-repository "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-9 main"
apt-get update
apt-get install -y clang-format-9
- name: Install cbindgen
run: cargo install --force --debug --version 0.14.1 cbindgen
- run: echo "::add-path::$HOME/.cargo/bin"
# If master has newer commits, github adds a merge commit from master into
roligugus marked this conversation as resolved.
Show resolved Hide resolved
# our branch.
# This screws up git clang-format as it'll format the merge as well and
# reports any formatting issues in those commits on master as well.
# We don't care about them here. So let's use the real branch HEAD commit.
- name: Checkout HEAD on branch, not merge commit!
uses: actions/checkout@v1
with:
ref: ${{ github.head_ref }}
# ref: ${{ github.event.pull_request.head.sha }}
- run: git clone https://github.com/OISF/libhtp -b 0.5.x
- run: ./autogen.sh
- run: ./configure --enable-unittests
- name: Check formatting
run: |
# Do not use make as it hides the script's exit code
# make diffstat-style-branch >> check_formatting_log.txt 2>&1
./scripts/clang-format.sh check-branch --make --diffstat --show-commits >> check_formatting_log.txt 2>&1
rc=$?
if [ $rc -eq 0 ]; then
cat check_formatting_log.txt
echo "Formatting is following code style guide"
elif [ $rc -eq 1 ]; then
# limit output as it might be a lot in the worst case
tail -n 100 check_formatting_log.txt
echo "::warning ::Formatting is not following code style guide!"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be trivial to switch formatting problems to an error here.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too keen on that right now.

Copy link
Member

Choose a reason for hiding this comment

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

Can we make it an error but start out with having the job be allowed to fail? This way we'll get visual feedback, but it won't fail the build. On of our travis tests is set up this way. Not sure if github actions allows for it.

Copy link
Contributor Author

@roligugus roligugus Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just played around a bit.

There's the continue-on-error one can set for a job which would allow us to set an "error" in the check-formatting job. However, the visual representation still sucks in github. It'll still set the job to green.

From what i can gather it's a limitation of github compared to e.g. Travis (and most other CI solutions). See open feature request: https://github.com/actions/toolkit/issues/399

There's also a way to have a job fail and then still run follow-on jobs with e.g. if: always(). However, that means configuring the follow-on jobs to still run if any of the previous jobs failed. No idea, how github decides on the order of jobs and dependencies...

Copy link
Contributor Author

@roligugus roligugus Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

To be more precise, setting continue-on-error while not perfect is much better.

It'll show the "formatting-check" as failed in the pull-request's checks overview. See e.g. roligugus#5
Don't know if that would prevent you from merging a pull request if formatting was not ok.

Compare that with the previous approach of a warning only and formatting-check always succeeded.

Copy link
Contributor Author

@roligugus roligugus Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like that the continue-on-error setting is almost useless as it seems to only affect the yaml's jobs it is in. It could be interesting if ever a rustfmt/cargo fmt check was added into the same formatting-check.yaml though.

Anyhow, it seems one can pick which status checks that have to pass in your "Branch Protection Rules", see https://docs.github.com/en/github/administering-a-repository/enabling-required-status-checks

So as long as you don't require the formatting-check to pass, you can merge pull requests.

Hence, I'd recommend the formatting-check to error out in case of wrong formatting problems and not pick it as a required "status check" for merging purposes. That way, you'd see the red failure indication in the pull request, yet your merging is not blocked. I.e. it's up to you as maintainers to decide whether to request a formatting change on the pull request or merge something in anyways.

@jasonish and @victorjulien Any thoughts on such an approach?

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, we can always disable it again if we find it to be too intrusive.

else
cat check_formatting_log.txt
# use last output line as error
last_line=$(tail -n 1 check_formatting_log.txt)
echo "::error ::$last_line"
exit 1
fi
shell: bash {0}
34 changes: 34 additions & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,37 @@ endif
@echo "For more information please see:"
@echo " https://suricata.readthedocs.io/en/latest/rule-management/index.html"
@echo ""

#--- clang-formatting ---
# Check if branch changes formatting is correct
.PHONY: check-style-branch
Copy link
Contributor Author

@roligugus roligugus Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These targets help with github actions. More importantly, they help with reformatting code and whole branches. See https://github.com/roligugus/suricata/blob/clang-format-proposal-v4/doc/devguide/codebase/code-style.rst#use-make-targets

The makefile targets, as well as the underlying script that gets called, also isolate the dev from knowing which git-flang-format version to call (on ubuntu).

Copy link
Member

Choose a reason for hiding this comment

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

Do these script wrappers in the Makefile actually help with github actions over just calling the scripts directly? I ask as they seem to be direct wrappers over scripts, and often the scripts are easier to discover, so I wonder the value of providing these as mark targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, they do not matter for the actions, as that is all scripted. In fact I no longer use make as it
hides the scripts exit code" there.

I only added the make targets as an option and alternative to use for people essentially. The idea was that it might be easier for people to remember some of the more complicated calls such as make diffstat-style-branch than ./scripts/clang-format.sh check-branch --make --diffstat --show-commits. Having said that, for the ones that mostly count for devs, there is not much difference directly calling the script.

Some projects like having that option, some not. I am usually in the latter part to be honest, but wanted to give you guys an option. It can easily be removed if not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any quick yay or nay on whether to keep the make targets? I get the feeling from @jasonish they should rather be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the make targets for v5. Agree with @jasonish that they are not that useful

check-style-branch:
./scripts/clang-format.sh check-branch --make

# Check if branch changes formatting is correct and print diff
.PHONY: diff-style-branch
diff-style-branch:
./scripts/clang-format.sh check-branch --make --diff

# Check if branch changes formatting is correct and print diffstat, i.e. files
.PHONY: diffstat-style-branch
diffstat-style-branch:
./scripts/clang-format.sh check-branch --make --diffstat --show-commits

# Format changes in git staging
.PHONY: style
style:
./scripts/clang-format.sh cached

# Reformat all changes in branch. Allows you to add the formatting as a separate
# commit "at the end".
.PHONY: style-branch
style-branch:
./scripts/clang-format.sh branch

# Reformat all commits in branch off master and rewrite history using the
# existing commit metadata.
.PHONY: style-rewrite-branch
style-rewrite-branch:
./scripts/clang-format.sh rewrite-branch

135 changes: 135 additions & 0 deletions clang-format.full.do_not_merge
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
# To use minimalistic .clang-format with BasedOnStyle: or the full shebang like
# this?
roligugus marked this conversation as resolved.
Show resolved Hide resolved
#
# This probably needs some more pruning if we wanted the full thing, such as
# cleaning up IncludeCategories (that is not used as SortIncludes:false).
#
# Reproduce from minimalistic .clang-format with:
# clang-format --dump-config --style=file >clang-format.full.do_not_merge
#
---
Language: Cpp
AccessModifierOffset: -2
AlignAfterOpenBracket: DontAlign
AlignConsecutiveMacros: true
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands: true
AlignTrailingComments: true
AllowAllArgumentsOnNextLine: true
AllowAllConstructorInitializersOnNextLine: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: None
AllowShortLambdasOnASingleLine: All
AllowShortIfStatementsOnASingleLine: Never
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: MultiLine
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
AfterCaseLabel: false
AfterClass: true
AfterControlStatement: false
AfterEnum: false
AfterFunction: true
AfterNamespace: false
AfterObjCDeclaration: false
AfterStruct: false
AfterUnion: false
AfterExternBlock: true
BeforeCatch: false
BeforeElse: false
IndentBraces: false
SplitEmptyFunction: true
SplitEmptyRecord: true
SplitEmptyNamespace: true
BreakBeforeBinaryOperators: None
BreakBeforeBraces: Custom
BreakBeforeInheritanceComma: false
BreakInheritanceList: BeforeColon
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: false
BreakConstructorInitializers: BeforeColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
ColumnLimit: 80
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 8
ContinuationIndentWidth: 8
Cpp11BracedListStyle: true
DerivePointerAlignment: false
DisableFormat: false
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true
ForEachMacros:
- json_array_foreach
- json_object_foreach
- BOOST_FOREACH
IncludeBlocks: Preserve
IncludeCategories:
- Regex: '^"(llvm|llvm-c|clang|clang-c)/'
Priority: 2
- Regex: '^(<|"(gtest|gmock|isl|json)/)'
Priority: 3
- Regex: '.*'
Priority: 1
IncludeIsMainRegex: '(Test)?$'
IndentCaseLabels: true
IndentPPDirectives: None
IndentWidth: 4
IndentWrappedFunctionNames: false
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: true
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBinPackProtocolList: Auto
ObjCBlockIndentWidth: 2
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyBreakTemplateDeclaration: 10
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 60
PointerAlignment: Right
ReflowComments: true
SortIncludes: false
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterLogicalNot: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: ControlStatements
SpaceBeforeRangeBasedForLoopColon: true
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard: Cpp11
StatementMacros:
- Q_UNUSED
- QT_REQUIRE_VERSION
TabWidth: 8
UseTab: Never
...

Loading