Codecov Report

Attention: Patch coverage is 83.18584% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 70.19%. Comparing base (4e44f5a) to head (0da6350).

@@             Coverage Diff              @@
##           unstable     #538      +/-   ##
============================================
- Coverage     70.20%   70.19%   -0.02%     
============================================
  Files           109      109              
  Lines         59913    59916       +3     
============================================
- Hits          42064    42056       -8     
- Misses        17849    17860      +11     

@bjosv PTAL

We have been using jidicula/clang-format-action successfully for a while, so it seems like a good use here.
The example steps above should work I believe.

We can use this action to avoid implementing this: https://github.com/jidicula/clang-format-action

Can we add a README for how to run this locally if we use some 3P validator. I don't want to have to submit a "dummy" PR to check the format.

Can we add a README for how to run this locally

Let's add it in CONTRIBUTING.md?

We can use this action to avoid implementing this: https://github.com/jidicula/clang-format-action

Then the job becomes

    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6
    - name: Run clang-format style check (.c and .h)
      uses: jidicula/clang-format-action@c74383674bf5f7c69f60ce562019c1c94bc1421a # v4.13.0
      with:
        check-path: 'src'

This action doesn't seem to honor our clang-format settings? https://github.com/valkey-io/valkey/actions/runs/9211589156/job/25341302397?pr=538

any tips?

@bjosv can you please take a look at this PR when you get a chance? I suspect that jidicula/clang-format-action is not picking up the .clang-format config file. However, I noticed that you have a .clang-format file in libvalkeycluster. Does it work for you?

I have only used it in non-valkey repos sofar, but we will get it up and running in libvalkey soon.
I have tried it a bit and I get the same result using something like:

    - name: Run clang-format style check (.c and .h)
      uses: jidicula/clang-format-action@c74383674bf5f7c69f60ce562019c1c94bc1421a # v4.13.0
      with:
        exclude-regex: '^\./deps/|^\./tests/|^\./utils/'

...
src/rand.c:48:30: error: code should be clang-formatted [-Wclang-format-violations]
#define LOW(x) ((unsigned)(x) & MASK)
...

but I also get similar changes when running manually in the repo, like:
clang-format -i src/rand.c
gives

+++ b/src/rand.c
@@ -45,7 +45,7 @@
 
 #define N 16
 #define MASK ((1 << (N - 1)) + (1 << (N - 1)) - 1)
-#define LOW(x) ((unsigned)(x) & MASK)
+#define LOW(x) ((unsigned)(x)&MASK)

Should unstable be up-to-date with clang-format already, or is the checker correct?

In your initial commit where the clang-format was manually installed on a ubuntu-latest (i.e. ubuntu-22.04) you probably got clang-format-14. Not sure if that is our problem though..

Thanks @bjosv!

The below change looks weird to me. I haven't figured a way to tell clang-format to remove the spaces aroud "&". All the styles I've tried left spaces around "&" with or without a .clang-format. I think I will go back to installing my own clang-format for now. Don't want to leave this action hanging for too long.

-#define LOW(x) ((unsigned)(x) & MASK)
+#define LOW(x) ((unsigned)(x)&MASK)

I haven't figured a way to tell clang-format to remove the spaces aroud "&"

This seems fixed in clang-format-17, but I couldn't find anything in release notes..

Then, we have another issue. The .clang-format-ignore file is only supported from clang-format-18, so using

    - name: Run clang-format style check (.c and .h)
      uses: jidicula/clang-format-action@c74383674bf5f7c69f60ce562019c1c94bc1421a # v4.13.0
      with:
        clang-format-version: 18
        check-path: src

works a lot better, but still.. there are some few additional changes needed.

I haven't figured a way to tell clang-format to remove the spaces aroud "&"

This seems fixed in clang-format-17, but I couldn't find anything in release notes..

Then, we have another issue. The .clang-format-ignore file is only supported from clang-format-18, so using

    - name: Run clang-format style check (.c and .h)
      uses: jidicula/clang-format-action@c74383674bf5f7c69f60ce562019c1c94bc1421a # v4.13.0
      with:
        clang-format-version: 18
        check-path: src

works a lot better, but still.. there are some few additional changes needed.

Thanks @bjosv. That makes a lot of sense. Let me try out jidicula/clang-format-action with version 18 again.

So I ended up using a custom action.

1. jidicula/clang-format-action uses clang-format 18.1.6 but I only managed to install 18.1.3 on my box. These two however don't agree with each other even when running the same clang-format configs
2. jidicula/clang-format-action doesn't print out the diff (to my knowledge). This plus 1) above makes it harder to figure out the formatting errors.
3. I had a lot of trouble getting clang-format 19 installed (due to the dependency on libffi7) but installing clang-format 18 was very straightforward. I am guessing this might be a common case for other people as well so I am sticking with 18.
4. there is however a bug in clang-format 18 (seems to have been fixed in 19) which incorrectly treats "new", "delete" and "not" as reserved keywords and this messes up the formatting. This is the reason for the renames in acl.c/listpack.c/utils.c.
5. the custom action prints out the actual diffs when formatting errors are detected, making it easier to fix formatting errors without using a locally installed clang-format.

[Will add to CONTRIBUTING.md]
How to install clang-format 18 on a Debian box

sudo apt-get update -y
sudo apt-get upgrade -y
sudo apt-get install software-properties-common -y
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | gpg --dearmor | sudo tee /usr/share/keyrings/llvm-toolchain.gpg > /dev/null
echo "deb [signed-by=/usr/share/keyrings/llvm-toolchain.gpg] http://apt.llvm.org/$(lsb_release -cs)/ llvm-toolchain-$(lsb_release -cs)-18 main" | sudo tee /etc/apt/sources.list.d/llvm.list
sudo apt-get update -y
sudo apt-get install clang-format-18 -y

How to run clang-format-check action locally

1. get act
2. commit the changes locally git commit -a -s
3. under valkey repo root directory, run
   pingxie@penguin ~/valkey (clang-format)>sudo act -j clang-format-check
4. here is a failure output

[Clang Format Check/clang-format-check]   ✅  Success - Main Set up Clang
[Clang Format Check/clang-format-check] ⭐ Run Main Run clang-format
[Clang Format Check/clang-format-check]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/clang-format.sh] user= workdir=
[Clang Format Check/clang-format-check]   ✅  Success - Main Run clang-format
[Clang Format Check/clang-format-check]   ⚙  ::set-output:: diff=ZGlmZiAtLWdpdCBhL3NyYy91dGlsLmMgYi9zcmMvdXRpbC5jCmluZGV4IDhiZDlhMjg2Yy4uMmVhOWZlMzg0IDEwMDY0NAotLS0gYS9zcmMvdXRpbC5jCisrKyBiL3NyYy91dGlsLmMKQEAgLTExMCw3ICsxMTAsMTEgQEAgc3RhdGljIGludCBzdHJpbmdtYXRjaGxlbl9pbXBsKGNvbnN0IGNoYXIgKnBhdHRlcm4sCiAgICAgICAgICAgICAgICAgICAgIGlmIChwYXR0ZXJuWzBdID09IHN0cmluZ1swXSkgbWF0Y2ggPSAxOwogICAgICAgICAgICAgICAgIH0gZWxzZSBpZiAocGF0dGVyblswXSA9PSAnXScpIHsKICAgICAgICAgICAgICAgICAgICAgYnJlYWs7Ci0gICAgICAgICAgICAgICAgfSBlbHNlIGlmIChwYXR0ZXJuTGVuID09IDApIHsgcGF0dGVybi0tOyBwYXR0ZXJuTGVuKys7IGJyZWFrOyB9IGVsc2UgaWYgKHBhdHRlcm5MZW4gPj0gMyAmJiBwYXR0ZXJuWzFdID09ICctJykgeworICAgICAgICAgICAgICAgIH0gZWxzZSBpZiAocGF0dGVybkxlbiA9PSAwKSB7CisgICAgICAgICAgICAgICAgICAgIHBhdHRlcm4tLTsKKyAgICAgICAgICAgICAgICAgICAgcGF0dGVybkxlbisrOworICAgICAgICAgICAgICAgICAgICBicmVhazsKKyAgICAgICAgICAgICAgICB9IGVsc2UgaWYgKHBhdHRlcm5MZW4gPj0gMyAmJiBwYXR0ZXJuWzFdID09ICctJykgewogICAgICAgICAgICAgICAgICAgICBpbnQgc3RhcnQgPSBwYXR0ZXJuWzBdOwogICAgICAgICAgICAgICAgICAgICBpbnQgZW5kID0gcGF0dGVyblsyXTsKICAgICAgICAgICAgICAgICAgICAgaW50IGMgPSBzdHJpbmdbMF07Cg==
[Clang Format Check/clang-format-check] ⭐ Run Main Check for formatting changes
[Clang Format Check/clang-format-check]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/3.sh] user= workdir=
| Code is not formatted correctly. Here is the diff:
| diff --git a/src/util.c b/src/util.c
| index 8bd9a286c..2ea9fe384 100644
| --- a/src/util.c
| +++ b/src/util.c
| @@ -110,7 +110,11 @@ static int stringmatchlen_impl(const char *pattern,
|                      if (pattern[0] == string[0]) match = 1;
|                  } else if (pattern[0] == ']') {
|                      break;
| -                } else if (patternLen == 0) { pattern--; patternLen++; break; } else if (patternLen >= 3 && pattern[1] == '-') {
| +                } else if (patternLen == 0) {
| +                    pattern--;
| +                    patternLen++;
| +                    break;
| +                } else if (patternLen >= 3 && pattern[1] == '-') {
|                      int start = pattern[0];
|                      int end = pattern[2];
|                      int c = string[0];
[Clang Format Check/clang-format-check]   ❌  Failure - Main Check for formatting changes
[Clang Format Check/clang-format-check] exitcode '1': failure
[Clang Format Check/clang-format-check] 🏁  Job failed

5. here is a successful run

[Clang Format Check/clang-format-check]   ✅  Success - Main Set up Clang
[Clang Format Check/clang-format-check] ⭐ Run Main Run clang-format
[Clang Format Check/clang-format-check]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /var/run/act/workflow/clang-format.sh] user= workdir=
[Clang Format Check/clang-format-check]   ✅  Success - Main Run clang-format
[Clang Format Check/clang-format-check] Cleaning up container for job clang-format-check
[Clang Format Check/clang-format-check] 🏁  Job succeeded

Nice!

jidicula/clang-format-action doesn't print out the diff (to my knowledge). This plus 1) above makes it harder to figure out the formatting errors.

I don't think the action can do that and it seems helpful. It only logs the warnings and gives a list of files in the job summary.

We could also add a make target like make format that just formats the code appropriately, which can be run if CI fails or before committing.

We could also add a make target like make format that just formats the code appropriately, which can be run if CI fails or before committing.

That is a good idea. Let me merge this one first and work on improving the "developer" experience in a separate PR. need to do some research first.