Use fixed clang format #1461

MarcelKoch · 2023-11-14T15:21:48Z

Old PR: #1404

This PR enables us to force users to use the same version of clang-format as our CI. This is done by requiring contributors to install pre-commit (which can be done easily through pip/pipx). pre-commit allows us to fix the used version of clang-format, so that contributors will use exactly the same version as our CI.

If pre-commit can't be detected during cmake an error is thrown. Otherwise, the pre-commit hooks are installed.

Since this replaces our git-cmake-format, I have removed it.

To see it in action: here is an PR in a fork that shows the CI use cases MarcelKoch#9

Additional changes:

change not supported values in .clang-format
run pre-commit checks over all files

Warning

For those using git-worktrees, if one working tree is updated with this PR it will overwrite the git hooks for all working trees (I don't think you can have different hooks for different working trees). The easy fix is of course to just update all used working trees accordingly.

Note: I think the previous PR #1405 got closed because it was based not on develop and it was from my fork, but who knows?

MarcelKoch · 2023-11-14T15:31:21Z

When this is merged, we should merge ginkgo-project/git-cmake-format#13

MarcelKoch · 2023-11-14T15:58:54Z

I think the current check-format error is again due to the CI using the configuration from develop. So we have to ignore it again.

yhmtsai

should old pr be #1405 ?
Could you add the pre-commit requirement into the README.md and pre-commit and mirrors-clang-format license into ABOUT-LICENSING.md.
I just noticed ABOUT-LICENSING render is broken. Could you also fix them?

.github/workflows/check-formatting.yml

yhmtsai · 2023-11-15T08:57:27Z

.pre-commit-config.yaml

+- 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
+ hooks:
+ - id: clang-format
+ types_or: [c, c++, cuda, inc]
+ exclude: third_party/SuiteSparse/AMD/.*


nit: 2 space indention as the other yaml

yhmtsai · 2023-11-15T09:01:20Z

examples/kokkos_assembly/kokkos_assembly.cpp

 #include <omp.h>
-#include <Kokkos_Core.hpp>
 #include <ginkgo/ginkgo.hpp>
+#include <Kokkos_Core.hpp>


I think it should be separated

yhmtsai · 2023-11-15T09:20:27Z

.github/workflows/check-formatting.yml

+jobs:
+ pre-commit:
+ name: Run pre-commit hooks
+ runs-on: ubuntu-latest


Suggested change

runs-on: ubuntu-latest

runs-on: ubuntu-22.04

because format_header currently still use the system clang-format-14, using 22.04 might be more stable

I will change this, but what is the purpose of format_header.sh anyway? It seems to reorder the includes, but that should be covered by clang-format already. Special handling like the force-on-top could be handled by disabling clang-format.

no, clang-format reordering does not fit our main header (it's somehow complex, so I would also like to change) and the ordering can not be handled across sections (or across-section reorder will only contain one space line for sep?)

the format_header did two things:

put the correct main header on the top

put all the other headers together, reorder the header with clang-format 12+ and then replace one line sep -> two line sep.

There is a way to achieve 1. with clang-format. But that requires changing

#include <ginkgo/core/what/ever.hpp>

into

#include "ginkgo/core/what/ever.hpp"

So clang-format recognizes the main header of a source file only if its in "", but not if its in <>.
I will also look into 2.

No, the main header issue is we have several rules for main header. Please take a look at https://github.com/ginkgo-project/ginkgo/blob/develop/dev_tools/scripts/config
Some of them could be merged, but it's still complex if we do not have certain forced rules.
We try writing the general rules in https://github.com/ginkgo-project/ginkgo/wiki/Contributing-guidelines#main-header
From https://clang.llvm.org/docs/ClangFormatStyleOptions.html#includeismainregex, you can only have simple regex for all of them.

Thanks for looking at that, look forward to avoiding complex format_header.
I suggest it should be another pr if you find something easy to maintain.

MarcelKoch · 2023-11-15T09:36:02Z

@yhmtsai I'm not sure if we need to add either pre-commit, or the clang-format hook to our licensing description. Our ginkgo package (the source code + whatever is in the third_party stuff) does not contain any code or binaries from either pre-commit or clang-format. The pre-commit config could be licensed, but we didn't get into that yet.

yhmtsai · 2023-11-15T09:39:54Z

just copy paste the comment from #1405 if want to use clang-format from pre-commit in the editor. (easier find in the future)
It's not pre-commit intended usage. It might be failed if they change the cache way.
The following script such that we can use the auto installation in editor easily, too.

#!/usr/bin/env bash

PRE_COMMIT_CACHE="${HOME}/.cache/pre-commit"
# get the python environment folder from pre-commit
VENV="$(sqlite3 -header -csv ${PRE_COMMIT_CACHE}/db.db "select * from repos;" | grep clang-format,v14.0.0 | sed -E 's/.*,.*,(.*)/\1/g')"
ACTIVATE="$(find ${VENV} -name activate -print -quit)"
# activate env
source "${ACTIVATE}"
# forward the arguments
clang-format "$@"

yhmtsai

Something I can not put comments directly but needs updates:

git-cmake-format can be deleted in ABOUT-LICENSING.md
INSTALL.md replaces git-cmake-format by pre-commit
delete third-party git-cmake-format
dummy_hook/CMakeLists.txt needs to update

yhmtsai · 2023-11-16T11:15:46Z

include/ginkgo/core/log/papi.hpp

 #include <mutex>
+
+
 #include <sde_lib.h>


actually, I do not get this changes.
do you add new breaks and only run format without format_header?

Yes, I forgot the format_header. I will also run this.

although I do not know why the format_header change it, it shuold keep everything in #if block

just go through again the format_header. the thing in #if only raises a warning only. the header is still considered in clang-format with regroup (in format_header)

MarcelKoch · 2023-11-16T14:47:44Z

format!

- fix format-rebase.sh - yml formatting - update checkout action version - fix ABOUT-LICENSING.md formatting - use fixed ubuntu version Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>

sonarcloud · 2023-11-16T21:50:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

This merge fixes a mistake I made merging #1461. The issue was that the bot_pr_format_base.sh script tried to create the same branch twice. Related PR: #1469

MarcelKoch self-assigned this Nov 14, 2023

MarcelKoch requested a review from yhmtsai November 14, 2023 15:22

MarcelKoch requested a review from upsj November 14, 2023 15:22

upsj approved these changes Nov 14, 2023

View reviewed changes

upsj added the 1:ST:no-changelog-entry Skip the wiki check for changelog update label Nov 14, 2023

MarcelKoch force-pushed the use-fixed-clang-format branch from d578455 to f89317d Compare November 15, 2023 08:33

yhmtsai requested changes Nov 15, 2023

View reviewed changes

yhmtsai reviewed Nov 15, 2023

View reviewed changes

yhmtsai approved these changes Nov 16, 2023

View reviewed changes

yhmtsai reviewed Nov 16, 2023

View reviewed changes

MarcelKoch force-pushed the use-fixed-clang-format branch from 8653ec9 to e2418ca Compare November 16, 2023 14:52

MarcelKoch and others added 6 commits November 16, 2023 15:53

adds pre-commit hooks

c479bbb

uses pre-commit in CI

84652dd

removes git-cmake-format dependency

25face6

only use allowed values in .clang-format

77e2387

run clang-format on all files

9b8c601

review comments:

1cce135

- fix format-rebase.sh - yml formatting - update checkout action version - fix ABOUT-LICENSING.md formatting - use fixed ubuntu version Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>

MarcelKoch force-pushed the use-fixed-clang-format branch from e2418ca to 1cce135 Compare November 16, 2023 14:53

upsj added the 1:ST:ready-to-merge This PR is ready to merge. label Nov 16, 2023

MarcelKoch merged commit fa71990 into develop Nov 16, 2023
10 of 15 checks passed

MarcelKoch deleted the use-fixed-clang-format branch November 16, 2023 18:02

MarcelKoch mentioned this pull request Nov 24, 2023

fixup checking out branch for CI #1469

Merged

MarcelKoch added a commit that referenced this pull request Nov 24, 2023

Merge CI checkout fix

5cc844f

This merge fixes a mistake I made merging #1461. The issue was that the bot_pr_format_base.sh script tried to create the same branch twice. Related PR: #1469

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fixed clang format #1461

Use fixed clang format #1461

MarcelKoch commented Nov 14, 2023 •

edited

Loading

MarcelKoch commented Nov 14, 2023

MarcelKoch commented Nov 14, 2023

yhmtsai left a comment

yhmtsai Nov 15, 2023

yhmtsai Nov 15, 2023

yhmtsai Nov 15, 2023

MarcelKoch Nov 15, 2023

yhmtsai Nov 15, 2023

yhmtsai Nov 15, 2023 •

edited

Loading

MarcelKoch Nov 15, 2023

yhmtsai Nov 15, 2023

MarcelKoch commented Nov 15, 2023

yhmtsai commented Nov 15, 2023

yhmtsai left a comment

yhmtsai Nov 16, 2023

MarcelKoch Nov 16, 2023

yhmtsai Nov 16, 2023

yhmtsai Nov 16, 2023

MarcelKoch commented Nov 16, 2023

sonarcloud bot commented Nov 16, 2023

Use fixed clang format #1461

Use fixed clang format #1461

Conversation

MarcelKoch commented Nov 14, 2023 • edited Loading

Warning

MarcelKoch commented Nov 14, 2023

MarcelKoch commented Nov 14, 2023

yhmtsai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhmtsai Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcelKoch commented Nov 15, 2023

yhmtsai commented Nov 15, 2023

yhmtsai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcelKoch commented Nov 16, 2023

sonarcloud bot commented Nov 16, 2023

MarcelKoch commented Nov 14, 2023 •

edited

Loading

yhmtsai Nov 15, 2023 •

edited

Loading