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

ament_cmake_clang_tidy: No compilation database files found #283

Open
nnmm opened this issue Jan 5, 2021 · 12 comments · May be fixed by #300
Open

ament_cmake_clang_tidy: No compilation database files found #283

nnmm opened this issue Jan 5, 2021 · 12 comments · May be fixed by #300
Assignees
Labels
help wanted Extra attention is needed

Comments

@nnmm
Copy link

nnmm commented Jan 5, 2021

It's unclear to me how to run clang-tidy in addition to the ament_lint_common linter suite. I'm building my package with --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=1, which creates build/compilation_database.json, but ament_cmake_clang_tidy doesn't seem to pick it up and instead gives the error in the title.

In CMakeLists.txt, I have

find_package(ament_lint_auto REQUIRED)
ament_lint_auto_find_test_dependencies()

and in package.xml

<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_cmake_clang_tidy</test_depend>
<test_depend>ament_lint_common</test_depend>

Is something missing there?

@tylerjw
Copy link
Contributor

tylerjw commented Jan 5, 2021

Based on how it works I believe that error is being caused by it being executed from some working driectory where searching for compile_commands.json is failing. If you look at the documentation for running it directly (https://github.com/ament/ament_lint/blob/master/ament_clang_tidy/doc/index.rst) you see there is a positional argument to pass in the path to use for the search for the compile_commands.json files. I've been looking around for how to pass arguemnts into the cmake ament linters and I've yet to figure it out.

@tylerjw
Copy link
Contributor

tylerjw commented Jan 5, 2021

Based on this line:

list(APPEND cmd ${ARG_UNPARSED_ARGUMENTS})

It looks like the cmake function ament_lint_auto_find_test_dependencies has no way to know that this argument should be passed. I think if I understand it correctly if you want to use ament_clang_tidy you can't use the auto lint feature. Instead in your cmake you can do this:

find_package(ament_cmake REQUIRED)
if(BUILD_TESTING)
  find_package(ament_clang_tidy REQUIRED)
  ament_clang_tidy(${CMAKE_BINARY_DIR})
endif()

I haven't tested this and it is contrary to the documentation for ament_cmake_clang_tidy (https://github.com/ament/ament_lint/blob/master/ament_cmake_clang_tidy/doc/index.rst) as I added the argument ${CMAKE_BINARY_DIR}. Maybe someone who understands this better can respond.

@tylerjw
Copy link
Contributor

tylerjw commented Jan 5, 2021

Looking over the code for ament_lint_auto_find_test_dependencies I'm honestly confused how it calls any of the linters or gets them setup to run. From my reading of it (https://github.com/ament/ament_lint/blob/master/ament_lint_auto/cmake/ament_lint_auto_find_test_dependencies.cmake) it just looks like it calls find_package on each of the linters and appends their names to the list ${PROJECT_NAME}_FOUND_TEST_DEPENDS. Maybe that's all that is required to get them to run when you invoke ctest.

@tylerjw
Copy link
Contributor

tylerjw commented Jan 8, 2021

I tested this in my workspace and I needed this to run clang-tidy:

find_package(ament_cmake REQUIRED)
if(BUILD_TESTING)
  find_package(ament_cmake_clang_tidy REQUIRED)
  ament_clang_tidy(${CMAKE_BINARY_DIR})
endif()

Note there is yet one more difference between this and the instructions. I needed to find_package on the cmake package. The one with the binary ament_clang_tidy as the documentation calls out won't work because it doesn't export a find cmake file for that package. I'll submit a PR to fix this part of the documentation.

The next thing that is broken (and relates to all my other PRs and issues around this) is that there is no way to configure the timeout of this test. It runs for the default 60sec and fails every time. Adding it to a relatively simple ros package (moveit_servo) for testing that only has a handful of compilation units causes it to timeout consistently locally. Is anyone else using this linter? Am I just trying to use it wrong?

@mm318
Copy link
Member

mm318 commented Jan 19, 2021

I don't believe ament_clang_tidy is part of ament_lint_common/ament_lint_auto yet. As far as I know, it is the most recently introduced linter. Making it a default linter could have huge implications on all ROS packages that use ament_linting (like build breakage). It would be something good to bring up on ROS discourse.

@clalancette
Copy link
Contributor

The next thing that is broken (and relates to all my other PRs and issues around this) is that there is no way to configure the timeout of this test. It runs for the default 60sec and fails every time. Adding it to a relatively simple ros package (moveit_servo) for testing that only has a handful of compilation units causes it to timeout consistently locally. Is anyone else using this linter? Am I just trying to use it wrong?

It does indeed look like there is no way to specify a TIMEOUT:

cmake_parse_arguments(ARG "" "TESTNAME;CONFIG_FILE" "" ${ARGN})
. However, it ends up using ament_add_test under the hood which does support a TIMEOUT, so it should be a simple matter of adding a TIMEOUT parameter to the ament_clang_tidy cmake function.

I don't believe ament_clang_tidy is part of ament_lint_common/ament_lint_auto yet. As far as I know, it is the most recently introduced linter. Making it a default linter could have huge implications on all ROS packages that use ament_linting (like build breakage). It would be something good to bring up on ROS discourse.

Right, that's exactly the case. Anything we add to the default will have to be fixed across all of the ROS 2 core packages, plus anything downstream that is using it. We can certainly consider doing that, but it would take some dedicated effort and we would have to do it long before the next release.

@clalancette clalancette added the help wanted Extra attention is needed label Feb 4, 2021
@mm318
Copy link
Member

mm318 commented Feb 9, 2021

I don't believe ament_clang_tidy is part of ament_lint_common/ament_lint_auto yet. As far as I know, it is the most recently introduced linter. Making it a default linter could have huge implications on all ROS packages that use ament_linting (like build breakage). It would be something good to bring up on ROS discourse.

Right, that's exactly the case. Anything we add to the default will have to be fixed across all of the ROS 2 core packages, plus anything downstream that is using it. We can certainly consider doing that, but it would take some dedicated effort and we would have to do it long before the next release.

Can we still add ament_clang_tidy to ament_lint_common but make it controlled by an environment variable or something?

@clalancette
Copy link
Contributor

Can we still add ament_clang_tidy to ament_lint_common but make it controlled by an environment variable or something?

We could do something like that. Before we go down that road, I'm curious to see what the situation would be if we just enabled it on the core packages.

@mm318
Copy link
Member

mm318 commented Feb 10, 2021

Sounds good. I'll try to get something implemented by end of next week.

@tylerjw
Copy link
Contributor

tylerjw commented Feb 10, 2021

Let me know if I can help with this. I imagine you are going to discover with any large codebase running clang-tidy will take ~10x the time it takes to build and very few people would be happy about that in the CI system.

@clalancette
Copy link
Contributor

Let me know if I can help with this. I imagine you are going to discover with any large codebase running clang-tidy will take ~10x the time it takes to build and very few people would be happy about that in the CI system.

If that is really the case, enabling it by default is a non-starter. Our overnight jobs already take somewhere around 4 hours to run, so we couldn't add something that made them take 40 hours.

I think it is still worthwhile to see what the actual penalty is, but I just want to set expectations.

@tylerjw
Copy link
Contributor

tylerjw commented Feb 11, 2021

If I do a full run of clang-tidy over all of the moveit repo (even with --jobs 8) on my laptop it takes ~2 hours. A full build without a fresh cache is about 20min. This is the reason for our approach of only running it over packages with changes in a given PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants