Skip to content
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.

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

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion .github/workflows/linting.yml
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ jobs:
- doxygen
- pylint
- codespell
# - clang-format # not required
- clang-format
- yaml-lint
- copyright
if: (!cancelled())
Expand Down
88 changes: 46 additions & 42 deletions utils/githooks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,68 +8,78 @@ The DAOS repo contains several built-in githooks that are intended
to help developers conform to DAOS community coding standards and practices
and to avoid some common mistakes in the development cycle.

To enable these standard githooks requires a two step process:
## Install DAOS Git Hooks
Installing is a two-step process:
grom72 marked this conversation as resolved.
Show resolved Hide resolved

1. Install the hooks

Configure your core.hookspath as follows (Recommended):
### 1. Install the hooks

Recommended: Configure your `core.hookspath`.
Any new githooks added to the repository will automatically run,
but possibly require additional software to produce the desired effect.
Additionally, as the branch changes, the githooks change with it.
```sh
git config core.hookspath utils/githooks
```

Additionally, one can copy the files into an already configured path.

With the first option, any new githooks added to the repository will
automatically run, but possibly require additional software to produce the
desired effect. Additionally, as the branch changes, the githooks
change with it.

2. Install all of the required tools
### 2. Install the required tools
grom72 marked this conversation as resolved.
Show resolved Hide resolved

The Githooks framework in DAOS is such that the hooks will all run.
However, some hooks will simply check for required software and are
effectively a noop if such is not installed.

On many systems, the required packages can be installed through standard means
but customization may be required. Some are specified in
[requirements.txt](../../requirements.txt) so can be installed using
`pip install -r requirements.txt` and `pip install -r utils/cq/requirements.txt`
which can also be done using a virtual environment. The following
packages are used by built-in githooks.
Requirements come from a combination of `pip` and system packages and can usually be installed through standard means.
To install `pip` packages specified in [utils/cq/requirements.txt](../../utils/cq/requirements.txt):
grom72 marked this conversation as resolved.
Show resolved Hide resolved
```sh
python3 -m pip install -r utils/cq/requirements.txt
```
To install system packages with your package manager - for example:
grom72 marked this conversation as resolved.
Show resolved Hide resolved
```sh
dnf install git-clang-format
grom72 marked this conversation as resolved.
Show resolved Hide resolved
```
### Installed tools
grom72 marked this conversation as resolved.
Show resolved Hide resolved
The following packages are used by built-in githooks.

1. clang-format version 14.0.5 or higher. If the check is unable to parse
the version output, it will fail. Try running
1. `clang-format` version 14.0.5 or higher - Formatter for C code. If the check is unable to parse
grom72 marked this conversation as resolved.
Show resolved Hide resolved
the version output, it will fail. Try running
`<root>/site_scons/site_tools/extra/extra.py` to check.
2. pylint
3. flake8
4. yamllint
5. gofmt

There is a daos wrapper around pylint at `utils/cq/daos_pylint.py` which will perform standard
pylint checks whilst managing scons and PYTHONPATH setup changes automatically. Installing
the python packages in `utils/cq/requirements.txt` will allow it to test for all the python dependencies.
2. `pylint`
3. `flake8`
4. `isort`
5. `yamllint`
6. `gofmt`
7. `codespell`

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
grom72 marked this conversation as resolved.
Show resolved Hide resolved

Additionally, [find_base.sh](find_base.sh) attempts to determine the base
branch using `gh`, the Github CLI. If this isn't installed, it will use
`master` as the base which can result in a larger diff and more files being
`master` or a `release` branch as the base, which can result in a larger diff and more files being
checked than expected.

## Checks performed by built-in scripts

### pre-commit

1. clang-format will update any C/C++ files changed using configuration in
[.clang-format](../../.clang-format). If anything changed, it will exit,
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.
grom72 marked this conversation as resolved.
Show resolved Hide resolved
2. `codespell` - Linter for spelling mistakes in modified files
- See [codespell.ignores](../../ci/codespell.ignores) for ignored words.
- See [words.dict](../../utils/cq/words.dict) for words added to the DAOS project.
3. Jenkinsfile - Custom linter if the Jenkinsfile is modified
4. `yamllint` - Linter for modified YAML configs
5. `clang-format` - Automatically formats for C/C++ files modified. If anything changed it will exit,
allowing the user to inspect the changes and retry the commit.
2. pylint will check python changes and fail if there are errors.
3. flake8 will check python changes and fail if there are errors.
4. yamllint will check YAML file changes and fail if there are errors.
5. gofmt will check Go files changed and fail if there are errors.
6. Copyrights will be checked and updated if needed.
- See [.clang-format](../../.clang-format) for configuration
6. `gofmt` - Automatically formats for modified GO files
7. `isort` - Linter for python imports on modified python files
8. `flake8` - Linter for python files
9. `pylint` - Additional linter for modified python files
- See [daos_pylint.py](../../utils/cq/daos_pylint.py) for a custom wrapper around `pylint` which manages `PYTHONPATH` setup internally.
10. ftest - Custom linter for modified ftest files

### prepare-commit-msg

Expand All @@ -79,12 +89,6 @@ the comment can be ignored. If not, it's likely someone else changed
it and an update is needed before commit. In such a case, abort the
commit by exiting without saving.

### commit-msg

1. Checks to see if githooks are installed locally and adds a watermark
to the commit message that is checked in CI. It is not fatal but
gatekeepers may ask that githooks be used.

## Adding user specific hooks

The framework is extensible. In order to add a custom user hook, a developer
Expand Down
5 changes: 3 additions & 2 deletions utils/githooks/pre-commit.d/50-clang-format.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/bin/bash
#
# Copyright 2022-2024 Intel Corporation.
# Copyright 2025 Hewlett Packard Enterprise Development LP
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
Expand All @@ -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
grom72 marked this conversation as resolved.
Show resolved Hide resolved
fi
if ! command -v clang-format > /dev/null 2>&1; then
grom72 marked this conversation as resolved.
Show resolved Hide resolved
echo "clang-format not installed. Skipping"
echo "clang-format not installed. See ./utils/githooks/README.md for instructions"
grom72 marked this conversation as resolved.
Show resolved Hide resolved
exit 0
grom72 marked this conversation as resolved.
Show resolved Hide resolved
fi

Expand Down
Loading