From a295b6ea49f0f1bd5de23bcf506458294f6ada38 Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Tue, 29 Nov 2022 10:21:50 -0500 Subject: [PATCH] build: Use nix to manage dev tooling starting with clang-tidy (#4292) * use nix for clang tidy and clang format * test using nix for clang tidy * check for only errors * allow output from action * dont lint tests with clang-tidy * grouping and prevent false positives * dont warn on magic numbers * rename * add missing change * remove a few more lints * back to 14 * Create .clang-format * Update .clang-format * use src dir to allow -fix * add to format * improve usability of script --- .clang-tidy | 2 +- .github/workflows/lint.yml | 29 ++++---- .scripts/linux/run-clang-tidy.sh | 20 ------ flake.lock | 41 ++++++++++++ flake.nix | 110 +++++++++++++++++++++++++++++++ utl/clang-format.sh | 4 +- 6 files changed, 170 insertions(+), 36 deletions(-) delete mode 100755 .scripts/linux/run-clang-tidy.sh create mode 100644 flake.lock create mode 100644 flake.nix diff --git a/.clang-tidy b/.clang-tidy index 2bfc903576d..9c692c85664 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,5 +1,5 @@ { - "Checks": "-*,readability-*,modernize-*,performance-*,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables,readability-identifier-naming,-modernize-use-trailing-return-type,-readability-identifier-length,-readability-uppercase-literal-suffix", + "Checks": "-*,readability-*,modernize-*,performance-*,cppcoreguidelines-pro-type-member-init,cppcoreguidelines-init-variables,readability-identifier-naming,-modernize-use-trailing-return-type,-readability-magic-numbers,-readability-function-cognitive-complexity,-readability-identifier-length,-readability-uppercase-literal-suffix", "FormatStyle": "file", "WarningsAsErrors": "-*,performance-*,modernize-use-using,readability-braces-around-statements", "CheckOptions": [ diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index af9f0bee978..99393a5c31d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -9,7 +9,7 @@ on: branches: - '*' -concurrency: +concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.sha }} cancel-in-progress: true @@ -138,18 +138,21 @@ jobs: fi run-clang-tidy: name: lint.c++.clang-tidy - container: - image: vowpalwabbit/ubuntu2004-dev:latest runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - with: - submodules: recursive - - name: Install Ninja - shell: bash + - uses: actions/checkout@v3 + - uses: cachix/install-nix-action@v18 + - name: Run clang-tidy run: | - apt update - apt install -y libspdlog-dev libfmt-dev libboost-math-dev - - name: Run clang tidy - shell: bash - run: ./.scripts/linux/run-clang-tidy.sh + echo "::group::nix develop -c vw-clang-tidy" + nix develop -c vw-clang-tidy | tee tidy_out.txt || true + # Prevent false positives for THROW("error: ...") by using the prepended : + grep -A 3 ": error: " tidy_out.txt > tidy_onlyerrors.txt || true + echo "::endgroup::" + if [ -s tidy_onlyerrors.txt ]; then + # file has contents + echo "::group::clang-tidy errors" + cat tidy_onlyerrors.txt + echo "::endgroup::" + exit 11 + fi diff --git a/.scripts/linux/run-clang-tidy.sh b/.scripts/linux/run-clang-tidy.sh deleted file mode 100755 index 83f49a1664d..00000000000 --- a/.scripts/linux/run-clang-tidy.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/bin/bash -set -e -set -x - -SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -REPO_DIR=$SCRIPT_DIR/../../ -cd $REPO_DIR - -# -DCMAKE_EXPORT_COMPILE_COMMANDS=On is manually set because in CMake 3.16, just setting it in the CMakeLists.txt does not work. -cmake -S . -B build -DFMT_SYS_DEP=On -DSPDLOG_SYS_DEP=On -DVW_BOOST_MATH_SYS_DEP=On -DBUILD_TESTING=Off -DVW_BUILD_VW_C_WRAPPER=Off -DCMAKE_EXPORT_COMPILE_COMMANDS=On -run-clang-tidy -p build -quiet -header-filter=vw/* 1>tidy_out.txt 2>tidy_error.txt || true -grep -A 3 "error: " tidy_out.txt > tidy_onlyerrors.txt || true - -if [ -s tidy_onlyerrors.txt ]; then - # file has contents - cat tidy_onlyerrors.txt - exit 11 -else - cat tidy_out.txt -fi diff --git a/flake.lock b/flake.lock new file mode 100644 index 00000000000..8b63424a174 --- /dev/null +++ b/flake.lock @@ -0,0 +1,41 @@ +{ + "nodes": { + "flake-utils": { + "locked": { + "lastModified": 1667395993, + "narHash": "sha256-nuEHfE/LcWyuSWnS8t12N1wc105Qtau+/OdUAjtQ0rA=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "5aed5285a952e0b949eb3ba02c12fa4fcfef535f", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1669597967, + "narHash": "sha256-R+2NaDkXsYkOpFOhmVR8jBZ77Pq55Z6ilaqwFLLn000=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "be9e3762e719211368d186f547f847737baad720", + "type": "github" + }, + "original": { + "id": "nixpkgs", + "type": "indirect" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 00000000000..72b729575cf --- /dev/null +++ b/flake.nix @@ -0,0 +1,110 @@ +{ + # Currently, this flake only contains tools helpful for development. + # Over time, we may add a package definition and other things. + description = "Development utils for Vowpal Wabbit."; + + inputs.flake-utils.url = "github:numtide/flake-utils"; + + outputs = { self, nixpkgs, flake-utils }: + flake-utils.lib.eachDefaultSystem (system: + let pkgs = nixpkgs.legacyPackages.${system}; in + let + generate-compile-commands = '' + echo -n "Generating compile_commands.json... " + rm -rf $TMPDIR/compile_commands_build + mkdir -p $TMPDIR/compile_commands_build + cmake -S . -B "$TMPDIR/compile_commands_build" \ + -DCMAKE_EXPORT_COMPILE_COMMANDS=On \ + -DRAPIDJSON_SYS_DEP=On \ + -DFMT_SYS_DEP=On \ + -DSPDLOG_SYS_DEP=On \ + -DVW_BOOST_MATH_SYS_DEP=On \ + -DVW_ZLIB_SYS_DEP=On \ + -DVW_GTEST_SYS_DEP=On \ + -DVW_EIGEN_SYS_DEP=On \ + -DBUILD_TESTING=Off \ + -DVW_BUILD_VW_C_WRAPPER=Off > cmake_compile_commands_output.txt 2>&1 + if [ $? -ne 0 ]; then + echo "Failed" + echo + cat cmake_compile_commands_output.txt >&2 + exit 1 + else + echo "Done" + rm cmake_compile_commands_output.txt + fi + ''; + in + let + core-dependencies = [ + pkgs.spdlog + pkgs.fmt + pkgs.zlib + pkgs.rapidjson + pkgs.eigen + pkgs.gtest + pkgs.boost + pkgs.cmake + ]; + in + let + python-clang-tidy-package = pkgs.stdenv.mkDerivation { + name = "python-clang-tidy"; + src = pkgs.fetchurl { + url = "https://github.com/llvm/llvm-project/releases/download/llvmorg-14.0.6/clang-tools-extra-14.0.6.src.tar.xz"; + sha256 = "sha256-fPO4/1bGXE0erjxWiD/EpsvD/586FTCnTWbkXScnGGY="; + }; + sourceRoot = "clang-tools-extra-14.0.6.src"; + phases = [ "unpackPhase" "installPhase" "fixupPhase" ]; + propagatedBuildInputs = [ pkgs.python3 pkgs.clang-tools_14 ]; + installPhase = '' + mkdir -p $out/bin + cp clang-tidy/tool/run-clang-tidy.py $out/bin + cp clang-tidy/tool/run-clang-tidy.py $out/bin/run-clang-tidy + cp clang-tidy/tool/clang-tidy-diff.py $out/bin + cp clang-tidy/tool/clang-tidy-diff.py $out/bin/clang-tidy-diff + ''; + }; + in + let + python-clang-format-package = pkgs.stdenv.mkDerivation { + name = "python-clang-format"; + src = pkgs.fetchurl { + url = "https://github.com/llvm/llvm-project/releases/download/llvmorg-14.0.6/clang-14.0.6.src.tar.xz"; + sha256 = "sha256-K1hHtqYxGLnv5chVSDY8gf/glrZsOzZ16VPiY0KuQDE="; + }; + sourceRoot = "clang-14.0.6.src"; + phases = [ "unpackPhase" "installPhase" "fixupPhase" ]; + propagatedBuildInputs = [ pkgs.python3 pkgs.clang-tools_14 ]; + installPhase = '' + mkdir -p $out/bin + cp tools/clang-format/clang-format-diff.py $out/bin + cp tools/clang-format/clang-format-diff.py $out/bin/clang-format-diff + ''; + }; + in + let + clang-tidy-all-script = pkgs.writeShellScriptBin "vw-clang-tidy" '' + ${generate-compile-commands} + ${python-clang-tidy-package}/bin/run-clang-tidy -p $TMPDIR/compile_commands_build -quiet -header-filter=vw/* "$@" + ''; + in + let + clang-tidy-diff-script = pkgs.writeShellScriptBin "vw-clang-tidy-diff" '' + ${generate-compile-commands} + ${python-clang-tidy-package}/bin/clang-tidy-diff -p1 -path $TMPDIR/compile_commands_build -r '^.*\.(cc|h)\$' -quiet -use-color "$@" <&0 + ''; + in + { + formatter = pkgs.nixpkgs-fmt; + devShell = pkgs.mkShell { + packages = [ + python-clang-tidy-package + python-clang-format-package + clang-tidy-all-script + clang-tidy-diff-script + ] ++ core-dependencies; + }; + } + ); +} diff --git a/utl/clang-format.sh b/utl/clang-format.sh index 5bdd3b66180..f12be97fce7 100755 --- a/utl/clang-format.sh +++ b/utl/clang-format.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/usr/bin/env bash # Set environment variable GH_WORKFLOW_LOGGING to output logging that Azure pipelines will interpret as a warning. # Set environment variable WARNING_AS_ERROR to make the script exit with a non-zero code if issues are found. @@ -33,7 +33,7 @@ clang-format --version for FILE in $(find . -type f -not -path './ext_libs/*' -not -path './cs/cli/*' \( -name '*.cc' -o -name "*.h" \) ); do if [[ "$1" == "check" ]]; then - diff $FILE <(clang-format $FILE); + clang-format --dry-run --Werror $FILE if [ $? -ne 0 ]; then ISSUE_FOUND="true" if [[ -v GH_WORKFLOW_LOGGING ]]; then