Skip to content

Document the clang-format flow in CONTRIBUTING.md #659

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

Open
SoulPancake wants to merge 3 commits into
base: unstable
Choose a base branch
from
Open

Document the clang-format flow in CONTRIBUTING.md #659

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a9dff4
feat: document the clang-format flow
SoulPancake Jun 17, 2024
f90e4ca
feat: address suggestion
SoulPancake Jun 18, 2024
855b2b9
Update CONTRIBUTING.md
SoulPancake Jun 18, 2024
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
68 changes: 68 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,74 @@ The Valkey project is led by a Technical Steering Committee, whose responsibilit
* Want to help with documentation? [Move on to valkey-doc](https://github.com/valkey-io/valkey-doc)
* Report a vulnerability? See [SECURITY.md](SECURITY.md)

## How to Install clang-format 18 on a Debian Box
Copy link
Copy Markdown
Member

@madolson madolson Jun 18, 2024

Choose a reason for hiding this comment

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

I don't really want to maintain an installation guide of clang-format for debian in this repository. Is there some better place we can point to? Do we need at least clang-format version 18 for this to run correctly?

Copy link
Copy Markdown
Member

@PingXie PingXie Jun 18, 2024

Choose a reason for hiding this comment

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

yes 18 is the most compatible one. I think we can go higher but the installation becomes challenging.

What would you recommend for this documentation? Would it work we clarify this being debian specific (so we can extend it in the future to other platforms)?

Copy link
Copy Markdown
Contributor

@hwware hwware Jun 19, 2024

Choose a reason for hiding this comment

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

I install clang-format version 14 on Ubuntu 22, it works fine. But I have no chance to older version, So we can say:

First run git clang-format, If you still meet an error, please upgrade your clang-format to latest version.

Copy link
Copy Markdown
Member

@madolson madolson Jun 18, 2024

Choose a reason for hiding this comment

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

It would also be good to put this under a code format section, instead of a section for installing clang format. It would be nice to include an example for how to run it.


```sh
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`](https://github.com/nektos/act).
Copy link
Copy Markdown
Member

@madolson madolson Jun 18, 2024

Choose a reason for hiding this comment

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

Suggested change
1. Get [`act`](https://github.com/nektos/act).
1. Install [`act`](https://nektosact.com/installation/index.html).

2. Commit the changes locally:
```sh
git commit -a -s
```
Comment on lines +37 to +40
Copy link
Copy Markdown
Member

@madolson madolson Jun 18, 2024

Choose a reason for hiding this comment

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

Suggested change
2. Commit the changes locally:
```sh
git commit -a -s
```

Instead, I think you should indicate it runs on what is current committed locally.

3. Under the repository root directory, run:
```sh
sudo act -j clang-format-check
```

### Here is a Failure Output
Copy link
Copy Markdown
Member

@madolson madolson Jun 18, 2024

Choose a reason for hiding this comment

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

I think these two sections are overkill. The most useful part is the git diff part, so maybe just keep that? If there is no diff, then it will pass.


```sh
[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
```

### Here is a Successful Run

```sh
[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
```

Copy link
Copy Markdown
Member

@PingXie PingXie Jun 18, 2024

Choose a reason for hiding this comment

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

We should probably keep the DCO at the top as this is the most important information IMO for any new contributor. Also while you are at it, can you add some git commands to show how to add the DCO? I noticed that the commands were not obvious to some folks in the past.

## Developer Certificate of Origin

We respect the intellectual property rights of others and we want to make sure
Expand Down