DAOS-16923 cq: make clang-format required #15685

daltonbohning · 2025-01-06T17:51:25Z

Make clang-format GHA required on PRs.

Skip-build: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Make clang-format GHA required on PRs. Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

github-actions · 2025-01-06T17:52:10Z

Ticket title is 'Make clang-format required on PRs'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-16923

daltonbohning · 2025-01-06T17:52:39Z

@mchaarawi Per request, it's this simple to make clang-format required. Should we alert devs before merging this?

daltonbohning · 2025-01-06T17:53:57Z

Note that since this is version controlled, inflight PRs will not be affected until this change is merged into their branch

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

Since it may conflict with system. Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

jolivier23 · 2025-01-07T23:50:14Z

I think you're going to get some pushback internally on this one. I'm personally ok with it.

jolivier23 · 2025-01-07T23:52:20Z

If it's required, does it mean a PR has to be force landed. You might want to add or point to documentation for the comments to disable it (such as for FOREACH macros) when it does weird things.

daltonbohning · 2025-01-08T16:13:16Z

If it's required, does it mean a PR has to be force landed. You might want to add or point to documentation for the comments to disable it (such as for FOREACH macros) when it does weird things.

If it's required, does it mean a PR has to be force landed

Right. It either needs to pass or needs force landing.

documentation for the comments to disable it (such as for FOREACH macros) when it does weird things

I don't work with C code enough to know - do you have an example of this? Or do you mean disable the hook with DAOS_GITHOOK_SKIP? (which would then require force landing)

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

grom72 · 2025-01-22T09:03:04Z

utils/githooks/README.md

+## Install DAOS Git Hooks
+Installing is a two-step process:


## Install DAOS Git Hooks Installing is a two-step process:

An empty line after ## ... is required.

Required? this seems to render correctly. I'll update though

grom72 · 2025-01-22T09:03:14Z

utils/githooks/README.md

-change with it.
-
-2. Install all of the required tools
+### 2. Install the required tools


### 2. Install the tools
Some of them are optional as mentioned below.

utils/githooks/README.md

grom72 · 2025-01-22T09:03:33Z

utils/githooks/README.md

+```
+To install system packages with your package manager - for example:
+```sh
+dnf install git-clang-format


sudo dnf install git-clang-format -y

grom72 · 2025-01-22T09:03:40Z

utils/githooks/README.md

+```sh
+dnf install git-clang-format
+```
+### Installed tools


#### Recommended tools
Is it a subsection of Install the~~ required~~ tools?

It was not, but makes sense. Thanks

grom72 · 2025-01-22T09:03:55Z

utils/githooks/README.md

+It is important to check the output on commit for any errors that may indicate
+any one of the required tools is missing.
+
+1. copyright - Custom tool that automatically updates copyrights in modified files.


The script's name is update-copyright.
and
why some of names are within '`'?
All names refer to *.sh scripts in utils\githooks\pre-commit.d.

Removed '`' and made names match script names

grom72 · 2025-01-22T09:04:08Z

utils/githooks/pre-commit.d/50-clang-format.sh

@@ -13,11 +14,11 @@ if [ -e .git/MERGE_HEAD ]; then
 fi

 if ! command -v git-clang-format > /dev/null 2>&1; then
-    echo "git-clang-format not installed. Skipping"
+    echo "git-clang-format not installed. See ./utils/githooks/README.md for instructions"
    exit 0


exit 1
Shall we also make git-clang-format not optional?

No objection from me. But TBH I don't write enough C code to know whether that will bother people

If we enable this in CI we should also enable it in githooks.
Otherwise build fails and user will have to make another commit to fix that issue.

I do agree with you. But in the past people have not liked when a githook prevents them from committing. Hopefully with updated documentation and it being required in the GHA, people will accept it 😛

If we get acceptance here we can also modify all other pre-commit scripts later to force everybody with pre-validation for every changes that are later verified by CI ;).

in the past people have not liked when a githook prevents them from committing

Funny thing clang changes over time and differs from distro to distro. So, you may even have clang-format pre-hook enabled and still be stopped by a check on GitHub. Just because of the different versions I believe.

Here --no-verify toggle becomes very useful. Just turn it off when it is in your way. ;-)

grom72 · 2025-01-22T09:04:09Z

utils/githooks/pre-commit.d/50-clang-format.sh

    exit 0
 fi
 if ! command -v clang-format > /dev/null 2>&1; then
-    echo "clang-format not installed. Skipping"
+    echo "clang-format not installed. See ./utils/githooks/README.md for instructions"
    exit 0


exit 1
Shall we also make git-clang-format not optional?

grom72 · 2025-01-22T09:05:59Z

utils/githooks/pre-commit.d/50-clang-format.sh

    exit 0
 fi
 if ! command -v clang-format > /dev/null 2>&1; then
-    echo "clang-format not installed. Skipping"
+    echo "clang-format not installed. See ./utils/githooks/README.md for instructions"


./utils/githooks/README.md#2-install-the-required-tools
Let's try to use link to particular section - helps a lot.

grom72 · 2025-01-22T09:07:13Z

utils/githooks/README.md


-It is important to check the output on commit for any errors that may indicate
-any one of the required tools is missing.
+### Optional tools


#### Optional tools
Is it a subsection of Install the~~ required~~ tools?

kccain · 2025-01-22T12:12:53Z

If it's required, does it mean a PR has to be force landed. You might want to add or point to documentation for the comments to disable it (such as for FOREACH macros) when it does weird things.

If it's required, does it mean a PR has to be force landed

Right. It either needs to pass or needs force landing.

documentation for the comments to disable it (such as for FOREACH macros) when it does weird things

I don't work with C code enough to know - do you have an example of this? Or do you mean disable the hook with DAOS_GITHOOK_SKIP? (which would then require force landing)

clang-format has made unwanted (human-unreadable) changes to RPC definition macros. Pretty bad. And in those cases we've had to repair this after the fact in subsequent commits by using C comments containing "clang-format off" and "clang-format on" to mark the region of the source code that clang-format should not modify. Example region with the control comments:

daos/src/pool/rpc.h

Line 103 in fe389f9

/* clang-format off */

...

daos/src/pool/rpc.h

Line 132 in fe389f9

/* clang-format on */

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

grom72 · 2025-01-23T14:53:30Z

utils/githooks/pre-commit.d/50-clang-format.sh

-    exit 0
+    echo "git-clang-format not installed."
+    echo "See ./utils/githooks/README.md#2-install-the-required-tools for instructions."
+    exit 1
 fi
 if ! command -v clang-format > /dev/null 2>&1; then


if ! command -v clang-format > /dev/null 2>&1; then
I think we can remove this check as:

there is a strict dependency between git-clang-format pkg and clang-tools-extra (clang-format) pkg. Every time

the presence of clang-format is checked by extra.py script few lines later

Thanks. I admit I don't know much about this tool

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

DAOS-16923 cq: make clang-format required

63d7743

Make clang-format GHA required on PRs. Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

daltonbohning self-assigned this Jan 6, 2025

daltonbohning requested review from gnailzenh, mchaarawi and jolivier23 January 6, 2025 17:51

daltonbohning marked this pull request as ready for review January 6, 2025 17:51

daltonbohning requested a review from a team as a code owner January 6, 2025 17:51

update githook README

1e6a7cd

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

daltonbohning requested a review from a team as a code owner January 6, 2025 19:04

daltonbohning added 2 commits January 6, 2025 19:09

backout scons from requirements.txt

16536e5

Since it may conflict with system. Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

... and in the readme

fbad815

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

jolivier23 previously approved these changes Jan 7, 2025

View reviewed changes

daltonbohning added 2 commits January 21, 2025 18:06

Merge remote-tracking branch 'origin/master' into dbohning/daos-16923

0e947c9

reference readme if clang-format not installed

13354b3

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

daltonbohning dismissed jolivier23’s stale review via 13354b3 January 21, 2025 18:45

grom72 requested changes Jan 22, 2025

View reviewed changes

daltonbohning added 3 commits January 22, 2025 16:40

review updates

23df06e

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

make clang-format githook required

d5bf037

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

reference build env

83e791e

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

grom72 requested changes Jan 23, 2025

View reviewed changes

daltonbohning added 2 commits January 23, 2025 16:45

Merge remote-tracking branch 'origin/master' into dbohning/daos-16923

c6f3369

remove unnecessary check

e34b6c1

Skip-build: true Signed-off-by: Dalton Bohning <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16923 cq: make clang-format required #15685

DAOS-16923 cq: make clang-format required #15685

daltonbohning commented Jan 6, 2025

github-actions bot commented Jan 6, 2025

daltonbohning commented Jan 6, 2025

daltonbohning commented Jan 6, 2025

jolivier23 commented Jan 7, 2025

jolivier23 commented Jan 7, 2025

daltonbohning commented Jan 8, 2025

grom72 Jan 22, 2025

daltonbohning Jan 22, 2025

grom72 Jan 22, 2025

grom72 Jan 22, 2025 •

edited

Loading

grom72 Jan 22, 2025

daltonbohning Jan 22, 2025

grom72 Jan 22, 2025

daltonbohning Jan 22, 2025

grom72 Jan 22, 2025

daltonbohning Jan 22, 2025

grom72 Jan 22, 2025

daltonbohning Jan 22, 2025

grom72 Jan 22, 2025

janekmi Jan 22, 2025

grom72 Jan 22, 2025

grom72 Jan 22, 2025

grom72 Jan 22, 2025

kccain commented Jan 22, 2025

grom72 Jan 23, 2025

daltonbohning Jan 23, 2025

DAOS-16923 cq: make clang-format required #15685

Are you sure you want to change the base?

DAOS-16923 cq: make clang-format required #15685

Conversation

daltonbohning commented Jan 6, 2025

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 6, 2025

daltonbohning commented Jan 6, 2025

daltonbohning commented Jan 6, 2025

jolivier23 commented Jan 7, 2025

jolivier23 commented Jan 7, 2025

daltonbohning commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grom72 Jan 22, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kccain commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grom72 Jan 22, 2025 •

edited

Loading