diff options
Diffstat (limited to 'test/lint')
| -rw-r--r-- | test/lint/README.md | 14 | ||||
| -rwxr-xr-x | test/lint/commit-script-check.sh | 5 | ||||
| -rwxr-xr-x | test/lint/lint-git-commit-check.py | 23 | ||||
| -rwxr-xr-x | test/lint/lint-include-guards.py | 8 | ||||
| -rwxr-xr-x | test/lint/lint-includes.py | 9 | ||||
| -rwxr-xr-x | test/lint/lint-spelling.py | 5 | ||||
| -rwxr-xr-x | test/lint/lint-whitespace.py | 136 | ||||
| -rw-r--r-- | test/lint/lint_ignore_dirs.py | 5 | ||||
| -rw-r--r-- | test/lint/test_runner/src/main.rs | 213 |
9 files changed, 239 insertions, 179 deletions
diff --git a/test/lint/README.md b/test/lint/README.md index 1fba41d9ea..9d167bac72 100644 --- a/test/lint/README.md +++ b/test/lint/README.md @@ -16,10 +16,14 @@ result is cached and it prevents issues when the image changes. test runner =========== -To run all the lint checks in the test runner outside the docker, use: +To run all the lint checks in the test runner outside the docker you first need +to install the rust toolchain using your package manager of choice or +[rustup](https://www.rust-lang.org/tools/install). + +Then you can use: ```sh -( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run ) +( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run ) ``` #### Dependencies @@ -34,7 +38,7 @@ To run all the lint checks in the test runner outside the docker, use: | [`lint-shell.py`](lint/lint-shell.py) | [ShellCheck](https://github.com/koalaman/shellcheck) | [`lint-spelling.py`](lint/lint-spelling.py) | [codespell](https://github.com/codespell-project/codespell) -In use versions and install instructions are available in the [CI setup](../ci/lint/04_install.sh). +In use versions and install instructions are available in the [CI setup](../../ci/lint/04_install.sh). Please be aware that on Linux distributions all dependencies are usually available as packages, but could be outdated. @@ -83,3 +87,7 @@ To do so, add the upstream repository as remote: ``` git remote add --fetch secp256k1 https://github.com/bitcoin-core/secp256k1.git ``` + +lint_ignore_dirs.py +=================== +Add list of common directories to ignore when running tests diff --git a/test/lint/commit-script-check.sh b/test/lint/commit-script-check.sh index 55c9528dea..fe845ed19e 100755 --- a/test/lint/commit-script-check.sh +++ b/test/lint/commit-script-check.sh @@ -22,6 +22,11 @@ if ! sed --help 2>&1 | grep -q 'GNU'; then exit 1; fi +if ! grep --help 2>&1 | grep -q 'GNU'; then + echo "Error: the installed grep package is not compatible. Please make sure you have GNU grep installed in your system."; + exit 1; +fi + RET=0 PREV_BRANCH=$(git name-rev --name-only HEAD) PREV_HEAD=$(git rev-parse HEAD) diff --git a/test/lint/lint-git-commit-check.py b/test/lint/lint-git-commit-check.py index 5897a17e70..5dc30cc755 100755 --- a/test/lint/lint-git-commit-check.py +++ b/test/lint/lint-git-commit-check.py @@ -23,31 +23,18 @@ def parse_args(): """, epilog=f""" You can manually set the commit-range with the COMMIT_RANGE - environment variable (e.g. "COMMIT_RANGE='47ba2c3...ee50c9e' - {sys.argv[0]}"). Defaults to current merge base when neither - prev-commits nor the environment variable is set. + environment variable (e.g. "COMMIT_RANGE='HEAD~n..HEAD' + {sys.argv[0]}") for the last 'n' commits. """) - - parser.add_argument("--prev-commits", "-p", required=False, help="The previous n commits to check") - return parser.parse_args() def main(): - args = parse_args() + parse_args() exit_code = 0 - if not os.getenv("COMMIT_RANGE"): - if args.prev_commits: - commit_range = "HEAD~" + args.prev_commits + "...HEAD" - else: - # This assumes that the target branch of the pull request will be master. - merge_base = check_output(["git", "merge-base", "HEAD", "master"], text=True, encoding="utf8").rstrip("\n") - commit_range = merge_base + "..HEAD" - else: - commit_range = os.getenv("COMMIT_RANGE") - if commit_range == "SKIP_EMPTY_NOT_A_PR": - sys.exit(0) + assert os.getenv("COMMIT_RANGE") # E.g. COMMIT_RANGE='HEAD~n..HEAD' + commit_range = os.getenv("COMMIT_RANGE") commit_hashes = check_output(["git", "log", commit_range, "--format=%H"], text=True, encoding="utf8").splitlines() diff --git a/test/lint/lint-include-guards.py b/test/lint/lint-include-guards.py index 291e528c1d..77af05c1c2 100755 --- a/test/lint/lint-include-guards.py +++ b/test/lint/lint-include-guards.py @@ -12,19 +12,17 @@ import re import sys from subprocess import check_output +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES + HEADER_ID_PREFIX = 'BITCOIN_' HEADER_ID_SUFFIX = '_H' EXCLUDE_FILES_WITH_PREFIX = ['contrib/devtools/bitcoin-tidy', 'src/crypto/ctaes', - 'src/leveldb', - 'src/crc32c', - 'src/secp256k1', - 'src/minisketch', 'src/tinyformat.h', 'src/bench/nanobench.h', - 'src/test/fuzz/FuzzedDataProvider.h'] + 'src/test/fuzz/FuzzedDataProvider.h'] + SHARED_EXCLUDED_SUBTREES def _get_header_file_lst() -> list[str]: diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index 6386a92c0f..90884299d5 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -14,13 +14,11 @@ import sys from subprocess import check_output, CalledProcessError +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES + EXCLUDED_DIRS = ["contrib/devtools/bitcoin-tidy/", - "src/leveldb/", - "src/crc32c/", - "src/secp256k1/", - "src/minisketch/", - ] + ] + SHARED_EXCLUDED_SUBTREES EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp", "boost/multi_index/detail/hash_index_iterator.hpp", @@ -32,7 +30,6 @@ EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp", "boost/multi_index/tag.hpp", "boost/multi_index_container.hpp", "boost/operators.hpp", - "boost/process.hpp", "boost/signals2/connection.hpp", "boost/signals2/optional_last_value.hpp", "boost/signals2/signal.hpp", diff --git a/test/lint/lint-spelling.py b/test/lint/lint-spelling.py index ac0bddeaa6..3e578b218f 100755 --- a/test/lint/lint-spelling.py +++ b/test/lint/lint-spelling.py @@ -11,8 +11,11 @@ Note: Will exit successfully regardless of spelling errors. from subprocess import check_output, STDOUT, CalledProcessError +from lint_ignore_dirs import SHARED_EXCLUDED_SUBTREES + IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt' -FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/guix/patches"] +FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/guix/patches"] +FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES] def check_codespell_install(): diff --git a/test/lint/lint-whitespace.py b/test/lint/lint-whitespace.py deleted file mode 100755 index f5e4a776d0..0000000000 --- a/test/lint/lint-whitespace.py +++ /dev/null @@ -1,136 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2017-2022 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Check for new lines in diff that introduce trailing whitespace or -# tab characters instead of spaces. - -# We can't run this check unless we know the commit range for the PR. - -import argparse -import os -import re -import sys - -from subprocess import check_output - -EXCLUDED_DIRS = ["depends/patches/", - "contrib/guix/patches/", - "src/leveldb/", - "src/crc32c/", - "src/secp256k1/", - "src/minisketch/", - "doc/release-notes/", - "src/qt/locale"] - -def parse_args(): - """Parse command line arguments.""" - parser = argparse.ArgumentParser( - description=""" - Check for new lines in diff that introduce trailing whitespace - or tab characters instead of spaces in unstaged changes, the - previous n commits, or a commit-range. - """, - epilog=f""" - You can manually set the commit-range with the COMMIT_RANGE - environment variable (e.g. "COMMIT_RANGE='47ba2c3...ee50c9e' - {sys.argv[0]}"). Defaults to current merge base when neither - prev-commits nor the environment variable is set. - """) - - parser.add_argument("--prev-commits", "-p", required=False, help="The previous n commits to check") - - return parser.parse_args() - - -def report_diff(selection): - filename = "" - seen = False - seenln = False - - print("The following changes were suspected:") - - for line in selection: - if re.match(r"^diff", line): - filename = line - seen = False - elif re.match(r"^@@", line): - linenumber = line - seenln = False - else: - if not seen: - # The first time a file is seen with trailing whitespace or a tab character, we print the - # filename (preceded by a newline). - print("") - print(filename) - seen = True - if not seenln: - print(linenumber) - seenln = True - print(line) - - -def get_diff(commit_range, check_only_code): - exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS] - - if check_only_code: - what_files = ["*.cpp", "*.h", "*.md", "*.py", "*.sh"] - else: - what_files = ["."] - - diff = check_output(["git", "diff", "-U0", commit_range, "--"] + what_files + exclude_args, text=True, encoding="utf8") - - return diff - - -def main(): - args = parse_args() - - if not os.getenv("COMMIT_RANGE"): - if args.prev_commits: - commit_range = "HEAD~" + args.prev_commits + "...HEAD" - else: - # This assumes that the target branch of the pull request will be master. - merge_base = check_output(["git", "merge-base", "HEAD", "master"], text=True, encoding="utf8").rstrip("\n") - commit_range = merge_base + "..HEAD" - else: - commit_range = os.getenv("COMMIT_RANGE") - if commit_range == "SKIP_EMPTY_NOT_A_PR": - sys.exit(0) - - whitespace_selection = [] - tab_selection = [] - - # Check if trailing whitespace was found in the diff. - for line in get_diff(commit_range, check_only_code=False).splitlines(): - if re.match(r"^(diff --git|\@@|^\+.*\s+$)", line): - whitespace_selection.append(line) - - whitespace_additions = [i for i in whitespace_selection if i.startswith("+")] - - # Check if tab characters were found in the diff. - for line in get_diff(commit_range, check_only_code=True).splitlines(): - if re.match(r"^(diff --git|\@@|^\+.*\t)", line): - tab_selection.append(line) - - tab_additions = [i for i in tab_selection if i.startswith("+")] - - ret = 0 - - if len(whitespace_additions) > 0: - print("This diff appears to have added new lines with trailing whitespace.") - report_diff(whitespace_selection) - ret = 1 - - if len(tab_additions) > 0: - print("This diff appears to have added new lines with tab characters instead of spaces.") - report_diff(tab_selection) - ret = 1 - - sys.exit(ret) - - -if __name__ == "__main__": - main() diff --git a/test/lint/lint_ignore_dirs.py b/test/lint/lint_ignore_dirs.py new file mode 100644 index 0000000000..af9ee7ef6b --- /dev/null +++ b/test/lint/lint_ignore_dirs.py @@ -0,0 +1,5 @@ +SHARED_EXCLUDED_SUBTREES = ["src/leveldb/", + "src/crc32c/", + "src/secp256k1/", + "src/minisketch/", + ] diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 1dc79e97bd..e22e047e4b 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -4,7 +4,7 @@ use std::env; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use std::process::ExitCode; @@ -14,7 +14,9 @@ type LintFn = fn() -> LintResult; /// Return the git command fn git() -> Command { - Command::new("git") + let mut git = Command::new("git"); + git.arg("--no-pager"); + git } /// Return stdout @@ -34,17 +36,30 @@ fn get_git_root() -> PathBuf { PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap()) } +/// Return all subtree paths +fn get_subtrees() -> Vec<&'static str> { + vec![ + "src/crc32c", + "src/crypto/ctaes", + "src/leveldb", + "src/minisketch", + "src/secp256k1", + ] +} + +/// Return the pathspecs to exclude all subtrees +fn get_pathspecs_exclude_subtrees() -> Vec<String> { + get_subtrees() + .iter() + .map(|s| format!(":(exclude){}", s)) + .collect() +} + fn lint_subtree() -> LintResult { // This only checks that the trees are pure subtrees, it is not doing a full // check with -r to not have to fetch all the remotes. let mut good = true; - for subtree in [ - "src/crypto/ctaes", - "src/secp256k1", - "src/minisketch", - "src/leveldb", - "src/crc32c", - ] { + for subtree in get_subtrees() { good &= Command::new("test/lint/git-subtree-check.sh") .arg(subtree) .status() @@ -82,6 +97,181 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted. } } +/// Return the pathspecs for whitespace related excludes +fn get_pathspecs_exclude_whitespace() -> Vec<String> { + let mut list = get_pathspecs_exclude_subtrees(); + list.extend( + [ + // Permanent excludes + "*.patch", + "src/qt/locale", + "contrib/windeploy/win-codesign.cert", + "doc/README_windows.txt", + // Temporary excludes, or existing violations + "doc/release-notes/release-notes-0.*", + "contrib/init/bitcoind.openrc", + "contrib/macdeploy/macdeployqtplus", + "src/crypto/sha256_sse4.cpp", + "src/qt/res/src/*.svg", + "test/functional/test_framework/crypto/ellswift_decode_test_vectors.csv", + "test/functional/test_framework/crypto/xswiftec_inv_test_vectors.csv", + "contrib/qos/tc.sh", + "contrib/verify-commits/gpg.sh", + "src/univalue/include/univalue_escapes.h", + "src/univalue/test/object.cpp", + "test/lint/git-subtree-check.sh", + ] + .iter() + .map(|s| format!(":(exclude){}", s)), + ); + list +} + +fn lint_trailing_whitespace() -> LintResult { + let trailing_space = git() + .args(["grep", "-I", "--line-number", "\\s$", "--"]) + .args(get_pathspecs_exclude_whitespace()) + .status() + .expect("command error") + .success(); + if trailing_space { + Err(r#" +^^^ +Trailing whitespace is problematic, because git may warn about it, or editors may remove it by +default, forcing developers in the future to either undo the changes manually or spend time on +review. + +Thus, it is best to remove the trailing space now. + +Please add any false positives, such as subtrees, Windows-related files, patch files, or externally +sourced files to the exclude list. + "# + .to_string()) + } else { + Ok(()) + } +} + +fn lint_tabs_whitespace() -> LintResult { + let tabs = git() + .args(["grep", "-I", "--line-number", "--perl-regexp", "^\\t", "--"]) + .args(["*.cpp", "*.h", "*.md", "*.py", "*.sh"]) + .args(get_pathspecs_exclude_whitespace()) + .status() + .expect("command error") + .success(); + if tabs { + Err(r#" +^^^ +Use of tabs in this codebase is problematic, because existing code uses spaces and tabs will cause +display issues and conflict with editor settings. + +Please remove the tabs. + +Please add any false positives, such as subtrees, or externally sourced files to the exclude list. + "# + .to_string()) + } else { + Ok(()) + } +} + +fn lint_includes_build_config() -> LintResult { + let config_path = "./src/config/bitcoin-config.h.in"; + let include_directive = "#include <config/bitcoin-config.h>"; + if !Path::new(config_path).is_file() { + assert!(Command::new("./autogen.sh") + .status() + .expect("command error") + .success()); + } + let defines_regex = format!( + r"^\s*(?!//).*({})", + check_output(Command::new("grep").args(["undef ", "--", config_path])) + .expect("grep failed") + .lines() + .map(|line| { + line.split("undef ") + .nth(1) + .unwrap_or_else(|| panic!("Could not extract name in line: {line}")) + }) + .collect::<Vec<_>>() + .join("|") + ); + let print_affected_files = |mode: bool| { + // * mode==true: Print files which use the define, but lack the include + // * mode==false: Print files which lack the define, but use the include + let defines_files = check_output( + git() + .args([ + "grep", + "--perl-regexp", + if mode { + "--files-with-matches" + } else { + "--files-without-match" + }, + &defines_regex, + "--", + "*.cpp", + "*.h", + ]) + .args(get_pathspecs_exclude_subtrees()) + .args([ + // These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds + // these cppflags manually. + ":(exclude)src/crypto/sha256_arm_shani.cpp", + ":(exclude)src/crypto/sha256_avx2.cpp", + ":(exclude)src/crypto/sha256_sse41.cpp", + ":(exclude)src/crypto/sha256_x86_shani.cpp", + ]), + ) + .expect("grep failed"); + git() + .args([ + "grep", + if mode { + "--files-without-match" + } else { + "--files-with-matches" + }, + include_directive, + "--", + ]) + .args(defines_files.lines()) + .status() + .expect("command error") + .success() + }; + let missing = print_affected_files(true); + if missing { + return Err(format!( + r#" +^^^ +One or more files use a symbol declared in the bitcoin-config.h header. However, they are not +including the header. This is problematic, because the header may or may not be indirectly +included. If the indirect include were to be intentionally or accidentally removed, the build could +still succeed, but silently be buggy. For example, a slower fallback algorithm could be picked, +even though bitcoin-config.h indicates that a faster feature is available and should be used. + +If you are unsure which symbol is used, you can find it with this command: +git grep --perl-regexp '{}' -- file_name + "#, + defines_regex + )); + } + let redundant = print_affected_files(false); + if redundant { + return Err(r#" +^^^ +None of the files use a symbol declared in the bitcoin-config.h header. However, they are including +the header. Consider removing the unused include. + "# + .to_string()); + } + Ok(()) +} + fn lint_doc() -> LintResult { if Command::new("test/lint/check-doc.py") .status() @@ -123,6 +313,9 @@ fn main() -> ExitCode { let test_list: Vec<(&str, LintFn)> = vec![ ("subtree check", lint_subtree), ("std::filesystem check", lint_std_filesystem), + ("trailing whitespace check", lint_trailing_whitespace), + ("no-tabs check", lint_tabs_whitespace), + ("build config includes check", lint_includes_build_config), ("-help=1 documentation check", lint_doc), ("lint-*.py scripts", lint_all), ]; @@ -134,7 +327,7 @@ fn main() -> ExitCode { // chdir to root before each lint test env::set_current_dir(&git_root).unwrap(); if let Err(err) = lint_fn() { - println!("{err}\n^---- Failure generated from {lint_name}!"); + println!("{err}\n^---- ⚠️ Failure generated from {lint_name}!"); test_failed = true; } } |