From bf5f846fc689156792f91dc035c61648bea6bd8b Mon Sep 17 00:00:00 2001 From: Oxan van Leeuwen Date: Mon, 9 Aug 2021 22:43:18 +0200 Subject: [PATCH] Refactor clang-tidy script to use actual compiler flags and includes (#2133) Co-authored-by: Otto winter --- .clang-tidy | 6 +- .github/workflows/ci.yml | 4 - .gitignore | 1 + .vscode/tasks.json | 2 +- esphome/components/sgp30/sgp30.cpp | 4 +- esphome/components/sgp40/sgp40.cpp | 4 +- esphome/components/sntp/sntp_component.cpp | 5 ++ platformio.ini | 52 +++++++++--- script/build_compile_commands.py | 16 ---- script/clang-tidy | 54 ++++++++++--- script/helpers.py | 92 ++++++++-------------- 11 files changed, 131 insertions(+), 109 deletions(-) delete mode 100755 script/build_compile_commands.py diff --git a/.clang-tidy b/.clang-tidy index fc5ce854f1..473529a9b1 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -14,7 +14,12 @@ Checks: >- -cert-str34-c, -clang-analyzer-optin.cplusplus.UninitializedObject, -clang-analyzer-osx.*, + -clang-diagnostic-delete-abstract-non-virtual-dtor, + -clang-diagnostic-delete-non-abstract-non-virtual-dtor, -clang-diagnostic-shadow-field, + -clang-diagnostic-sign-compare, + -clang-diagnostic-unused-variable, + -clang-diagnostic-unused-const-variable, -cppcoreguidelines-avoid-c-arrays, -cppcoreguidelines-avoid-goto, -cppcoreguidelines-avoid-magic-numbers, @@ -80,7 +85,6 @@ Checks: >- -readability-use-anyofallof, -warnings-as-errors WarningsAsErrors: '*' -HeaderFilterRegex: '^.*/src/esphome/.*' AnalyzeTemporaryDtors: false FormatStyle: google CheckOptions: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 433c7fb872..d7a841f84c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,10 +36,6 @@ jobs: container: ghcr.io/esphome/esphome-lint:1.1 steps: - uses: actions/checkout@v2 - # Set up the pio project so that the cpp checks know how files are compiled - # (build flags, libraries etc) - - name: Set up platformio environment - run: pio init --ide atom - name: Register problem matchers run: | diff --git a/.gitignore b/.gitignore index 954ecb2cb8..92d92e4b3b 100644 --- a/.gitignore +++ b/.gitignore @@ -125,4 +125,5 @@ config/ tests/build/ tests/.esphome/ /.temp-clang-tidy.cpp +/.temp/ .pio/ diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 8c55646f28..b6584bc735 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -10,7 +10,7 @@ { "label": "clang-tidy", "type": "shell", - "command": "test -f .gcc-flags.json || pio init --silent --ide atom; ./script/clang-tidy", + "command": "./script/clang-tidy", "problemMatcher": [ { "owner": "clang-tidy", diff --git a/esphome/components/sgp30/sgp30.cpp b/esphome/components/sgp30/sgp30.cpp index fdc6ae031d..e30d6d3adc 100644 --- a/esphome/components/sgp30/sgp30.cpp +++ b/esphome/components/sgp30/sgp30.cpp @@ -47,7 +47,7 @@ void SGP30Component::setup() { } this->serial_number_ = (uint64_t(raw_serial_number[0]) << 24) | (uint64_t(raw_serial_number[1]) << 16) | (uint64_t(raw_serial_number[2])); - ESP_LOGD(TAG, "Serial Number: %llu", this->serial_number_); + ESP_LOGD(TAG, "Serial Number: %" PRIu64, this->serial_number_); // Featureset identification for future use if (!this->write_command_(SGP30_CMD_GET_FEATURESET)) { @@ -245,7 +245,7 @@ void SGP30Component::dump_config() { break; } } else { - ESP_LOGCONFIG(TAG, " Serial number: %llu", this->serial_number_); + ESP_LOGCONFIG(TAG, " Serial number: %" PRIu64, this->serial_number_); if (this->eco2_baseline_ != 0x0000 && this->tvoc_baseline_ != 0x0000) { ESP_LOGCONFIG(TAG, " Baseline:"); ESP_LOGCONFIG(TAG, " eCO2 Baseline: 0x%04X", this->eco2_baseline_); diff --git a/esphome/components/sgp40/sgp40.cpp b/esphome/components/sgp40/sgp40.cpp index 3b634353c4..8d93b3e1b1 100644 --- a/esphome/components/sgp40/sgp40.cpp +++ b/esphome/components/sgp40/sgp40.cpp @@ -23,7 +23,7 @@ void SGP40Component::setup() { } this->serial_number_ = (uint64_t(raw_serial_number[0]) << 24) | (uint64_t(raw_serial_number[1]) << 16) | (uint64_t(raw_serial_number[2])); - ESP_LOGD(TAG, "Serial Number: %llu", this->serial_number_); + ESP_LOGD(TAG, "Serial Number: %" PRIu64, this->serial_number_); // Featureset identification for future use if (!this->write_command_(SGP40_CMD_GET_FEATURESET)) { @@ -248,7 +248,7 @@ void SGP40Component::dump_config() { break; } } else { - ESP_LOGCONFIG(TAG, " Serial number: %llu", this->serial_number_); + ESP_LOGCONFIG(TAG, " Serial number: %" PRIu64, this->serial_number_); ESP_LOGCONFIG(TAG, " Minimum Samples: %f", VOC_ALGORITHM_INITIAL_BLACKOUT); } LOG_UPDATE_INTERVAL(this); diff --git a/esphome/components/sntp/sntp_component.cpp b/esphome/components/sntp/sntp_component.cpp index b667f3b1ce..ff176b1d4e 100644 --- a/esphome/components/sntp/sntp_component.cpp +++ b/esphome/components/sntp/sntp_component.cpp @@ -8,6 +8,11 @@ #include "sntp.h" #endif +// Yes, the server names are leaked, but that's fine. +#ifdef CLANG_TIDY +#define strdup(x) (const_cast(x)) +#endif + namespace esphome { namespace sntp { diff --git a/platformio.ini b/platformio.ini index e1fb845358..9f1366d9af 100644 --- a/platformio.ini +++ b/platformio.ini @@ -1,12 +1,28 @@ -; This file is so that the C++ files in this repo -; can be edited with IDEs like VSCode or CLion -; with the platformio system +; This PlatformIO project is for development purposes *only*: clang-tidy derives its compilation +; database from here, and IDEs like CLion and VSCode also use it. This does not actually create a +; usable binary. ; It's *not* used during runtime. [platformio] -default_envs = livingroom8266 +default_envs = esp8266 src_dir = . -include_dir = include +include_dir = + +[runtime] +; This are the flags as set by the runtime. +build_flags = + -Wno-unused-variable + -Wno-unused-but-set-variable + -Wno-sign-compare + +[clangtidy] +; This are the flags for clang-tidy. +build_flags = + -Wall + -Wunreachable-code + -Wfor-loop-analysis + -Wshadow-field + -Wshadow-field-in-constructor [common] lib_deps = @@ -19,17 +35,13 @@ lib_deps = 6865@1.0.0 ; TM1651 Battery Display 6306@1.0.3 ; HM3301 build_flags = - -fno-exceptions - -Wno-sign-compare - -Wno-unused-but-set-variable - -Wno-unused-variable - -DCLANG_TIDY -DESPHOME_LOG_LEVEL=ESPHOME_LOG_LEVEL_VERY_VERBOSE src_filter = + + + +<.temp/all-include.cpp> -[env:livingroom8266] +[common:esp8266] ; use Arduino framework v2.4.2 for clang-tidy (latest 2.5.2 breaks static code analysis, see #760) platform = platformio/espressif8266@1.8.0 framework = arduino @@ -42,7 +54,7 @@ lib_deps = build_flags = ${common.build_flags} src_filter = ${common.src_filter} -[env:livingroom32] +[common:esp32] platform = platformio/espressif32@3.2.0 framework = arduino board = nodemcu-32s @@ -56,3 +68,19 @@ build_flags = src_filter = ${common.src_filter} - + +[env:esp8266] +extends = common:esp8266 +build_flags = ${common:esp8266.build_flags} ${runtime.build_flags} + +[env:esp8266-tidy] +extends = common:esp8266 +build_flags = ${common:esp8266.build_flags} ${clangtidy.build_flags} + +[env:esp32] +extends = common:esp32 +build_flags = ${common:esp32.build_flags} ${runtime.build_flags} + +[env:esp32-tidy] +extends = common:esp32 +build_flags = ${common:esp32.build_flags} ${clangtidy.build_flags} diff --git a/script/build_compile_commands.py b/script/build_compile_commands.py deleted file mode 100755 index 4ac14f08b4..0000000000 --- a/script/build_compile_commands.py +++ /dev/null @@ -1,16 +0,0 @@ -#!/usr/bin/env python3 -import sys -import os.path - -sys.path.append(os.path.dirname(__file__)) -from helpers import build_all_include, build_compile_commands - - -def main(): - build_all_include() - build_compile_commands() - print("Done.") - - -if __name__ == "__main__": - main() diff --git a/script/clang-tidy b/script/clang-tidy index 11b9818016..c2f47bad11 100755 --- a/script/clang-tidy +++ b/script/clang-tidy @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import argparse +import json import multiprocessing import os import queue @@ -15,27 +16,57 @@ import click import pexpect sys.path.append(os.path.dirname(__file__)) -from helpers import basepath, shlex_quote, get_output, build_compile_commands, \ - build_all_include, temp_header_file, git_ls_files, filter_changed +from helpers import shlex_quote, get_output, \ + build_all_include, temp_header_file, git_ls_files, filter_changed, load_idedata -def run_tidy(args, tmpdir, queue, lock, failed_files): +def clang_options(idedata): + cmd = [ + # target 32-bit arch (this prevents size mismatch errors on a 64-bit host) + '-m32', + # disable built-in include directories from the host + '-nostdinc', + '-nostdinc++', + # allow to condition code on the presence of clang-tidy + '-DCLANG_TIDY' + ] + + # copy compiler flags, except those clang doesn't understand. + cmd.extend(flag for flag in idedata['cxx_flags'].split(' ') + if flag not in ('-free', '-fipa-pta', '-mlongcalls', '-mtext-section-literals')) + + # defines + cmd.extend(f'-D{define}' for define in idedata['defines']) + + # add include directories, using -isystem for dependencies to suppress their errors + for directory in idedata['includes']['toolchain']: + cmd.extend(['-isystem', directory]) + for directory in sorted(set(idedata['includes']['build'])): + dependency = "framework-arduino" in directory or "/libdeps/" in directory + cmd.extend(['-isystem' if dependency else '-I', directory]) + + return cmd + + +def run_tidy(args, options, tmpdir, queue, lock, failed_files): while True: path = queue.get() - invocation = ['clang-tidy-11', '-header-filter=^{}/.*'.format(re.escape(basepath))] + invocation = ['clang-tidy-11'] + if tmpdir is not None: - invocation.append('-export-fixes') + invocation.append('--export-fixes') # Get a temporary file. We immediately close the handle so clang-tidy can # overwrite it. (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir) os.close(handle) invocation.append(name) - invocation.append('-p=.') + if args.quiet: invocation.append('-quiet') - for arg in ['-Wfor-loop-analysis', '-Wshadow-field', '-Wshadow-field-in-constructor']: - invocation.append('-extra-arg={}'.format(arg)) + invocation.append(os.path.abspath(path)) + invocation.append('--') + invocation.extend(options) invocation_s = ' '.join(shlex_quote(x) for x in invocation) # Use pexpect for a pseudy-TTY with colored output @@ -95,8 +126,8 @@ def main(): """) return 1 - build_all_include() - build_compile_commands() + idedata = load_idedata("esp8266-tidy") + options = clang_options(idedata) files = [] for path in git_ls_files(['*.cpp']): @@ -116,6 +147,7 @@ def main(): files = split_list(files, args.split_num)[args.split_at - 1] if args.all_headers and args.split_at in (None, 1): + build_all_include() files.insert(0, temp_header_file) tmpdir = None @@ -128,7 +160,7 @@ def main(): lock = threading.Lock() for _ in range(args.jobs): t = threading.Thread(target=run_tidy, - args=(args, tmpdir, task_queue, lock, failed_files)) + args=(args, options, tmpdir, task_queue, lock, failed_files)) t.daemon = True t.start() diff --git a/script/helpers.py b/script/helpers.py index e0ec2d169e..8bf8eec5cc 100644 --- a/script/helpers.py +++ b/script/helpers.py @@ -1,13 +1,14 @@ import codecs -import json import os.path import re import subprocess -import sys +import json +from pathlib import Path root_path = os.path.abspath(os.path.normpath(os.path.join(__file__, "..", ".."))) basepath = os.path.join(root_path, "esphome") -temp_header_file = os.path.join(root_path, ".temp-clang-tidy.cpp") +temp_folder = os.path.join(root_path, ".temp") +temp_header_file = os.path.join(temp_folder, "all-include.cpp") def shlex_quote(s): @@ -33,63 +34,9 @@ def build_all_include(): headers.sort() headers.append("") content = "\n".join(headers) - with codecs.open(temp_header_file, "w", encoding="utf-8") as f: - f.write(content) - - -def build_compile_commands(): - gcc_flags_json = os.path.join(root_path, ".gcc-flags.json") - if not os.path.isfile(gcc_flags_json): - print("Could not find {} file which is required for clang-tidy.".format(gcc_flags_json)) - print( - 'Please run "pio init --ide atom" in the root esphome folder to generate that file.' - ) - sys.exit(1) - with codecs.open(gcc_flags_json, "r", encoding="utf-8") as f: - gcc_flags = json.load(f) - exec_path = gcc_flags["execPath"] - include_paths = gcc_flags["gccIncludePaths"].split(",") - includes = [f"-I{p}" for p in include_paths] - cpp_flags = gcc_flags["gccDefaultCppFlags"].split(" ") - defines = [flag for flag in cpp_flags if flag.startswith("-D")] - command = [exec_path] - command.extend(includes) - command.extend(defines) - command.append("-std=gnu++11") - command.append("-Wall") - command.append("-Wno-delete-non-virtual-dtor") - command.append("-Wno-unused-variable") - command.append("-Wunreachable-code") - - source_files = [] - for path in walk_files(basepath): - filetypes = (".cpp",) - ext = os.path.splitext(path)[1] - if ext in filetypes: - source_files.append(os.path.abspath(path)) - source_files.append(temp_header_file) - source_files.sort() - compile_commands = [ - { - "directory": root_path, - "command": " ".join( - shlex_quote(x) for x in (command + ["-o", p + ".o", "-c", p]) - ), - "file": p, - } - for p in source_files - ] - compile_commands_json = os.path.join(root_path, "compile_commands.json") - if os.path.isfile(compile_commands_json): - with codecs.open(compile_commands_json, "r", encoding="utf-8") as f: - try: - if json.load(f) == compile_commands: - return - # pylint: disable=bare-except - except: - pass - with codecs.open(compile_commands_json, "w", encoding="utf-8") as f: - json.dump(compile_commands, f, indent=2) + p = Path(temp_header_file) + p.parent.mkdir(exist_ok=True) + p.write_text(content) def walk_files(path): @@ -153,3 +100,28 @@ def git_ls_files(patterns=None): output, err = proc.communicate() lines = [x.split() for x in output.decode("utf-8").splitlines()] return {s[3].strip(): int(s[0]) for s in lines} + + +def load_idedata(environment): + platformio_ini = Path(root_path) / "platformio.ini" + temp_idedata = Path(temp_folder) / f"idedata-{environment}.json" + if not platformio_ini.is_file() or not temp_idedata.is_file(): + changed = True + elif platformio_ini.stat().st_mtime >= temp_idedata.stat().st_mtime: + changed = True + else: + changed = False + + if not changed: + text = temp_idedata.read_text() + else: + stdout = subprocess.check_output( + ["pio", "run", "-t", "idedata", "-e", environment] + ) + match = re.search(r'{\s*".*}', stdout.decode("utf-8")) + text = match.group() + + temp_idedata.parent.mkdir(exist_ok=True) + temp_idedata.write_text(text) + + return json.loads(text)