Skip to content

Use fixed clang format #1405

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

Closed
wants to merge 6 commits into from
Closed

Use fixed clang format #1405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b93a16f
adds pre-commit hooks
MarcelKoch Nov 14, 2023
1e51515
uses pre-commit in CI
MarcelKoch Nov 14, 2023
1db9847
removes git-cmake-format dependency
MarcelKoch Aug 17, 2023
370ff2a
review comments:
MarcelKoch Nov 13, 2023
6a98b11
only use allowed values in .clang-format
MarcelKoch Nov 14, 2023
d578455
run clang-format on all files
MarcelKoch Nov 14, 2023
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
14 changes: 7 additions & 7 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,26 @@ Language: Cpp
# BasedOnStyle: Google
AccessModifierOffset: -4
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignConsecutiveAssignments: None
AlignConsecutiveDeclarations: None
AlignEscapedNewlines: Left
AlignOperands: true
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortBlocksOnASingleLine: Never
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: All
AllowShortIfStatementsOnASingleLine: true
AllowShortLoopsOnASingleLine: true
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: true
AlwaysBreakTemplateDeclarations: true
AlwaysBreakTemplateDeclarations: Yes
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
AfterClass: false
AfterControlStatement: false
AfterControlStatement: Never
AfterEnum: false
AfterFunction: true
AfterNamespace: false
Expand Down Expand Up @@ -88,7 +88,7 @@ PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 200
PointerAlignment: Left
ReflowComments: true
SortIncludes: true
SortIncludes: CaseInsensitive
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: true
Expand All @@ -101,7 +101,7 @@ SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard: Cpp11
Standard: c++14
TabWidth: 8
UseTab: Never
...
Expand Down
28 changes: 10 additions & 18 deletions .github/bot-pr-format-base.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
#!/usr/bin/env bash

source .github/bot-pr-base.sh
cp .github/bot-pr-base.sh /tmp
source /tmp/bot-pr-base.sh

echo "Retrieving PR file list"
PR_FILES=$(bot_get_all_changed_files ${PR_URL})
NUM=$(echo "${PR_FILES}" | wc -l)
echo "PR has ${NUM} changed files"

TO_FORMAT="$(echo "$PR_FILES" | grep -E $EXTENSION_REGEX || true)"
echo "Set-up working tree"

git remote add fork "$HEAD_URL"
git fetch fork "$HEAD_BRANCH"
git fetch origin "$BASE_BRANCH"

# checkout current PR head
LOCAL_BRANCH=format-tmp-$HEAD_BRANCH
git checkout -b $LOCAL_BRANCH fork/$HEAD_BRANCH

git config user.email "ginkgo.library@gmail.com"
git config user.name "ginkgo-bot"

# save scripts from develop
pushd dev_tools/scripts
pushd dev_tools/scripts || exit 1
cp add_license.sh format_header.sh update_ginkgo_header.sh /tmp
popd
popd || exit 1

# checkout current PR head
LOCAL_BRANCH=format-tmp-$HEAD_BRANCH
Expand All @@ -28,12 +29,3 @@ git checkout -b $LOCAL_BRANCH fork/$HEAD_BRANCH
cp /tmp/add_license.sh dev_tools/scripts/
cp /tmp/format_header.sh dev_tools/scripts/
cp /tmp/update_ginkgo_header.sh dev_tools/scripts/

# format files
dev_tools/scripts/add_license.sh
dev_tools/scripts/update_ginkgo_header.sh
for f in $(echo "$TO_FORMAT" | grep -E $FORMAT_HEADER_REGEX); do dev_tools/scripts/format_header.sh "$f"; done
for f in $(echo "$TO_FORMAT" | grep -E $FORMAT_REGEX); do "$CLANG_FORMAT" -i -style=file "$f"; done

# restore formatting scripts so they don't appear in the diff
git checkout -- dev_tools/scripts/*.sh
10 changes: 10 additions & 0 deletions .github/check-format.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,27 @@
cp .github/bot-pr-format-base.sh /tmp
source /tmp/bot-pr-format-base.sh

echo -n "Run Pre-Commit checks"

pipx run pre-commit run --show-diff-on-failure --color=always --from-ref "origin/$BASE_BRANCH" --to-ref HEAD || true

echo -n "Collecting information on changed files"

# check for changed files, replace newlines by \n
LIST_FILES=$(git diff --name-only | sed '$!s/$/\\n/' | tr -d '\n')
echo -n .

git diff > /tmp/format.patch
mv /tmp/format.patch .
echo -n .

bot_delete_comments_matching "Error: The following files need to be formatted"
echo -n .

if [[ "$LIST_FILES" != "" ]]; then
MESSAGE="The following files need to be formatted:\n"'```'"\n$LIST_FILES\n"'```'
MESSAGE="$MESSAGE\nYou can find a formatting patch under **Artifacts** [here]"
MESSAGE="$MESSAGE($JOB_URL) or run "'`format!` if you have write access to Ginkgo'
bot_error "$MESSAGE"
fi
echo .
24 changes: 3 additions & 21 deletions .github/format-rebase.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,7 @@
#!/usr/bin/env bash

source .github/bot-pr-base.sh

git remote add base "$BASE_URL"
git remote add fork "$HEAD_URL"

git remote -v

git fetch base $BASE_BRANCH
git fetch fork $HEAD_BRANCH

git config user.email "$USER_EMAIL"
git config user.name "$USER_NAME"

LOCAL_BRANCH=rebase-tmp-$HEAD_BRANCH
git checkout -b $LOCAL_BRANCH fork/$HEAD_BRANCH

# save scripts from develop
pushd dev_tools/scripts
cp add_license.sh format_header.sh update_ginkgo_header.sh /tmp
popd
cp .github/bot-pr-format-base.sh /tmp
source /tmp/bot-pr-format-base.sh

bot_delete_comments_matching "Error: Rebase failed"

Expand All @@ -30,7 +12,7 @@ git rebase --rebase-merges --empty=drop --no-keep-empty \
--exec "cp /tmp/add_license.sh /tmp/format_header.sh /tmp/update_ginkgo_header.sh dev_tools/scripts/ && \
dev_tools/scripts/add_license.sh && dev_tools/scripts/update_ginkgo_header.sh && \
for f in \$($DIFF_COMMAND | grep -E '$FORMAT_HEADER_REGEX'); do dev_tools/scripts/format_header.sh \$f; done && \
for f in \$($DIFF_COMMAND | grep -E '$FORMAT_REGEX'); do $CLANG_FORMAT -i \$f; done && \
pipx run pre-commit run && \
git checkout dev_tools/scripts && (git diff >> /tmp/difflog; true) && (git diff --quiet || git commit -a --amend --no-edit --allow-empty)" \
base/$BASE_BRANCH 2>&1 || bot_error "Rebase failed, see the related [Action]($JOB_URL) for details"

Expand Down
22 changes: 20 additions & 2 deletions .github/format.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,29 @@
cp .github/bot-pr-format-base.sh /tmp
source /tmp/bot-pr-format-base.sh

echo "Retrieving PR file list"
PR_FILES=$(bot_get_all_changed_files ${PR_URL})
NUM=$(echo "${PR_FILES}" | wc -l)
echo "PR has ${NUM} changed files"

TO_FORMAT="$(echo "$PR_FILES" | grep -E $EXTENSION_REGEX || true)"

# format files
dev_tools/scripts/add_license.sh
dev_tools/scripts/update_ginkgo_header.sh
for f in $(echo "$TO_FORMAT" | grep -E $FORMAT_HEADER_REGEX); do dev_tools/scripts/format_header.sh "$f"; done
pipx run pre-commit run --files $TO_FORMAT || true

# restore formatting scripts so they don't appear in the diff
git checkout -- dev_tools/scripts/*.sh

# check for changed files, replace newlines by \n
LIST_FILES=$(git diff --name-only | sed '$!s/$/\\n/' | tr -d '\n')
CHANGES=$(git diff --name-only | sed '$!s/$/\\n/' | tr -d '\n')

echo "$CHANGES"

# commit changes if necessary
if [[ "$LIST_FILES" != "" ]]; then
if [[ "$CHANGES" != "" ]]; then
git commit -a -m "Format files

Co-authored-by: $USER_COMBINED"
Expand Down
25 changes: 8 additions & 17 deletions .github/workflows/bot-pr-comment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
name: OnCommentPR

on:
issue_comment:
types: [created]
name: OnCommentPR

jobs:
label:
runs-on: ubuntu-latest
Expand All @@ -15,25 +17,12 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.BOT_TOKEN }}
run: cp --preserve .github/label.sh /tmp && /tmp/label.sh

check_format:
name: check-format
runs-on: ubuntu-22.04
if: github.event.issue.pull_request != '' && github.event.comment.body == 'check-format!' && (github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')
steps:
- name: Checkout the latest code (shallow clone)
uses: actions/checkout@v3
with:
ref: develop
- name: Check for formatting changes
env:
GITHUB_TOKEN: ${{ secrets.BOT_TOKEN }}
run: cp --preserve .github/check-format.sh /tmp && /tmp/check-format.sh
- name: Upload code formatting patch
if: failure()
uses: actions/upload-artifact@v3
with:
name: patch
path: format.patch
uses: ./.github/workflows/check-formatting.yml

format:
name: format
runs-on: ubuntu-22.04
Expand All @@ -48,6 +37,7 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.BOT_TOKEN }}
run: cp --preserve .github/format.sh /tmp && /tmp/format.sh

rebase:
name: rebase
if: github.event.issue.pull_request != '' && github.event.comment.body == 'rebase!' && (github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')
Expand All @@ -63,6 +53,7 @@ jobs:
env:
GITHUB_TOKEN: ${{ secrets.BOT_TOKEN }}
run: cp --preserve .github/rebase.sh /tmp && /tmp/rebase.sh

format-rebase:
name: format-rebase
if: github.event.issue.pull_request != '' && github.event.comment.body == 'format-rebase!' && (github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')
Expand Down
28 changes: 9 additions & 19 deletions .github/workflows/bot-pr-updated.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,18 @@
name: OnSyncPR

on:
pull_request_target:
types: [opened,synchronize]
name: OnSyncPR

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
cancel-in-progress: true

jobs:
check-format:
runs-on: ubuntu-22.04
if: github.event.pull_request.author_association == 'COLLABORATOR' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'OWNER'
steps:
- name: Checkout the latest code (shallow clone)
uses: actions/checkout@v3
with:
ref: develop
- name: Check for formatting changes
env:
GITHUB_TOKEN: ${{ secrets.BOT_TOKEN }}
run: cp .github/check-format.sh /tmp && /tmp/check-format.sh
- name: Upload code formatting patch
if: failure()
uses: actions/upload-artifact@v3
with:
name: patch
path: format.patch
uses: ./.github/workflows/check-formatting.yml

abidiff:
Copy link
Member

Choose a reason for hiding this comment

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

abidiff is deleted accidentally

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I will revert that, when I prepare this branch for merging

runs-on: ubuntu-latest
if: github.event.pull_request.author_association == 'COLLABORATOR' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'OWNER'
Expand Down Expand Up @@ -58,8 +47,9 @@ jobs:
if: failure()
uses: actions/upload-artifact@v3
with:
name: abi
path: abi.diff
name: abi
path: abi.diff

check-wiki-changelog:
runs-on: ubuntu-latest
if: github.event.pull_request.author_association == 'COLLABORATOR' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'OWNER'
Expand Down
21 changes: 21 additions & 0 deletions .github/workflows/check-formatting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: Check formatting
on: workflow_call

jobs:
pre-commit:
name: Run pre-commit hooks
runs-on: ubuntu-latest
steps:
- name: Checkout the latest code (shallow clone)
uses: actions/checkout@v4
- name: Run Pre-Commit checks
run: cp .github/check-format.sh /tmp && /tmp/check-format.sh
id: pre-commit
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Upload code formatting patch
if: ${{ failure() }}
uses: actions/upload-artifact@v3
with:
name: patch
path: format.patch
7 changes: 7 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
repos:
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: 'v14.0.0' # The default in Ubuntu 22.04, which is used in our CI
Comment on lines +1 to +3
Copy link
Member

Choose a reason for hiding this comment

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

because it install clang-format by pip, I wonder it affects/updates the local python package without notification.
Do you know how pre-commit manages the python environment and whether we can use this clang-format in editor?
nit: use two space to be consistent with the other yaml format

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know, how the user environment is changes. But what I can tell you is that on my system, where I use pre-commit, I don't have clang-14 in my path, even though it is used by pre-commit.

Copy link
Member

Choose a reason for hiding this comment

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

After some search, it is handled by python virtualenv.
The default folder is ~/.cache/pre-commit https://[pre-commit](https://pre-commit.com/#managing-ci-caches).com/#managing-ci-caches
there's a folder (py_env-python3.9) under a random folder, source py_env-python3.9/bin/activate can run clang-format from the environment.
I am still looking for how to get the path correctly and use it in editor

Copy link
Member Author

Choose a reason for hiding this comment

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

For what do you need it in your editor? You can just run pre-commit run and it will do the formatting.

Copy link
Member

Choose a reason for hiding this comment

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

I usually run several times clang-format during coding with the default shortcut.
pre-commit requires us to run via command and pass files (or all-files)

Copy link
Member

@yhmtsai yhmtsai Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, I mean no-concatenation will introduce issue from inconsistent clang-format.
local clang-format may break some comments which are considered long with this format. but pre-commit one does not break in that format.
For example, assuming pre-commit clang-format only puts the normal indentation for arguments, but local clang-format aligns the arguments
1 is the original code

void func(...,
          tttt // some text ...

2 is from local clang-format (break when it is too long)

void func(...,
          tttt // some text
               // more text

3 is format 1 from pre-commit clang-format, less indention so no need for breaking

void func(...,
    tttt // some text ...

4 is format 2 from pre-commit clang-format, no-concatenation if it is already split

void func(...,
    tttt // some text
         // more text

3 and 4 are different. If we apply format directly, we should get 3.
With inconsistent clang-format, we may get 4 in the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

can you still provide your clang-format version? I will still look into the issue you mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I think this might be one of those issues which just change depending on the clang format versions.
Just as an example, I used this:

    LinOp* apply(ptr_param<const LinOp> alpha,
                 ptr_param<const LinOp> b, // tis a long comment describing the parametr b for what ever reasons
                 ptr_param<const LinOp> beta,
                 ptr_param<LinOp> x)

and the pre-commit gave me:

    LinOp* apply(ptr_param<const LinOp> alpha,
                 ptr_param<const LinOp> b,  // tis a long comment describing the
                                            // parametr b for what ever reasons
                 ptr_param<const LinOp> beta, ptr_param<LinOp> x)

which is your 2. But maybe it depends more on the actual example.

Copy link
Member

@yhmtsai yhmtsai Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The issue happens when the different indention format clang-format, that's why the example is under the assumption.
For more realistic example, you need to look for the attribute usage.
Clang-format does not give any consistence between version if no option to control behavior such that we may have different indention. It's also why we need to fix a version for repo code not just rely on the config version requirement. There are always some undefined behavior across versions.
Otherwise, the config only requires 9+ and format_header only requires 12+.
clang-format 9, 12, 14, 17

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I understand your point. The whole point of this PR is to fix a clang-format version that is used both in the CI and each local repo.

hooks:
- id: clang-format
types_or: [c, c++, cuda, inc]
exclude: third_party/SuiteSparse/AMD/.*
33 changes: 27 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,13 +354,34 @@ if(GINKGO_BUILD_BENCHMARKS)
endif()

if(GINKGO_DEVEL_TOOLS)
add_custom_target(add_license
COMMAND ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/add_license.sh
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR})
# if git-cmake-format can not build format target, do not add the dependencies
if(TARGET format)
add_dependencies(format add_license)
find_program(PRE_COMMIT pre-commit)
if(NOT PRE_COMMIT)
message(FATAL_ERROR "The pre-commit command was not found. It is necessary if you want to commit changes to Ginkgo. "
"If that is not the case, set GINKGO_DEVEL_TOOLS=OFF. "
"Otherwise install pre-commit via pipx (or pip) using:\n"
" pipx install pre-commit")
endif()

execute_process(COMMAND "${PRE_COMMIT}" "install"
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR}
RESULT_VARIABLE pre-commit-result
OUTPUT_VARIABLE pre-commit-output
ERROR_VARIABLE pre-commit-error)
if(pre-commit-result)
message(FATAL_ERROR
"Failed to install the git hooks via pre-commit. Please check the error message:\n"
"${pre-commit-output}\n${pre-commit-error}")
endif()

add_custom_target(format
COMMAND bash -c "${PRE_COMMIT} run"
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR}
VERBATIM)

add_custom_target(add_license
COMMAND ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/add_license.sh
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR})
add_dependencies(format add_license)
endif()

# MacOS needs to install bash, gnu-sed, findutils and coreutils
Expand Down
4 changes: 2 additions & 2 deletions core/test/base/deferred_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ struct test_impl<gko::xstd::void_t<decltype(T(std::declval<Args>()...))>, T,

// specialization for DF2 with_factory_list
template <typename... Args>
struct test_impl<gko::xstd::void_t<decltype(DF2::param{}.with_factory_list(
std::declval<Args>()...))>,
struct test_impl<gko::xstd::void_t<decltype(
DF2::param{}.with_factory_list(std::declval<Args>()...))>,
DummyFlag, Args...> : std::true_type {};

// test the object can be constructable or not with Args.
Expand Down
Loading