diff --git a/test/lint/test_runner/src/lint_cpp.rs b/test/lint/test_runner/src/lint_cpp.rs new file mode 100644 index 00000000000..250ebb46a95 --- /dev/null +++ b/test/lint/test_runner/src/lint_cpp.rs @@ -0,0 +1,180 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::process::Command; + +use crate::util::{check_output, get_pathspecs_default_excludes, git, LintResult}; + +pub fn lint_includes_build_config() -> LintResult { + let config_path = "./cmake/bitcoin-build-config.h.in"; + let defines_regex = format!( + r"^\s*(?!//).*({})", + check_output(Command::new("grep").args(["define", "--", config_path])) + .expect("grep failed") + .lines() + .map(|line| { + line.split_whitespace() + .nth(1) + .unwrap_or_else(|| panic!("Could not extract name in line: {line}")) + }) + .collect::>() + .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_default_excludes()), + ) + .expect("grep failed"); + git() + .args([ + "grep", + if mode { + "--files-without-match" + } else { + "--files-with-matches" + }, + if mode { + "^#include // IWYU pragma: keep$" + } else { + "#include " // Catch redundant includes with and without the IWYU pragma + }, + "--", + ]) + .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-build-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-build-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 '{defines_regex}' -- file_name + +Make sure to include it with the IWYU pragma. Otherwise, IWYU may falsely instruct to remove the +include again. + +#include // IWYU pragma: keep + "# + ) + .trim() + .to_string()); + } + let redundant = print_affected_files(false); + if redundant { + return Err(r#" +None of the files use a symbol declared in the bitcoin-build-config.h header. However, they are including +the header. Consider removing the unused include. + "# + .to_string()); + } + Ok(()) +} + +pub fn lint_std_filesystem() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "std::filesystem", + "--", + "./src/", + ":(exclude)src/ipc/libmultiprocess/", + ":(exclude)src/util/fs.h", + ":(exclude)src/test/kernel/test_kernel.cpp", + ":(exclude)src/bitcoin-chainstate.cpp", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +Direct use of std::filesystem may be dangerous and buggy. Please include and use the +fs:: namespace, which has unsafe filesystem functions marked as deleted. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +pub fn lint_rpc_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"\<(A|a)ss(ume|ert)\(", + "--", + "src/rpc/", + "src/wallet/rpc*", + ":(exclude)src/rpc/server.cpp", + // src/rpc/server.cpp is excluded from this check since it's mostly meta-code. + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. + +Aborting the whole process is undesirable for RPC code. So nonfatal +checks should be used over assert. See: src/util/check.h + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +pub fn lint_boost_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"BOOST_ASSERT\(", + "--", + "*.cpp", + "*.h", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary +include of the boost/assert.hpp dependency. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} diff --git a/test/lint/test_runner/src/lint_docs.rs b/test/lint/test_runner/src/lint_docs.rs new file mode 100644 index 00000000000..7063b508447 --- /dev/null +++ b/test/lint/test_runner/src/lint_docs.rs @@ -0,0 +1,85 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::io::ErrorKind; +use std::process::{Command, Stdio}; + +use crate::util::{check_output, get_subtrees, git, LintResult}; + +pub fn lint_doc_release_note_snippets() -> LintResult { + let non_release_notes = check_output(git().args([ + "ls-files", + "--", + "doc/release-notes/", + ":(exclude)doc/release-notes/*.*.md", // Assume that at least one dot implies a proper release note + ]))?; + if non_release_notes.is_empty() { + Ok(()) + } else { + println!("{non_release_notes}"); + Err(r#" +Release note snippets and other docs must be put into the doc/ folder directly. + +The doc/release-notes/ folder is for archived release notes of previous releases only. Snippets are +expected to follow the naming "/doc/release-notes-.md". + "# + .trim() + .to_string()) + } +} + +pub fn lint_doc_args() -> LintResult { + if Command::new("test/lint/check-doc.py") + .status() + .expect("command error") + .success() + { + Ok(()) + } else { + Err("".to_string()) + } +} + +pub fn lint_markdown() -> LintResult { + let bin_name = "mlc"; + let mut md_ignore_paths = get_subtrees(); + md_ignore_paths.push("./doc/README_doxygen.md"); + let md_ignore_path_str = md_ignore_paths.join(","); + + let mut cmd = Command::new(bin_name); + cmd.args([ + "--offline", + "--ignore-path", + md_ignore_path_str.as_str(), + "--gitignore", + "--gituntracked", + "--root-dir", + ".", + ]) + .stdout(Stdio::null()); // Suppress overly-verbose output + + match cmd.output() { + Ok(output) if output.status.success() => Ok(()), + Ok(output) => { + let stderr = String::from_utf8_lossy(&output.stderr); + Err(format!( + r#" +One or more markdown links are broken. + +Note: relative links are preferred as jump-to-file works natively within Emacs, but they are not required. + +Markdown link errors found: +{stderr} + "# + ) + .trim() + .to_string()) + } + Err(e) if e.kind() == ErrorKind::NotFound => { + println!("`mlc` was not found in $PATH, skipping markdown lint check."); + Ok(()) + } + Err(e) => Err(format!("Error running mlc: {e}")), // Misc errors + } +} diff --git a/test/lint/test_runner/src/lint_py.rs b/test/lint/test_runner/src/lint_py.rs new file mode 100644 index 00000000000..d9177c86907 --- /dev/null +++ b/test/lint/test_runner/src/lint_py.rs @@ -0,0 +1,75 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::io::ErrorKind; +use std::process::Command; + +use crate::util::{check_output, get_pathspecs_default_excludes, git, LintResult}; + +pub fn lint_py_lint() -> LintResult { + let bin_name = "ruff"; + let checks = format!( + "--select={}", + [ + "B006", // mutable-argument-default + "B008", // function-call-in-default-argument + "E101", // indentation contains mixed spaces and tabs + "E401", // multiple imports on one line + "E402", // module level import not at top of file + "E701", // multiple statements on one line (colon) + "E702", // multiple statements on one line (semicolon) + "E703", // statement ends with a semicolon + "E711", // comparison to None should be 'if cond is None:' + "E713", // test for membership should be "not in" + "E714", // test for object identity should be "is not" + "E721", // do not compare types, use "isinstance()" + "E722", // do not use bare 'except' + "E742", // do not define classes named "l", "O", or "I" + "E743", // do not define functions named "l", "O", or "I" + "F401", // module imported but unused + "F402", // import module from line N shadowed by loop variable + "F403", // 'from foo_module import *' used; unable to detect undefined names + "F404", // future import(s) name after other statements + "F405", // foo_function may be undefined, or defined from star imports: bar_module + "F406", // "from module import *" only allowed at module level + "F407", // an undefined __future__ feature name was imported + "F541", // f-string without any placeholders + "F601", // dictionary key name repeated with different values + "F602", // dictionary key variable name repeated with different values + "F621", // too many expressions in an assignment with star-unpacking + "F631", // assertion test is a tuple, which are always True + "F632", // use ==/!= to compare str, bytes, and int literals + "F811", // redefinition of unused name from line N + "F821", // undefined name 'Foo' + "F822", // undefined name name in __all__ + "F823", // local variable name … referenced before assignment + "F841", // local variable 'foo' is assigned to but never used + "PLE", // Pylint errors + "W191", // indentation contains tabs + "W291", // trailing whitespace + "W292", // no newline at end of file + "W293", // blank line contains whitespace + "W605", // invalid escape sequence "x" + ] + .join(",") + ); + let files = check_output( + git() + .args(["ls-files", "--", "*.py"]) + .args(get_pathspecs_default_excludes()), + )?; + + let mut cmd = Command::new(bin_name); + cmd.args(["check", &checks]).args(files.lines()); + + match cmd.status() { + Ok(status) if status.success() => Ok(()), + Ok(_) => Err(format!("`{bin_name}` found errors!")), + Err(e) if e.kind() == ErrorKind::NotFound => { + println!("`{bin_name}` was not found in $PATH, skipping those checks."); + Ok(()) + } + Err(e) => Err(format!("Error running `{bin_name}`: {e}")), + } +} diff --git a/test/lint/test_runner/src/lint_repo_hygiene.rs b/test/lint/test_runner/src/lint_repo_hygiene.rs new file mode 100644 index 00000000000..d2fc07918da --- /dev/null +++ b/test/lint/test_runner/src/lint_repo_hygiene.rs @@ -0,0 +1,38 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::process::Command; + +use crate::util::{commit_range, get_subtrees, LintResult}; + +pub 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 get_subtrees() { + good &= Command::new("test/lint/git-subtree-check.sh") + .arg(subtree) + .status() + .expect("command_error") + .success(); + } + if good { + Ok(()) + } else { + Err("".to_string()) + } +} + +pub fn lint_scripted_diff() -> LintResult { + if Command::new("test/lint/commit-script-check.sh") + .arg(commit_range()) + .status() + .expect("command error") + .success() + { + Ok(()) + } else { + Err("".to_string()) + } +} diff --git a/test/lint/test_runner/src/lint_text_format.rs b/test/lint/test_runner/src/lint_text_format.rs new file mode 100644 index 00000000000..ca51e6db081 --- /dev/null +++ b/test/lint/test_runner/src/lint_text_format.rs @@ -0,0 +1,159 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::fs::File; +use std::io::{Read, Seek, SeekFrom}; + +use crate::util::{check_output, commit_range, get_pathspecs_default_excludes, git, LintResult}; + +/// Return the pathspecs for whitespace related excludes +fn get_pathspecs_exclude_whitespace() -> Vec { + let mut list = get_pathspecs_default_excludes(); + list.extend( + [ + // Permanent excludes + "*.patch", + "src/qt/locale", + "contrib/windeploy/win-codesign.cert", + "doc/README_windows.txt", + // Temporary excludes, or existing violations + "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 +} + +pub 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 (including Windows line endings [CR LF]) 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. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +pub fn lint_trailing_newline() -> LintResult { + let files = check_output( + git() + .args([ + "ls-files", "--", "*.py", "*.cpp", "*.h", "*.md", "*.rs", "*.sh", "*.cmake", + ]) + .args(get_pathspecs_default_excludes()), + )?; + let mut missing_newline = false; + for path in files.lines() { + let mut file = File::open(path).expect("must be able to open file"); + if file.seek(SeekFrom::End(-1)).is_err() { + continue; // Allow fully empty files + } + let mut buffer = [0u8; 1]; + file.read_exact(&mut buffer) + .expect("must be able to read the last byte"); + if buffer[0] != b'\n' { + missing_newline = true; + println!("{path}"); + } + } + if missing_newline { + Err(r#" +A trailing newline is required, because git may warn about it missing. Also, it can make diffs +verbose and can break git blame after appending lines. + +Thus, it is best to add the trailing newline now. + +Please add any false positives to the exclude list. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +pub 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. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + +pub fn lint_commit_msg() -> LintResult { + let mut good = true; + let commit_hashes = check_output(git().args([ + "-c", + "log.showSignature=false", + "log", + &commit_range(), + "--format=%H", + ]))?; + for hash in commit_hashes.lines() { + let commit_info = check_output(git().args([ + "-c", + "log.showSignature=false", + "log", + "--format=%B", + "-n", + "1", + hash, + ]))?; + if let Some(line) = commit_info.lines().nth(1) { + if !line.is_empty() { + println!( + "The subject line of commit hash {hash} is followed by a non-empty line. Subject lines should always be followed by a blank line." + ); + good = false; + } + } + } + if good { + Ok(()) + } else { + Err("".to_string()) + } +} diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 171c98072aa..fa69d2b5f9a 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -2,18 +2,27 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or https://opensource.org/license/mit/. -use std::env; -use std::fs::{self, File}; -use std::io::{ErrorKind, Read, Seek, SeekFrom}; -use std::path::PathBuf; -use std::process::{Command, ExitCode, Stdio}; +mod lint_cpp; +mod lint_docs; +mod lint_py; +mod lint_repo_hygiene; +mod lint_text_format; +mod util; -/// A possible error returned by any of the linters. -/// -/// The error string should explain the failure type and list all violations. -type LintError = String; -type LintResult = Result<(), LintError>; -type LintFn = fn() -> LintResult; +use std::env; +use std::fs; +use std::process::{Command, ExitCode}; + +use lint_cpp::{ + lint_boost_assert, lint_includes_build_config, lint_rpc_assert, lint_std_filesystem, +}; +use lint_docs::{lint_doc_args, lint_doc_release_note_snippets, lint_markdown}; +use lint_py::lint_py_lint; +use lint_repo_hygiene::{lint_scripted_diff, lint_subtree}; +use lint_text_format::{ + lint_commit_msg, lint_tabs_whitespace, lint_trailing_newline, lint_trailing_whitespace, +}; +use util::{check_output, commit_range, get_git_root, git, LintFn, LintResult}; struct Linter { pub description: &'static str, @@ -26,7 +35,7 @@ fn get_linter_list() -> Vec<&'static Linter> { &Linter { description: "Check that all command line arguments are documented.", name: "doc", - lint_fn: lint_doc + lint_fn: lint_doc_args }, &Linter { description: "Check that no symbol from bitcoin-build-config.h is used without the header being included", @@ -157,577 +166,6 @@ fn parse_lint_args(args: &[String]) -> Vec<&'static Linter> { lint_values } -/// Return the git command -/// -/// Lint functions should use this command, so that only files tracked by git are considered and -/// temporary and untracked files are ignored. For example, instead of 'grep', 'git grep' should be -/// used. -fn git() -> Command { - let mut git = Command::new("git"); - git.arg("--no-pager"); - git -} - -/// Return stdout on success and a LintError on failure, when invalid UTF8 was detected or the -/// command did not succeed. -fn check_output(cmd: &mut std::process::Command) -> Result { - let out = cmd.output().expect("command error"); - if !out.status.success() { - return Err(String::from_utf8_lossy(&out.stderr).to_string()); - } - Ok(String::from_utf8(out.stdout) - .map_err(|e| { - format!("All path names, source code, messages, and output must be valid UTF8!\n{e}") - })? - .trim() - .to_string()) -} - -/// Return the git root as utf8, or panic -fn get_git_root() -> PathBuf { - PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap()) -} - -/// Return the commit range, or panic -fn commit_range() -> String { - // Use the env var, if set. E.g. COMMIT_RANGE='HEAD~n..HEAD' for the last 'n' commits. - env::var("COMMIT_RANGE").unwrap_or_else(|_| { - // Otherwise, assume that a merge commit exists. This merge commit is assumed - // to be the base, after which linting will be done. If the merge commit is - // HEAD, the range will be empty. - format!( - "{}..HEAD", - check_output(git().args(["rev-list", "--max-count=1", "--merges", "HEAD"])) - .expect("check_output failed") - ) - }) -} - -/// Return all subtree paths -fn get_subtrees() -> Vec<&'static str> { - // Keep in sync with [test/lint/README.md#git-subtree-checksh] - vec![ - "src/crc32c", - "src/crypto/ctaes", - "src/ipc/libmultiprocess", - "src/leveldb", - "src/minisketch", - "src/secp256k1", - ] -} - -/// Return the pathspecs to exclude by default -fn get_pathspecs_default_excludes() -> Vec { - get_subtrees() - .iter() - .chain(&[ - "doc/release-notes/release-notes-*", // archived notes - ]) - .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 get_subtrees() { - good &= Command::new("test/lint/git-subtree-check.sh") - .arg(subtree) - .status() - .expect("command_error") - .success(); - } - if good { - Ok(()) - } else { - Err("".to_string()) - } -} - -fn lint_scripted_diff() -> LintResult { - if Command::new("test/lint/commit-script-check.sh") - .arg(commit_range()) - .status() - .expect("command error") - .success() - { - Ok(()) - } else { - Err("".to_string()) - } -} - -fn lint_commit_msg() -> LintResult { - let mut good = true; - let commit_hashes = check_output(git().args([ - "-c", - "log.showSignature=false", - "log", - &commit_range(), - "--format=%H", - ]))?; - for hash in commit_hashes.lines() { - let commit_info = check_output(git().args([ - "-c", - "log.showSignature=false", - "log", - "--format=%B", - "-n", - "1", - hash, - ]))?; - if let Some(line) = commit_info.lines().nth(1) { - if !line.is_empty() { - println!( - "The subject line of commit hash {hash} is followed by a non-empty line. Subject lines should always be followed by a blank line." - ); - good = false; - } - } - } - if good { - Ok(()) - } else { - Err("".to_string()) - } -} - -fn lint_py_lint() -> LintResult { - let bin_name = "ruff"; - let checks = format!( - "--select={}", - [ - "B006", // mutable-argument-default - "B008", // function-call-in-default-argument - "E101", // indentation contains mixed spaces and tabs - "E401", // multiple imports on one line - "E402", // module level import not at top of file - "E701", // multiple statements on one line (colon) - "E702", // multiple statements on one line (semicolon) - "E703", // statement ends with a semicolon - "E711", // comparison to None should be 'if cond is None:' - "E713", // test for membership should be "not in" - "E714", // test for object identity should be "is not" - "E721", // do not compare types, use "isinstance()" - "E722", // do not use bare 'except' - "E742", // do not define classes named "l", "O", or "I" - "E743", // do not define functions named "l", "O", or "I" - "F401", // module imported but unused - "F402", // import module from line N shadowed by loop variable - "F403", // 'from foo_module import *' used; unable to detect undefined names - "F404", // future import(s) name after other statements - "F405", // foo_function may be undefined, or defined from star imports: bar_module - "F406", // "from module import *" only allowed at module level - "F407", // an undefined __future__ feature name was imported - "F541", // f-string without any placeholders - "F601", // dictionary key name repeated with different values - "F602", // dictionary key variable name repeated with different values - "F621", // too many expressions in an assignment with star-unpacking - "F631", // assertion test is a tuple, which are always True - "F632", // use ==/!= to compare str, bytes, and int literals - "F811", // redefinition of unused name from line N - "F821", // undefined name 'Foo' - "F822", // undefined name name in __all__ - "F823", // local variable name … referenced before assignment - "F841", // local variable 'foo' is assigned to but never used - "PLE", // Pylint errors - "W191", // indentation contains tabs - "W291", // trailing whitespace - "W292", // no newline at end of file - "W293", // blank line contains whitespace - "W605", // invalid escape sequence "x" - ] - .join(",") - ); - let files = check_output( - git() - .args(["ls-files", "--", "*.py"]) - .args(get_pathspecs_default_excludes()), - )?; - - let mut cmd = Command::new(bin_name); - cmd.args(["check", &checks]).args(files.lines()); - - match cmd.status() { - Ok(status) if status.success() => Ok(()), - Ok(_) => Err(format!("`{bin_name}` found errors!")), - Err(e) if e.kind() == ErrorKind::NotFound => { - println!( - "`{bin_name}` was not found in $PATH, skipping those checks." - ); - Ok(()) - } - Err(e) => Err(format!("Error running `{bin_name}`: {e}")), - } -} - -fn lint_std_filesystem() -> LintResult { - let found = git() - .args([ - "grep", - "--line-number", - "std::filesystem", - "--", - "./src/", - ":(exclude)src/ipc/libmultiprocess/", - ":(exclude)src/util/fs.h", - ":(exclude)src/test/kernel/test_kernel.cpp", - ":(exclude)src/bitcoin-chainstate.cpp", - ]) - .status() - .expect("command error") - .success(); - if found { - Err(r#" -Direct use of std::filesystem may be dangerous and buggy. Please include and use the -fs:: namespace, which has unsafe filesystem functions marked as deleted. - "# - .trim() - .to_string()) - } else { - Ok(()) - } -} - -fn lint_rpc_assert() -> LintResult { - let found = git() - .args([ - "grep", - "--line-number", - "--extended-regexp", - r"\<(A|a)ss(ume|ert)\(", - "--", - "src/rpc/", - "src/wallet/rpc*", - ":(exclude)src/rpc/server.cpp", - // src/rpc/server.cpp is excluded from this check since it's mostly meta-code. - ]) - .status() - .expect("command error") - .success(); - if found { - Err(r#" -CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. - -Aborting the whole process is undesirable for RPC code. So nonfatal -checks should be used over assert. See: src/util/check.h - "# - .trim() - .to_string()) - } else { - Ok(()) - } -} - -fn lint_boost_assert() -> LintResult { - let found = git() - .args([ - "grep", - "--line-number", - "--extended-regexp", - r"BOOST_ASSERT\(", - "--", - "*.cpp", - "*.h", - ]) - .status() - .expect("command error") - .success(); - if found { - Err(r#" -BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary -include of the boost/assert.hpp dependency. - "# - .trim() - .to_string()) - } else { - Ok(()) - } -} - -fn lint_doc_release_note_snippets() -> LintResult { - let non_release_notes = check_output(git().args([ - "ls-files", - "--", - "doc/release-notes/", - ":(exclude)doc/release-notes/*.*.md", // Assume that at least one dot implies a proper release note - ]))?; - if non_release_notes.is_empty() { - Ok(()) - } else { - println!("{non_release_notes}"); - Err(r#" -Release note snippets and other docs must be put into the doc/ folder directly. - -The doc/release-notes/ folder is for archived release notes of previous releases only. Snippets are -expected to follow the naming "/doc/release-notes-.md". - "# - .trim() - .to_string()) - } -} - -/// Return the pathspecs for whitespace related excludes -fn get_pathspecs_exclude_whitespace() -> Vec { - let mut list = get_pathspecs_default_excludes(); - list.extend( - [ - // Permanent excludes - "*.patch", - "src/qt/locale", - "contrib/windeploy/win-codesign.cert", - "doc/README_windows.txt", - // Temporary excludes, or existing violations - "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 (including Windows line endings [CR LF]) 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. - "# - .trim() - .to_string()) - } else { - Ok(()) - } -} - -fn lint_trailing_newline() -> LintResult { - let files = check_output( - git() - .args([ - "ls-files", "--", "*.py", "*.cpp", "*.h", "*.md", "*.rs", "*.sh", "*.cmake", - ]) - .args(get_pathspecs_default_excludes()), - )?; - let mut missing_newline = false; - for path in files.lines() { - let mut file = File::open(path).expect("must be able to open file"); - if file.seek(SeekFrom::End(-1)).is_err() { - continue; // Allow fully empty files - } - let mut buffer = [0u8; 1]; - file.read_exact(&mut buffer) - .expect("must be able to read the last byte"); - if buffer[0] != b'\n' { - missing_newline = true; - println!("{path}"); - } - } - if missing_newline { - Err(r#" -A trailing newline is required, because git may warn about it missing. Also, it can make diffs -verbose and can break git blame after appending lines. - -Thus, it is best to add the trailing newline now. - -Please add any false positives to the exclude list. - "# - .trim() - .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. - "# - .trim() - .to_string()) - } else { - Ok(()) - } -} - -fn lint_includes_build_config() -> LintResult { - let config_path = "./cmake/bitcoin-build-config.h.in"; - let defines_regex = format!( - r"^\s*(?!//).*({})", - check_output(Command::new("grep").args(["define", "--", config_path])) - .expect("grep failed") - .lines() - .map(|line| { - line.split_whitespace() - .nth(1) - .unwrap_or_else(|| panic!("Could not extract name in line: {line}")) - }) - .collect::>() - .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_default_excludes()), - ) - .expect("grep failed"); - git() - .args([ - "grep", - if mode { - "--files-without-match" - } else { - "--files-with-matches" - }, - if mode { - "^#include // IWYU pragma: keep$" - } else { - "#include " // Catch redundant includes with and without the IWYU pragma - }, - "--", - ]) - .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-build-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-build-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 '{defines_regex}' -- file_name - -Make sure to include it with the IWYU pragma. Otherwise, IWYU may falsely instruct to remove the -include again. - -#include // IWYU pragma: keep - "# - ) - .trim() - .to_string()); - } - let redundant = print_affected_files(false); - if redundant { - return Err(r#" -None of the files use a symbol declared in the bitcoin-build-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() - .expect("command error") - .success() - { - Ok(()) - } else { - Err("".to_string()) - } -} - -fn lint_markdown() -> LintResult { - let bin_name = "mlc"; - let mut md_ignore_paths = get_subtrees(); - md_ignore_paths.push("./doc/README_doxygen.md"); - let md_ignore_path_str = md_ignore_paths.join(","); - - let mut cmd = Command::new(bin_name); - cmd.args([ - "--offline", - "--ignore-path", - md_ignore_path_str.as_str(), - "--gitignore", - "--gituntracked", - "--root-dir", - ".", - ]) - .stdout(Stdio::null()); // Suppress overly-verbose output - - match cmd.output() { - Ok(output) if output.status.success() => Ok(()), - Ok(output) => { - let stderr = String::from_utf8_lossy(&output.stderr); - Err(format!( - r#" -One or more markdown links are broken. - -Note: relative links are preferred as jump-to-file works natively within Emacs, but they are not required. - -Markdown link errors found: -{stderr} - "# - ) - .trim() - .to_string()) - } - Err(e) if e.kind() == ErrorKind::NotFound => { - println!("`mlc` was not found in $PATH, skipping markdown lint check."); - Ok(()) - } - Err(e) => Err(format!("Error running mlc: {e}")), // Misc errors - } -} - fn run_all_python_linters() -> LintResult { let mut good = true; let lint_dir = get_git_root().join("test/lint"); diff --git a/test/lint/test_runner/src/util.rs b/test/lint/test_runner/src/util.rs new file mode 100644 index 00000000000..11e332e4c5e --- /dev/null +++ b/test/lint/test_runner/src/util.rs @@ -0,0 +1,84 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/license/mit/. + +use std::env; +use std::path::PathBuf; +use std::process::Command; + +/// A possible error returned by any of the linters. +/// +/// The error string should explain the failure type and list all violations. +pub type LintError = String; +pub type LintResult = Result<(), LintError>; +pub type LintFn = fn() -> LintResult; + +/// Return the git command +/// +/// Lint functions should use this command, so that only files tracked by git are considered and +/// temporary and untracked files are ignored. For example, instead of 'grep', 'git grep' should be +/// used. +pub fn git() -> Command { + let mut git = Command::new("git"); + git.arg("--no-pager"); + git +} + +/// Return stdout on success and a LintError on failure, when invalid UTF8 was detected or the +/// command did not succeed. +pub fn check_output(cmd: &mut Command) -> Result { + let out = cmd.output().expect("command error"); + if !out.status.success() { + return Err(String::from_utf8_lossy(&out.stderr).to_string()); + } + Ok(String::from_utf8(out.stdout) + .map_err(|e| { + format!("All path names, source code, messages, and output must be valid UTF8!\n{e}") + })? + .trim() + .to_string()) +} + +/// Return the git root as utf8, or panic +pub fn get_git_root() -> PathBuf { + PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap()) +} + +/// Return the commit range, or panic +pub fn commit_range() -> String { + // Use the env var, if set. E.g. COMMIT_RANGE='HEAD~n..HEAD' for the last 'n' commits. + env::var("COMMIT_RANGE").unwrap_or_else(|_| { + // Otherwise, assume that a merge commit exists. This merge commit is assumed + // to be the base, after which linting will be done. If the merge commit is + // HEAD, the range will be empty. + format!( + "{}..HEAD", + check_output(git().args(["rev-list", "--max-count=1", "--merges", "HEAD"])) + .expect("check_output failed") + ) + }) +} + +/// Return all subtree paths +pub fn get_subtrees() -> Vec<&'static str> { + // Keep in sync with [test/lint/README.md#git-subtree-checksh] + vec![ + "src/crc32c", + "src/crypto/ctaes", + "src/ipc/libmultiprocess", + "src/leveldb", + "src/minisketch", + "src/secp256k1", + ] +} + +/// Return the pathspecs to exclude by default +pub fn get_pathspecs_default_excludes() -> Vec { + get_subtrees() + .iter() + .chain(&[ + "doc/release-notes/release-notes-*", // archived notes + ]) + .map(|s| format!(":(exclude){s}")) + .collect() +}