Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality Tooling #12

Open
8 of 14 tasks
Mischback opened this issue Jun 7, 2022 · 3 comments
Open
8 of 14 tasks

Code Quality Tooling #12

Mischback opened this issue Jun 7, 2022 · 3 comments
Assignees
Labels
area/repository Affects the repository (e.g. structure) type/feature New feature or request
Milestone

Comments

@Mischback
Copy link
Owner

Mischback commented Jun 7, 2022

In order to increase the overall code quality, automate the usage of code quality tools (using pre-commit).

This repository contains primarily C code, the actual software for the ESP32. Some utility tools are implemented in different languages, though, e.g. Python.

@Mischback Mischback added type/feature New feature or request area/repository Affects the repository (e.g. structure) labels Jun 7, 2022
@Mischback Mischback self-assigned this Jun 7, 2022
@Mischback Mischback changed the title Code Quality Code Quality Tooling Jun 7, 2022
@Mischback
Copy link
Owner Author

Mischback commented Jun 8, 2022

DOES NOT WORK!

All of the following stuff does not work as expected. The referenced pre-commit hooks rely on having the tools available on the actual host system, which pretty much renders them useless as they may be provided as local hooks.

Further investigation reveals nothing similar to this! C / C++ developers seem to handle this in a different way!

Integrate clang-tidy / cppcheck and include-what-you-use

  • include this to pre-commit config: https://github.com/Takishima/cmake-pre-commit-hooks
  • Test Scenario A
    • fullclean, especially removing the build directory
    • run one of the llvm-based tools THIS IS EXPECTED TO FAIL BADLY!
    • build the project (this also creates the required compilation database)
    • run one of the llvm-based tools: This should work!
  • Test Scenario B
    • fullclean
    • build
    • remove content of build directory, except the compilation database (check README of https://github.com/Takishima/cmake-pre-commit-hooks: validation of the build directory is done by a cache file!)
    • run one of the llvm-based tools: This should work!

At this point, we have successfully verified, that the only requirement for running llvm-based tools is the compilation database. This enables us to do the following things:

  • create a GitHub Actions workflow, that generates a compilation database (there is a docker-based ESP-IDF action!), upload it as artifact, download the artifact to actually run the linters
  • configure the hooks to look for the build directory in a specific place for CI (e.g. ci_build), this is where the compilation database may be provided (see documentation of https://github.com/Takishima/cmake-pre-commit-hooks)
  • enable the hooks to (re-) generate the compilation database locally; this might require the use of some advanced pre-commit configuration, as the cmake command must be run with specific environment variables (in order to pick up all the ESP-IDF magic)

@Mischback
Copy link
Owner Author

Mischback commented Jun 9, 2022

Does not work!

Ok, this did not work! Making clang/llvm-based tools work with ESP-IDF is a much bigger effort than I like to invest at this point.

However, my development system (Debian Bullseye) has a very outdated version of cppcheck, that does work without the llvm-based tooling in place, so at least that one is included.

Integrate include-what-you-use locally

Intergrate in CI

Both methods are working locally, but Method A will be difficult in CI, because the whole ESP-IDF toolchain would have to be put in place.

  • Build a Github Action workflow
    • include building (https://github.com/espressif/esp-idf-ci-action) as the first step and fetch the required artifacts to make the linters work (Github upload-artifact action?)
    • Method A: System installation
      • keep track of all required linters in a dedicated file (e.g. requirements/apt/linters.txt)
      • install these packages using apt as part of the workflow
      • PROBLEM Linux runner is ubuntu, development machine is debian, so the packages may be out of sync (and may be means will be)
      • download the compilation database (Github download-artifact action?) to the correct path
      • run the linters via pre-commit, using dedicated make recipes (see mischback/django-calingen)
    • Method B: Docker container
      • keep track of all required linters in a dedicated file (e.g. requirements/apt/linters.txt)
      • build a dedicated Dockerfile, based on debian to provide identical versions of the linters
      • PROBLEM Additional complexity because of Docker
      • download the compilation database (Github download-artifact action?) to the correct path
      • PROBLEM How to run the linters? (Reference: github/super-linter)

@Mischback
Copy link
Owner Author

Mischback commented Jun 9, 2022

Installation Sizes

  • include-what-you-use
    • Debian package name: iwyu
    • Bullseye version: 0.15
    • Installation Size: 277 MB
  • clang-tidy
    • Debian package name: clang-tidy
    • Bullseye version: 11.0.1
    • Installation Size: 298 MB
  • cppcheck
    • Debian package name: cppcheck
    • Bullseye version: 2.3
    • Installation Size: 11 MB

Combined

apt-get install iwyu clang-tidy cppcheck

315 MB

@Mischback Mischback mentioned this issue Jun 20, 2022
14 tasks
@Mischback Mischback added this to the Release 1.0.0 milestone Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/repository Affects the repository (e.g. structure) type/feature New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant