add packages to check pep257 compliance

[dirk-thomas]

For now all categories for missing docstring are ignored (https://github.com/ament/ament_lint/pull/28/files#diff-8b85cd3ad5183599a6c2c7a769948199R41).

http://ci.ros2.org/job/ros2_batch_ci_linux/369/testReport/

[dirk-thomas]

Please review.

[wjwwood]

Why dose ament_cmake_test need to be a build dependency? I looked for documentation in the ament_cmake_test (https://github.com/ament/ament_cmake/tree/master/ament_cmake_test) package but didn't find anything about it.

[dirk-thomas]

That is already the case for all linter packages. ament_cmake_test e.g. defines the CMake option AMENT_ENABLE_TESTING which is used to check is tests are enabled.

[wjwwood]

Why does ament_cmake_test set AMENT_ENABLE_TESTING, doesn't the build tool/user do that?

https://github.com/ament/ament_tools/blob/master/ament_tools/build_types/ament_cmake.py#L97

It seems strange to me that ament_cmake_test, which is only used when testing, is always required, when that is not the case for other dependencies which are only used for testing.

Also, in other packages ament_cmake_test is a buildtool_depend, should that not be the case here as well?

[dirk-thomas]

ament_cmake_test defines the CMake option ament_tools is setting: https://github.com/ament/ament_cmake/blob/8a2a607e82b2f1d89c24e63e52e81b3f8095e297/ament_cmake_test/ament_cmake_test-extras.cmake#L18
WIthout that there would be no tests in CMake: https://github.com/ament/ament_cmake/blob/8a2a607e82b2f1d89c24e63e52e81b3f8095e297/ament_cmake_test/ament_cmake_test-extras.cmake#L26

buildtool_depend is actually more accurate. But the difference between that and build_depend is only relavant for cross compilation.

[wjwwood]

Wouldn't it make more sense to set that option in ament_cmake_core and make ament_cmake_test a test dependency which is only find_package'ed when testing is enabled?

+1 to merge this pr, but I think having ament_cmake_test as a build dependency doesn't look right.

[dirk-thomas]

ament_cmake_core simply does not know about testing at all. I think this separation makes sense.

Higher level packages anyway only depend on ament_cmake - and not on the inner "detailed" packages.

[wjwwood]

Well seeing as ament_cmake and ament_cmake_pep257 both transitively depend on ament_cmake_test, does this need to be in there at all?

I'd argue that ament_cmake_test should just be in ament_cmake_core anyways, but I'd also say that about the majority of the packages in the ament_cmake repository, so I guess I'm arguing a failing point...

This is what I'd propose:

• ament_cmake depends on ament_cmake_test (already true)
• ament_cmake drops dependencies on things like ament_cmake_{nose, gtest, gmock, ...}
• we recommend users depend on ament_cmake and therefore the option AMENT_ENABLE_TESTING is always available
• the documentation for test suites and linters tell users to buildtool depend on ament_cmake and test depend on the package for the test suite or linter.

This way the AMENT_ENABLE_TESTING option is always there (listed in the cmake gui for example) but other test related packages are never brought in or find_package'd unless it is set externally.

Currently the documentation recommends:

<buildtool_depend>ament_cmake_core</buildtool_depend> <!-- was implied but not stated -->
<build_depend>ament_cmake_test</build_depend>
<test_depend>ament_cmake_pep257</test_depend>

find_package(ament_cmake_core REQUIRED)  # again implied but not stated
find_package(ament_cmake_test REQUIRED)
if(AMENT_ENABLE_TESTING)
  find_package(ament_cmake_pep257 REQUIRED)
  ament_pep257()
endif()

But with the change I suggested:

<buildtool_depend>ament_cmake</buildtool_depend>
<test_depend>ament_cmake_pep257</test_depend>

find_package(ament_cmake REQUIRED)
if(AMENT_ENABLE_TESTING)
  find_package(ament_cmake_pep257 REQUIRED)
  ament_pep257()
endif()

I think this is a more obvious expected usage and it avoids ament_cmake implicitly pulling in testing stuff like support for gtest and nosetest.

Do you see any issues with that?

[dirk-thomas]

Sounds good to me:

• remove gmock/gtest/nose packages from ament_cmake ament_cmake#35
• update documentation for linters #29