Skip to content

Run Linter only on Changed Files

Simon Spannagel requested to merge tidyfast into master

The single job which takes longest in our CI pipeline is linting the whole code base. Since mostly a few files have been changed, this is obviously very inefficient. This MR adds an additional target called check-lint-diff which selectively runs clang-tidy only on those files which differ in the current job with respect to the master branch.

This is blazingly fast, here a side-by-side comparison for a full linting job and the diff'ed one for a single touched source file:

image image

From 40min down to 40sec.

It still works, so errors in the changed files are reported correctly. I tested this by introducing some things linters don't like, et voila:

$ ninja check-lint-diff 2> /dev/null
94520 warnings generated.
/builds/allpix-squared/allpix-squared/src/modules/DepositionCosmics/CosmicsGeneratorActionG4.cpp:33:5: error: 'auto setup' can be declared as 'auto *setup' [readability-qualified-auto,-warnings-as-errors]
    auto setup = new CRYSetup(config_.get<std::string>("_cry_config"), config_.get<std::string>("data_path"));
    ^~~~~
    auto *
/builds/allpix-squared/allpix-squared/src/modules/DepositionCosmics/CosmicsGeneratorActionG4.cpp:38:19: error: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast,-warnings-as-errors]
               << reinterpret_cast<std::uintptr_t>(CLHEP::HepRandom::getTheEngine());
                  ^
2 warnings treated as errors
ninja: build stopped: subcommand failed.
Cleaning up project directory and file based variables 00:01
ERROR: Job failed: exit code 1

(via https://gitlab.cern.ch/allpix-squared/allpix-squared/-/jobs/18475010)

The new job manually checks using git diff which files have been altered and generates individual clang-tidy calls for every file. It's a bit clunky that we rely on the manually assembled list of files (from formatting actually) to check which ones are source files we should care about - but it works for now. The old job uses run-clang-tidy in conjunction with the compilation database from CMake. So if we could parse the compilation database, that'd be sugar - but it's not straight forward and not really necessary.

Now, it would be great to have this in merge requests to automatically pick the correct target branch from the CI environment. This is possible with Merge Request Pipelines but I've played with them before and somehow don't get along with them.

Hence, we have two jobs now:

  • fmt:cc7-llvm-lint-diff - runs the short linting on changed files for every commit
  • fmt:cc7-llvm-lint - the full linting of the entire codebase, now only runs for schedules (nightlies) and tags

I think this should provide a nice setup and speed things up a bit on the CI side.

Edited by Simon Spannagel

Merge request reports