From 7fe553362bc9cbff1ca17647fa36c0e300018a41 Mon Sep 17 00:00:00 2001 From: Pavel Tikhomirov Date: Tue, 29 Aug 2023 11:09:59 +0800 Subject: [PATCH 1/3] CONTRIBUTING.md: don't mention ctags Ctags is mentioned in the beginning of the "Edit the source code" which is really confusing: Do you need ctags to edit CRIU code? - No. It is just one helpful tool to browse the code, and we do not want to enforce it. So, what is it doing in contribution guide? People who really need it should be able to find it in Makefile or just write oneliner of their own to collect tags... Signed-off-by: Pavel Tikhomirov --- CONTRIBUTING.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 87da08b343..3cd74128ed 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,12 +46,6 @@ This should create the `./criu/criu` executable. ## Edit the source code -If you use ctags, you can generate the ctags file by running - -``` - make tags -``` - When you change the source code, please keep in mind the following code conventions: * we prefer tabs and indentations to be 8 characters width From 1c28b2db02107ffbfd6ae2d524af89e925f6f291 Mon Sep 17 00:00:00 2001 From: Pavel Tikhomirov Date: Tue, 29 Aug 2023 12:10:59 +0800 Subject: [PATCH 2/3] CONTRIBUTING.md: improve coding-style related sections This is highlight that code readability is the real goal of all the coding-style rules. We should not do coding-style just for coding-style, e.g. when clang-format suggests crazy formating we should not follow it if we feel it is bad. Signed-off-by: Pavel Tikhomirov --- CONTRIBUTING.md | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3cd74128ed..a70506bfbf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,11 +48,16 @@ This should create the `./criu/criu` executable. When you change the source code, please keep in mind the following code conventions: +* code is written to be read, so the code readability is the most important thing you need to have in mind when preparing patches * we prefer tabs and indentations to be 8 characters width -* CRIU mostly follows [Linux kernel coding style](https://www.kernel.org/doc/Documentation/process/coding-style.rst), but we are less strict than the kernel community. +* we prefer line length of 80 characters or less, more is allowed if it helps with code readability +* CRIU mostly follows [Linux kernel coding style](https://www.kernel.org/doc/Documentation/process/coding-style.rst), but we are less strict than the kernel community -Other conventions can be learned from the source code itself. In short, make sure your new code -looks similar to what is already there. +Other conventions can be learned from the source code itself. In short, make sure your new code looks similar to what is already there. + +## Automatic tools to fix coding-style + +Important: These tools are there to advise you, but should not be considered as a "source of truth", as tools also make nasty mistakes from time to time which can completely break code readability. The following command can be used to automatically run a code linter for Python files (flake8), Shell scripts (shellcheck), text spelling (codespell), and a number of CRIU-specific checks (usage of print macros and EOL whitespace for C files). @@ -84,6 +89,41 @@ to check the last *N* commits for formatting errors, without applying the change Note that for pull requests, the "Run code linter" workflow runs these checks for all commits. If a clang-format error is detected we need to review the suggested changes and decide if they should be fixed before merging. +Here are some bad examples of clang-format-ing: + +* if clang-format tries to force 120 characters and breaks readability - it is wrong: + +``` +@@ -58,8 +59,7 @@ static int register_membarriers(void) + } + + if (!all_ok) { +- fail("can't register membarrier()s - tried %#x, kernel %#x", +- barriers_registered, barriers_supported); ++ fail("can't register membarrier()s - tried %#x, kernel %#x", barriers_registered, barriers_supported); + return -1; + } +``` + +* if clang-format breaks your beautiful readability friendly alignment in structures, comments or defines - it is wrong: + +``` +--- a/test/zdtm/static/membarrier.c ++++ b/test/zdtm/static/membarrier.c +@@ -27,9 +27,10 @@ static const struct { + int register_cmd; + int execute_cmd; + } membarrier_cmds[] = { +- { "", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, MEMBARRIER_CMD_PRIVATE_EXPEDITED }, +- { "_SYNC_CORE", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE }, +- { "_RSEQ", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ, MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ }, ++ { "", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, MEMBARRIER_CMD_PRIVATE_EXPEDITED }, ++ { "_SYNC_CORE", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, ++ MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE }, ++ { "_RSEQ", MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_RSEQ, MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ }, + }; +``` + ## Test your changes CRIU comes with an extensive test suite. To check whether your changes introduce any regressions, run From 99d878bd868bdc1ae3297101fd9d5a4654490479 Mon Sep 17 00:00:00 2001 From: Pavel Tikhomirov Date: Thu, 24 Aug 2023 13:34:05 +0800 Subject: [PATCH 3/3] lint: don't fail workflow on indent fail There are multiple cases where good human readable code block is converted to an unreadable mess by clang-format, so we don't want to rely on clang-format completely. Also there is no way, as far as I can see, to make clang-format only fix what we want it to fix without breaking something. So let's just display hints inline where clang-format is unhappy. When reviewer sees such a warning it's a good sign that something is broken in coding-style around this warning. We add special script which parses diff generated by indent and generates warning for each hunk. Signed-off-by: Pavel Tikhomirov --- .github/workflows/lint.yml | 18 ++++++++--------- Makefile | 1 + scripts/github-indent-warnings.py | 33 +++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 9 deletions(-) create mode 100755 scripts/github-indent-warnings.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e18f921f3e..f52bce8123 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -26,15 +26,15 @@ jobs: run: make lint - name: Run make indent - run: > + continue-on-error: true + run: | if [ -z "${{github.base_ref}}" ]; then - git fetch --deepen=1 && - if ! make indent OPTS=--diff; then - exit 1 - fi + git fetch --deepen=1 + make indent else - git fetch origin ${{github.base_ref}} && - if ! make indent OPTS=--diff BASE=origin/${{github.base_ref}}; then - exit 1 - fi + git fetch origin ${{github.base_ref}} + make indent BASE=origin/${{github.base_ref}} fi + - name: Raise in-line make indent warnings + run: | + git diff | ./scripts/github-indent-warnings.py diff --git a/Makefile b/Makefile index 9a297d2d83..ca71ddd6e2 100644 --- a/Makefile +++ b/Makefile @@ -442,6 +442,7 @@ lint: flake8 --config=scripts/flake8.cfg crit/setup.py flake8 --config=scripts/flake8.cfg scripts/uninstall_module.py flake8 --config=scripts/flake8.cfg coredump/ coredump/coredump + flake8 --config=scripts/flake8.cfg scripts/github-indent-warnings.py shellcheck --version shellcheck scripts/*.sh shellcheck scripts/ci/*.sh scripts/ci/apt-install diff --git a/scripts/github-indent-warnings.py b/scripts/github-indent-warnings.py new file mode 100755 index 0000000000..04f82d6c11 --- /dev/null +++ b/scripts/github-indent-warnings.py @@ -0,0 +1,33 @@ +#!/usr/bin/python3 +import sys +import re + +re_file = r'^diff --git a/(\S\S*)\s.*$' +re_line = r'^@@ -(\d\d*)\D.*@@.*$' + +if __name__ == '__main__': + if len(sys.argv) != 1 and len(sys.argv) != 2: + print(f'usage: {sys.argv[0]} ') + print(f'usage: | {sys.argv[0]}') + exit(1) + + input_file = sys.stdin.fileno() + if len(sys.argv) == 2: + input_file = sys.argv[1] + + with open(input_file, 'r') as fi: + file_name = None + line_number = None + for line in fi: + file_matches = re.findall(re_file, line) + if len(file_matches) == 1: + file_name = file_matches[0] + continue + + if file_name is None: + continue + + line_matches = re.findall(re_line, line) + if len(line_matches) == 1: + line_number = int(line_matches[0]) + 3 + print(f'::warning file={file_name},line={line_number}::clang-format: Possible coding style problem (https://github.com/checkpoint-restore/criu/blob/criu-dev/CONTRIBUTING.md#automatic-tools-to-fix-coding-style)')