diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 00000000000..69745b7cb5f --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,7 @@ +--- +Checks: '-*,modernize-use-nullptr,modernize-use-auto' +WarningsAsErrors: '' +HeaderFilterRegex: '' +AnalyzeTemporaryDtors: false +FormatStyle: none +... diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2d17748bee3..a205a1a29e7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -74,17 +74,32 @@ repos: stages: - commit - manual +- repo: local + hooks: + - id: clang-tidy + name: clang-tidy + entry: tools/clang_tidy.py + require_serial: true + verbose: true + stages: + - commit + - manual + language: system + files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$ + additional_dependencies: + - clang-tidy - repo: local hooks: - id: clang-format name: clang-format - entry: tools/clang_format_wrapper.py + description: "Run clang-format in two passes (reformat, then break long lines)" + entry: tools/clang_format.py require_serial: true stages: - commit - manual language: python - files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$ + files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|m|mm|proto|vert)$ additional_dependencies: - clang-format - repo: https://github.com/psf/black @@ -114,14 +129,3 @@ repos: stages: - commit - manual -- repo: local - hooks: - - id: line-length - name: line-length - description: Check for lines longer 100 and brakes them before 80. - entry: ./tools/line_length.py - stages: - - commit - - manual - language: python - files: \.(c|cpp|h)$ diff --git a/CMakeLists.txt b/CMakeLists.txt index 235a620616e..dc7444d4ff2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -319,6 +319,13 @@ if(CMAKE_VERSION VERSION_LESS "3.7.0") set(CMAKE_INCLUDE_CURRENT_DIR ON) endif() +# Export compile-commands.json and symlink it into the source directory (needed +# for clang-tidy pre-commit hook) +set(CMAKE_EXPORT_COMPILE_COMMANDS ON) +file(CREATE_LINK + ${CMAKE_BINARY_DIR}/compile_commands.json + ${CMAKE_SOURCE_DIR}/compile_commands.json SYMBOLIC) + # Mixxx itself add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL src/analyzer/analyzerbeats.cpp diff --git a/tools/clang_format.py b/tools/clang_format.py new file mode 100755 index 00000000000..d434ad49ca0 --- /dev/null +++ b/tools/clang_format.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +import argparse +import logging +import os +import re +import subprocess +import sys +import tempfile +import typing + +import githelper + + +# We recommend a maximum line length of 80, but do allow up to 100 characters +# if deemed necessary by the developer. Lines that exceed that limit will +# be wrapped after 80 characters automatically. +LINE_LENGTH_THRESHOLD = 100 +BREAK_BEFORE = 80 + + +def get_clang_format_config_with_columnlimit(rootdir, limit): + """Create a temporary config with ColumnLimit set to 80.""" + cpp_file = os.path.join(rootdir, "src/mixxx.cpp") + proc = subprocess.run( + ["clang-format", "--dump-config", cpp_file], + capture_output=True, + text=True, + ) + proc.check_returncode() + return re.sub( + r"(ColumnLimit:\s*)\d+", r"\g<1>{}".format(BREAK_BEFORE), proc.stdout + ) + + +def run_clang_format_on_lines(rootdir, file_to_format, stylepath=None): + logger = logging.getLogger(__name__) + + line_arguments = [ + "--lines={}:{}".format(start, end) + for start, end in file_to_format.lines + ] + assert line_arguments + + logger.info("Reformatting %s...", file_to_format.filename) + filename = os.path.join(rootdir, file_to_format.filename) + cmd = [ + "clang-format", + "--style=file", + # The --assume-filename argument sets the path for the .clang-format + # config file implcitly by assuming a different location of the file to + # format + "--assume-filename={}".format( + os.path.join( + stylepath if stylepath else rootdir, file_to_format.filename + ) + ), + *line_arguments, + ] + + with open(filename) as fp: + logger.debug("Executing: %r", cmd) + proc = subprocess.run(cmd, stdin=fp, capture_output=True, text=True) + try: + proc.check_returncode() + except subprocess.CalledProcessError: + logger.error( + "Error while executing command %s: %s", cmd, proc.stderr, + ) + raise + + if proc.stderr: + logger.error(proc.stderr) + with open(filename, mode="w") as fp: + fp.write(proc.stdout) + + +def main(argv: typing.Optional[typing.List[str]] = None) -> int: + logging.basicConfig( + format="[%(levelname)s] %(message)s", level=logging.INFO + ) + + logger = logging.getLogger(__name__) + + parser = argparse.ArgumentParser() + parser.add_argument("--from-ref", help="use changes changes since commit") + parser.add_argument("--to-ref", help="use changes until commit") + parser.add_argument("files", nargs="*", help="only check these files") + args = parser.parse_args(argv) + + if not args.from_ref: + args.from_ref = os.getenv("PRE_COMMIT_FROM_REF") or os.getenv( + "PRE_COMMIT_SOURCE" + ) + + if not args.to_ref: + args.to_ref = os.getenv("PRE_COMMIT_TO_REF") or os.getenv( + "PRE_COMMIT_ORIGIN" + ) + + # Filter filenames + rootdir = githelper.get_toplevel_path() + + # First pass: Format added/changed lines using clang-format + logger.info("First pass: Reformatting added/changed lines...") + files_with_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + to_ref=args.to_ref, + filter_lines=lambda line: line.added, + include_files=args.files, + ) + for changed_file in files_with_added_lines: + run_clang_format_on_lines(rootdir, changed_file) + + # Second pass: Wrap long added/changed lines using clang-format + logger.info("Second pass: Breaking long added/changed lines...") + files_with_long_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + to_ref=args.to_ref, + filter_lines=lambda line: line.added + and len(line.text) > LINE_LENGTH_THRESHOLD, + include_files=args.files, + ) + config = get_clang_format_config_with_columnlimit(rootdir, BREAK_BEFORE) + with tempfile.TemporaryDirectory(prefix="clang-format") as tempdir: + # Create temporary config with ColumnLimit enabled + configfile = os.path.join(tempdir, ".clang-format") + with open(configfile, mode="w") as configfp: + configfp.write(config) + + for changed_file in files_with_long_added_lines: + run_clang_format_on_lines(rootdir, changed_file, stylepath=tempdir) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/clang_format_wrapper.py b/tools/clang_format_wrapper.py deleted file mode 100755 index b2fc23454cf..00000000000 --- a/tools/clang_format_wrapper.py +++ /dev/null @@ -1,130 +0,0 @@ -#!/usr/bin/env python -"""Wrapper script for `git-clang-format` to avoid race conditions when -merging/rebasing CPP files with merge conflicts.""" -from collections import namedtuple -from os import environ as env -from subprocess import check_output -from typing import List, Iterable -import argparse -import enum -import logging -import sys - -logger = logging.getLogger(__name__) - - -class GitState(enum.Enum): - """ - Excerpt from https://git-scm.com/docs/git-status - - For paths with merge conflicts, X and Y show the modification states - of each side of the merge. For paths that do not have merge conflicts, - X shows the status of the index, and Y shows the status of the work - tree. For untracked paths, XY are ??. Other status codes can be - interpreted as follows: - M = modified - A = added - D = deleted - R = renamed - C = copied - U = updated but unmerged - """ - - ADDED = "A" - COPIED = "C" - DELETED = "D" - MODIFIED = "M" - RENAMED = "R" - UNMODIFIED = " " - UNTRACKED = "?" - UPDATED_BUT_UNMERGED = "U" - - -GitEdit = namedtuple("GitEdit", ["x", "y", "path"]) - - -def get_git_status() -> Iterable[GitEdit]: - """Inspect `git status` output, return GitEdits generator.""" - - command = ["git", "status", "--porcelain", "--untracked-files=no"] - logger.debug("Executing `%s`", " ".join(command)) - git_status_output = check_output(args=command, text=True) - - edits = [ - edit for edit in git_status_output.rstrip().split("\n") if edit != "" - ] - logger.debug( - "Found %d path(s) with state change in GIT status output", len(edits) - ) - - if edits: - logger.debug("Pending GIT edits:\n%s", "\n".join(edits)) - - return ( - git_edit - for git_edit in ( - GitEdit(edit[0:1], edit[1:2], edit[3:]) for edit in edits - ) - if git_edit.x in (GitState.MODIFIED.value, GitState.ADDED.value) - ) - - -def run_clang_format(file_list: List[str]) -> bool: - """Execute git-clang-format for provided `file_list`. - - Return True if `git-clang-format` has errors or has modified any file, - i.e. if it failed the formatting test. - """ - if not file_list: - logger.info("No file to be formatted, exiting") - return - - command = ["git-clang-format"] - - src_commit = env.get("PRE_COMMIT_FROM_REF") or env.get("PRE_COMMIT_SOURCE") - - if src_commit: - command.append(src_commit) - - command.append("--") - command.extend(file_list) - - logger.info("Executing `%s`", " ".join(command)) - out = check_output(command, text=True) - logger.debug(out) - - return False if out == "clang-format did not modify any files\n" else True - - -def main(): - # Setup CLI argument parser - logging.basicConfig( - format="[%(levelname)s] %(message)s", level=logging.DEBUG - ) - parser = argparse.ArgumentParser() - parser.add_argument( - "files", nargs="*", help="Restrict formatting to these files" - ) - args = parser.parse_args() - logger.debug("Wrapper called with `files` arguments: %s", args.files) - - # Inspect GIT status output and filter paths that must be clang-formatted - paths_to_be_clang_formatted = [ - status.path for status in get_git_status() if status.path in args.files - ] - logger.debug( - "Found %d staged path(s) to be clang-formatted", - len(paths_to_be_clang_formatted), - ) - - if paths_to_be_clang_formatted: - logger.debug("Paths:\n%s", "\n".join(paths_to_be_clang_formatted)) - - if run_clang_format(paths_to_be_clang_formatted): - return 1 - - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/tools/clang_tidy.py b/tools/clang_tidy.py new file mode 100755 index 00000000000..13513971a00 --- /dev/null +++ b/tools/clang_tidy.py @@ -0,0 +1,85 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +import argparse +import logging +import json +import os +import subprocess +import sys +import typing + +import githelper + + +def run_clang_tidy_on_lines(rootdir, changed_files): + logger = logging.getLogger(__name__) + + files = [] + lines = [] + for changed_file in changed_files: + logger.debug("Tidying up %s...", changed_file.filename) + filename = os.path.join(rootdir, changed_file.filename) + files.append(filename) + lines.append({"name": filename, "lines": changed_file.lines}) + + cmd = [ + "clang-tidy", + "--fix", + "--line-filter={}".format(json.dumps(lines)), + *files, + ] + + proc = subprocess.run(cmd, capture_output=True, text=True) + try: + proc.check_returncode() + except subprocess.CalledProcessError: + logger.error( + "Error while executing command %s: %s", cmd, proc.stderr, + ) + raise + + +def main(argv: typing.Optional[typing.List[str]] = None) -> int: + logging.basicConfig( + format="[%(levelname)s] %(message)s", level=logging.INFO + ) + + logger = logging.getLogger(__name__) + + parser = argparse.ArgumentParser() + parser.add_argument("--from-ref", help="compare against git reference") + parser.add_argument("files", nargs="*", help="only check these files") + args = parser.parse_args(argv) + + if not args.from_ref: + args.from_ref = os.getenv("PRE_COMMIT_FROM_REF") or os.getenv( + "PRE_COMMIT_SOURCE" + ) + + if not args.to_ref: + args.from_ref = os.getenv("PRE_COMMIT_TO_REF") or os.getenv( + "PRE_COMMIT_ORIGIN" + ) + + rootdir = githelper.get_toplevel_path() + + # clang-tidy requires a compile_commands.json file to work correctly + # If it doesn't exist, this hook should be skipped without failing. + if not os.path.exists(os.path.join(rootdir, "compile_commands.json")): + logger.warning("compile-commands.json not found, skipping hook...") + return 0 + + # Detect added/changed lines and run clang-tidy on them + files_with_added_lines = githelper.get_changed_lines_grouped( + from_ref=args.from_ref, + to_ref=args.to_ref, + filter_lines=lambda line: line.added, + include_files=args.files, + ) + run_clang_tidy_on_lines(rootdir, files_with_added_lines) + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/githelper.py b/tools/githelper.py new file mode 100644 index 00000000000..c526db3d0e9 --- /dev/null +++ b/tools/githelper.py @@ -0,0 +1,133 @@ +import itertools +import re +import logging +import subprocess +import typing + + +Line = typing.NamedTuple( + "Line", + [("sourcefile", str), ("number", int), ("text", str), ("added", bool)], +) +FileLines = typing.NamedTuple( + "FileLines", + [("filename", str), ("lines", typing.Sequence[typing.Tuple[int, int]])], +) + + +def get_toplevel_path() -> str: + logger = logging.getLogger(__name__) + + cmd = ["git", "rev-parse", "--show-toplevel"] + logger.debug("Executing: %r", cmd) + return subprocess.check_output(cmd, text=True).strip() + + +def get_changed_lines( + from_ref=None, to_ref=None, filter_lines=None, include_files=None +) -> typing.Iterable[Line]: + """Inspect `git diff-index` output, yields changed lines.""" + + logger = logging.getLogger(__name__) + + if to_ref: + changeset = "{}...{}".format(from_ref if from_ref else "HEAD", to_ref) + else: + changeset = from_ref if from_ref else "HEAD" + + # We're using the pre-commit framework which stashes all unstaged changes + # before running the pre-commit hooks, so we don't need to add `--cached` + # here. Also, if we run 2 hooks that modify the files, the second hook + # should work on the diff that includes the unstaged changes made by the + # first hook, not the original diff. + cmd = ["git", "diff", "--patch", "--unified=0", changeset] + if include_files: + cmd.extend(["--", *include_files]) + logger.debug("Executing: %r", cmd) + proc = subprocess.run(cmd, capture_output=True) + proc.check_returncode() + current_file = None + hunk_lines_left = 0 + for line in proc.stdout.decode(errors="replace").splitlines(): + match_file = re.match(r"^\+\+\+ b/(.*)$", line) + if match_file: + # Current line contains a diff filename + assert hunk_lines_left == 0 + current_file = match_file.group(1) + continue + + match_lineno = re.match( + r"^@@ -(\d+(?:,\d+)?) \+([0-9]+(?:,[0-9]+)?) @@", line + ) + if match_lineno: + # Current line contains a hunk header + assert current_file is not None + assert hunk_lines_left == 0 + start_removed, _, length_removed = match_lineno.group(1).partition( + "," + ) + start_added, _, length_added = match_lineno.group(2).partition(",") + lineno_removed = int(start_removed) + lineno_added = int(start_added) + hunk_lines_left = int(length_removed) if length_removed else 1 + hunk_lines_left += int(length_added) if length_added else 1 + continue + + if hunk_lines_left and line: + # Current line contains an added/removed line + hunk_lines_left -= 1 + + assert line[0] in ("+", "-") + if line.startswith("+"): + lineno_added += 1 + current_lineno = lineno_added + else: + lineno_removed += 1 + current_lineno = lineno_removed + + lineobj = Line( + sourcefile=current_file, + number=current_lineno - 1, + text=line[1:], + added=line.startswith("+"), + ) + + if filter_lines is None or filter_lines(lineobj): + yield lineobj + + # If we reach this part, the line does not contain a diff filename or a + # hunk header and does not belog to a hunk. This means that this line + # will be ignored implicitly. + + # Make sure we really parsed all lines from the last hunk + assert hunk_lines_left == 0 + + +def get_changed_lines_grouped( + from_ref=None, to_ref=None, filter_lines=None, include_files=None +) -> typing.Iterable[FileLines]: + """Inspect `git diff-index` output, yields lines grouped by file.""" + + lines = get_changed_lines(from_ref, to_ref, filter_lines, include_files) + for filename, file_lines in itertools.groupby( + lines, key=lambda line: line.sourcefile + ): + grouped_linenumbers = [] + start_linenumber = None + last_linenumber = None + for line in file_lines: + if None not in (start_linenumber, last_linenumber): + if line.number != last_linenumber + 1: + grouped_linenumbers.append( + (start_linenumber, last_linenumber) + ) + + if start_linenumber is None: + start_linenumber = line.number + last_linenumber = line.number + + if None not in (start_linenumber, last_linenumber): + grouped_linenumbers.append((start_linenumber, last_linenumber)) + + if grouped_linenumbers: + yield FileLines(filename, grouped_linenumbers) diff --git a/tools/line_length.py b/tools/line_length.py deleted file mode 100755 index dd816384ade..00000000000 --- a/tools/line_length.py +++ /dev/null @@ -1,184 +0,0 @@ -#!/usr/bin/env python3 -# -*- coding: utf-8 -*- -import argparse -import re -import logging -import os -import itertools -import subprocess -import tempfile -import typing - -# We recommend a maximum line length of 80, but do allow up to 100 characters -# if deemed necessary by the developer. Lines that exceed that limit will -# be wrapped after 80 characters automatically. -LINE_LENGTH_THRESHOLD = 100 -BREAK_BEFORE = 80 - -Line = typing.NamedTuple( - "Line", [("sourcefile", str), ("number", int), ("text", str)] -) -LineGenerator = typing.Generator[Line, None, None] -FileLines = typing.NamedTuple( - "FileLines", - [("filename", str), ("lines", typing.Sequence[typing.Tuple[int, int]])], -) - - -def get_git_added_lines() -> LineGenerator: - proc = subprocess.run( - ["git", "diff", "--cached", "--unified=0"], capture_output=True - ) - proc.check_returncode() - current_file = None - current_lineno = None - lines_left = 0 - for line in proc.stdout.decode(errors="replace").splitlines(): - match_file = re.match(r"^\+\+\+ b/(.*)$", line) - if match_file: - current_file = match_file.group(1) - lines_left = 0 - continue - - match_lineno = re.match( - r"^@@ -\d+(?:,\d+)? \+([0-9]+(?:,[0-9]+)?) @@", line - ) - if match_lineno: - start, _, length = match_lineno.group(1).partition(",") - current_lineno = int(start) - lines_left = 1 - if length: - lines_left += int(length) - continue - - if lines_left and line.startswith("+"): - yield Line( - sourcefile=current_file, number=current_lineno, text=line[1:] - ) - lines_left -= 1 - current_lineno += 1 - - -def group_lines( - lines: LineGenerator, -) -> typing.Generator[FileLines, None, None]: - for filename, file_lines in itertools.groupby( - lines, key=lambda line: line.sourcefile - ): - grouped_linenumbers = [] - start_linenumber = None - last_linenumber = None - for line in file_lines: - if None not in (start_linenumber, last_linenumber): - if line.number != last_linenumber + 1: - grouped_linenumbers.append( - (start_linenumber, last_linenumber) - ) - - if start_linenumber is None: - start_linenumber = line.number - last_linenumber = line.number - - if None not in (start_linenumber, last_linenumber): - grouped_linenumbers.append((start_linenumber, last_linenumber)) - - if grouped_linenumbers: - yield FileLines(filename, grouped_linenumbers) - - -def main(argv: typing.Optional[typing.List[str]] = None) -> int: - logging.basicConfig() - logger = logging.getLogger(__name__) - - import sys - - print(sys.argv) - - parser = argparse.ArgumentParser() - parser.add_argument("files", nargs="*", help="only check these files") - args = parser.parse_args(argv) - print(args) - - all_lines = get_git_added_lines() - - # Filter filenames - rootdir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") - if args.files: - files = [ - os.path.abspath(os.path.join(rootdir, filename)) - for filename in args.files - ] - all_lines = ( - line - for line in all_lines - if os.path.abspath(os.path.join(line.sourcefile)) in files - ) - - # Only keep long lines - long_lines = ( - line - for line in all_lines - if (len(line.text) - 1) >= LINE_LENGTH_THRESHOLD - ) - changed_files = group_lines(long_lines) - - cpp_file = os.path.join(rootdir, "src/mixxx.cpp") - proc = subprocess.run( - ["clang-format", "--dump-config", cpp_file], - capture_output=True, - text=True, - ) - proc.check_returncode() - - with tempfile.TemporaryDirectory(prefix="clang-format") as tempdir: - # Create temporary config with ColumnLimit enabled - configfile = os.path.join(tempdir, ".clang-format") - with open(configfile, mode="w") as configfp: - configfp.write( - re.sub( - r"(ColumnLimit:\s*)\d+", - r"\g<1>{}".format(BREAK_BEFORE), - proc.stdout, - ) - ) - - for changed_file in changed_files: - line_arguments = [ - "--lines={}:{}".format(start, end) - for start, end in changed_file.lines - ] - - if not line_arguments: - continue - - cmd = [ - "clang-format", - "--style=file", - "--assume-filename={}".format( - os.path.join(tempdir, changed_file.filename) - ), - *line_arguments, - ] - - filename = os.path.join(rootdir, changed_file.filename) - with open(filename) as fp: - proc = subprocess.run( - cmd, stdin=fp, capture_output=True, text=True - ) - try: - proc.check_returncode() - except subprocess.CalledProcessError: - logger.error( - "Error while executing command %s: %s", cmd, proc.stderr, - ) - return 1 - - if proc.stderr: - print(proc.stderr) - with open(filename, mode="w+") as fp: - fp.write(proc.stdout) - return 0 - - -if __name__ == "__main__": - exit(main())