build/install directories also being checked

[dhood]

If I run the linter tests from the directory with the CMakeLists.txt, they start testing the files created during the build step. This makes the test 1) spit out a bunch of junk and 2) fail.

For example,

~/ros2_ws/src/ros2/rclpy/rclpy$ ament test . --ctest-args -L linter

yields

The following tests FAILED:
      2 - copyright (Failed)
      4 - cpplint (Failed)
      5 - lint_cmake (Failed)
      6 - pep257 (Failed)
      9 - uncrustify (Failed)

with errors such as

2: -- run_test.py: invoking following command in '/home/dhood/ros2_ws/src/ros2/rclpy/rclpy':
2:  - /home/dhood/ros2_ws/install_isolated/ament_copyright/bin/ament_copyright --xunit-file /home/dhood/ros2_ws/src/ros2/rclpy/rclpy/build/rclpy/test_results/rclpy/copyright.xunit.xml
2: build/rclpy/CMakeFiles/2.8.12.2/CMakeCCompiler.cmake: could not find copyright notice
2: build/rclpy/rclpy/__init__.py: could not find copyright notice

and

9: -- run_test.py: invoking following command in '/home/dhood/ros2_ws/src/ros2/rclpy/rclpy':
9:  - /home/dhood/ros2_ws/install_isolated/ament_uncrustify/bin/ament_uncrustify --xunit-file /home/dhood/ros2_ws/src/ros2/rclpy/rclpy/build/rclpy/test_results/rclpy/uncrustify.xunit.xml
9: Code style divergence in file 'build/rclpy/CMakeFiles/2.8.12.2/CompilerIdC/CMakeCCompilerId.c'

I can fix this by instead calling

~/ros2_ws/src/ros2/rclpy$ ament test rclpy --ctest-args -L linter

but I wanted to make a note of it as I don't feel like I'll be the only person who will ever try to do this, and it wasn't immediately obvious why my tests were failing.

[dirk-thomas]

I assume ~/ros2_ws/src/ros2/rclpy/rclpy contains a package.xml file? ament is supposed to be called for a workspace and not from within a package folder. I am pretty sure nobody has tried that before.

Also what are the steps you did before since I get some different result?

[dhood]

yes ~/ros2_ws/src/ros2/rclpy/rclpy has a package.xml file. it's just a habit of mine to keep the build directory level with the CMakeLists.txt because I'm used to calling cmake directly instead of using catkin workspaces

The issue happens for me on fresh ros2 source files (and any subsequent time). Originally, there are just these files/directories: https://github.com/ros2/rclpy/tree/master/rclpy in ~/ros2_ws/src/ros2/rclpy/rclpy
and then after running
~/ros2_ws/src/ros2/rclpy/rclpy$ ament test . --ctest-args -L linter
the install and build directories are created at the same level as the src directory.

if I never do that step and instead run ament one level higher and specify where the src directory is, the build and install folders are not tested (presumably because they're out of reach of the crawlers).

[dirk-thomas]

ament won't work in this case as you would like it to. I think the problem is with the package itself. If the package would like to also test the generated code it can do so by calling the linter appropriately. Currently the linter might not have enough options to do it but the package can not rely on an in-source build. It should definitely also work with an out-of-source-build (which is what ament is doing).

[dhood]

I haven't found a package yet that doesn't cause this to happen, so I'm not sure it's a package specific thing (have tried ament_copyright, ros2/demos, ros2/examples, ros2/rclcpp, ros2/rclpy). Do you have any in mind that you think should not cause this?

Do I understand you correctly that you think the package might be purposefully testing the build and install directories so that it can check the formatting of all files generated? If that's the case, I would have thought that the result should be the same regardless of where the build/install directories are, since it could get access to those variables somehow. So I feel like this might just be lint tests finding files they're not supposed to.

The reason I thought it was worth mentioning is because, while I see that ament doesn't work in this case, I wonder if it is just an arbitrary restriction that cause it to not work, and so is worth changing for the 'freedom' of the user. If it's just something that should not be done for other reasons, what do you say about me starting to compile an FAQ list so it can be searched if people run into the same situation?

[dirk-thomas]

Currently most linters are checking for certain files (e.g. ending in .py) recursively from the directory the CMakeLists.txt is in. That is the default behavior if you don't specify something custom. So for an in-source build the build folder will be considered as well since it is a subfolder.

We could change the default behavior of the linters to not check a subfolder called build by default (maybe also ignore install / install_isolated but that might go to far?). That would make the result of in-source builds consistent with out-of-source builds.

We could also additionally add the CMAKE_BINARY_DIR to always check files generated in the build space.

Invoking ament within a single package is imo not really useful. The purpose of the tool is to build multiple packages in the correct order. Using it for an in-source build of a single package is simply not what it is designed for. I think the documentation already describes how to use the tool. It might be worth to mention that case explicitly but imo documenting cases which it shouldn't be used is an endless endeavor because users will try things you can never imagine 😉

[dhood]

ha - good point 😄

I'd support ignoring build and build_isolated directories so that ament build/test can be called from a directory which has a src directory, whether that's the src directory of one specific package or of the entire workspace.

I feel that it seems natural to then also ignore install and install_isolated since they are also causing tests to fail in this situation.

If you agree, I'll change the defaults of the ament_lint packages to ignore all four, and maybe at a later date add the option to test generated files.

[dirk-thomas]

Sounds good to me.

[mikaelarguedas]

@dhood do you want to stay assigned to this ?

[dhood]

nah we can label it as help wanted though if someone else is interested

[dirk-thomas]

I don't think ignoring a list of "known" folder names is reasonable for this. Simply because users can build in custom folders like build_release / build_debug.

I tried to reproduce the problem by running the following command in the rclpy source folder:

ament test . --ctest-args -L linter

It didn't check or warn about any of the files in the created build subfolder. @dhood Can you please clarify how to reproduce this. Otherwise I would suggest to close the ticket.

[mikaelarguedas]

I was able to reproduce this locally following the instructions of this issue.

in a fresh terminal:

. <MY_WORKSPACE>/install_isolated/local_setup.bash
cd <MY_WORKSPACE>/src/ros2/rclpy
ament test . --ctest-args -L linter

result:

The following tests FAILED:
	  2 - copyright (Failed)
	  4 - cpplint (Failed)
	  6 - lint_cmake (Failed)
	  8 - uncrustify (Failed)
Errors while running CTest
Makefile:127: recipe for target 'test' failed
make: *** [test] Error 8

[dirk-thomas]

If I run the command in the root of the repo it passes. But when I run it in the rclpy folder containing the package I get the same errors.

[dirk-thomas]

#81 addresses the problem with build spaces.

The install space is still a problem if e.g. generated code doesn't contain a copyright and/or license. In order to address that use case the ament_copyright needs to look for a known marker (e.g. intentionally no copyright and license) and in that case not report an error. Additional all generated code without a copyright / license would need to be updated to include that phrase.

[mikaelarguedas]

Remark for the other linters: We will still have errors reported when the generated code doesnt comply with the linters default rules (for example the generated code doesnt enforce line length)

[dirk-thomas]

I am going to close this for now since we don't plan to support in-source builds. #81 already provides enough improvement that you can use the linters in-source.