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

Enable clang-tidy on all samples #91

Open
torbsorb opened this issue Apr 22, 2020 · 5 comments
Open

Enable clang-tidy on all samples #91

torbsorb opened this issue Apr 22, 2020 · 5 comments
Labels
ci Continuous integration / devops

Comments

@torbsorb
Copy link
Contributor

You should not exclude samples here, then we lose out on testing of many files. Tooling should omit diagnostics for the headers of system libraries, so all additional diagnosticas triggered by this proposal will be in our files and should be fixed.

Originally posted by @nedrebo in #85

@torbsorb
Copy link
Contributor Author

@nedrebo,

We do not install Eigen, PCL nor OpenCV in our CI. That's why we don't run build them, and why I believe they're not linted.

When we run clang-tidy it uses CMakeLists.txt which will check whether (e.g.) Eigen is installed. Is there an acceptable way to skip this check when CMakeLists.txt is invoke with clang-tidy?

I tried the following and it works:

in lint.sh:

...
cmake -GNinja \
    -DLINT_ONLY=ON \
    -DUSE_EIGEN3=ON \
...

in CMakeLists.txt:

...
if(USE_EIGEN3)
    if(LINT_ONLY)
        message(Will lint Eigen3 samples, but do not require Eigen3 installed)
    elseif(NOT DEFINED EIGEN3_INCLUDE_DIR)
...

Is this acceptable, or is there a better way.

Btw, I find that cmake remembers a variable set through -D if not set in a consecutive run. Maybe we should clear cmake-cache between runs? Or, is that some of what allows cmake to only rerun on changes?

@torbsorb
Copy link
Contributor Author

I found -U :) So, we could just add:

...
cmake --build . || exit $?

cmake -ULINT_ONLY "$SOURCE_DIR" || exit $?

@nedrebo
Copy link

nedrebo commented Apr 23, 2020

I am not sure I understand correctly what you ask for, but you must install all headers to be able to lint. Code that cannot compile successfully cannot be analyzed.

@torbsorb
Copy link
Contributor Author

It sort of worked, although you're correct that it will throw an error on missing header. Were able to fix most other issues though. See #92.

Maybe this PR will have to wait until we've fixed the CI so that it installs these dependencies.

@nedrebo
Copy link

nedrebo commented Apr 23, 2020

Yes, I think you should first make all dependencies available in CI, then you don't need to do the weird workaround.

@SatjaSivcev SatjaSivcev added the ci Continuous integration / devops label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration / devops
Projects
None yet
Development

No branches or pull requests

3 participants